[pulseaudio-discuss] Revisiting raop2 patches

[Hajime Fujita]

Hi Anton,

> On Oct 26, 2016, at 9:58 PM, Hajime Fujita <crisp.fujita at nifty.com> wrote:
> 
> Hi Anton,
> 
> Thank you for your effort on taking a look at the patches!
> 
>> On Oct 25, 2016, at 3:47 PM, Anton Lundin <glance at acc.umu.se> wrote:
>> 
>> On 25 October, 2016 - Tanu Kaskinen wrote:
>> 
>>> On Mon, 2016-10-24 at 22:09 -0500, Hajime Fujita wrote:
>>>> On Oct 24, 2016, at 4:05 PM, Anton Lundin <glance at acc.umu.se> wrote:
>>>>> On 24 October, 2016 - Colin Leroy wrote:
>>>>>> On 24 October 2016 at 20h58, Tanu Kaskinen wrote:
>>>>>>> It would be great to have more people reviewing patches, but I don't
>>>>>>> know how to acquire those people. I don't think the reviews have to
>>>>>>> necessarily be done by someone with a title of "maintainer", but on
>>>>>>> the other hand, giving much trust to drive-by contributors seems risky
>>>>>>> too...
>>>>>> 
>>>>>> Reviewing is hard when you don't have an extensive knowledge of the
>>>>>> codebase... I'd propose to do it, but I'd suck at it - "yeah, seems
>>>>>> fine to me" :D
>>> 
>>> Reviewing is a good way to gain knowledge of the codebase, though. Even
>>> if the reviews weren't very thorough in the beginning, I'd still be
>>> willing to pay that price if the new reviewer was going to stay around
>>> for a longer time. (When you said "I'd propose to do it", you probably
>>> didn't mean becoming a long-term reviewer, but I thought I should make
>>> this point anyway.)
>>> 
>>>>> I think we can file the raop2 code under a quite special category. It
>>>>> doesn't have a big inpact outside of the raop2 code, and those bits are
>>>>> quite trivial to review. I just read them my self and they looked ok to
>>>>> me =)
>>> 
>>> Are there any patches in the set that you'd be comfortable if I tagged
>>> them with "Reviewed-by: Anton Lundin <glance at acc.umu.se>"?
>>> 
>> 
>> I see what you did there Tanu =)
>> 
>> It sort of pushed my buttons so here we go:
>> 
>> 
>> Support IPv6 address in pa_socket_client_new_string() (51d0fed4)
>> Reviewed-by: Anton Lundin <glance at acc.umu.se>
>> 
>> rtp: Freeing ioline when disconnecting (256724cc)
>> Reviewed-by: Anton Lundin <glance at acc.umu.se>
>> 
>> raop: Cosmetic fixes / Match coding style (1c9ccf74)
>> Converts the already messed up ö in Högskolan to a Danish ö ø instead of
>> the Swedish ö.
>> The text looks to have bin messed up way back in time by Colin, and I've
>> even saw traces that svn was involved, so what unholy things happed
>> there we should probably just leave to future archaeological
>> investigations to figure out.
>> I'm sending a patch. Rebase / merge the coding style patch on top of that
>> and I'm happy with it. 
>> Reviewed-by: Anton Lundin <glance at acc.umu.se>
> 
> Thank you for addressing this. I hope I didn’t make a mistake in a later commit to bring back this error...
> 
>> raop: Add pulsecore/core-utils a pa_str_in_list function (6835e40a)
>> Reviewed-by: Anton Lundin <glance at acc.umu.se>
>> 
>> raop: Parse server capabilities on discovery (88cbec61)
>> Why was the device part dropped from the device name? Nothing in the
>> commit message says anything about that. The device part was dropped
>> from the parsing to, even when some other no-op parsing blocks are left
>> there for future reference.
> 
> I actually asked this to Martin (the original author of the patch) before, and he did not remember the reason anymore.
> Now that nobody can explain the reason for change, maybe I should rewrite the code so that it makes sense to everyone.
> 
>> Also, value is set to NULL in some parsing blocks and not in others.
>> Some blocks strdup(value), and let avahi_free(value) free it, others
>> "steal" value and set value = NULL. I'd feel more comfortable with all
>> the code paths strdup'ing them selfs. Even, why not use pa_xstrdup ?
> 
> Will take a look.

So I took a look at this.

To me, this code makes some sense.
Strdup is used when the raop module generates a new string based on information in a value given by Avahi.
Value “stealing” happens when the raop module uses an Avahi string as-is. In this case strdup’ing is essentially a waste of memory and allocation/free cost. (Nobody probably cares about the cost here though)

“strdup” should be “pa_xstrdup”, I agree.

> 
>> I think this patch is placed to early in the patch stack to. It adds
>> options to the module-raop-sink which it can't parse.
> 
> Exactly. I reordered this to resolve dependency.
> 
>> rtp: New pa_rtsp_options function (1e7e70e4)
>> Reviewed-by: Anton Lundin <glance at acc.umu.se>
>> 
>> rtp: Random seq number at the beginning of the session (8e3c0047)
>> Reviewed-by: Anton Lundin <glance at acc.umu.se>
>> 
>> rtp: Introduce pa_rtsp_exec_ready() (5c4185ee)
>> Reviewed-by: Anton Lundin <glance at acc.umu.se>
>> 
>> raop: Add a MD5 hashing fuction (5f089f3)
>> Also touches the KTH text, and should be corrected.
>> The commit message could be clearer to. Maybe split the MD5 and the
>> base64 parts into two patches?
> 
> Sounds like a good idea.
> 
>> raop: Update and standardise source file headers (d0483146)
>> Removes KTH from the copyright header. Is that a good thing to do?
> 
> Good point. Will bring back the copyright line.
> 
>> The rest are just to big, hard and complicated for a sane review, at
>> least by me. One would need to really know both PA and raop to
>> understand those, and I'd guess even then it will be hard.
>> 
>> The upside is that they are contained to raop, so If I had a saying, its
>> better to just take them and improve them in mainline.
>> 
>> 
>> I've run the code against both my Denon 1912 and my nightly build Kodi
>> and it works. Is it perfect? Not even close. It's not that hard to get
>> it to lock up spewing "memblock.c: Pool full" but in the simple case it
>> works, which is way more than the current raop code.
>> 
>> 
>> It turned out to be a bit more reviewing than planed to, and it became a
>> quite messy email but I'll hope it makes sense.
>> 
>> 
>> //Anton
>> 
>> 
>> -- 
>> Anton Lundin	+46702-161604
>> _______________________________________________
>> pulseaudio-discuss mailing list
>> pulseaudio-discuss at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
> 
> 
> Thanks,
> Hajime
> 
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss