[pulseaudio-discuss] Revisiting raop2 patches

[Hajime Fujita]

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.

> 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