[pulseaudio-discuss] Revisiting raop2 patches

[Anton Lundin]

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>

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.

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 ?

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.

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?

raop: Update and standardise source file headers (d0483146)
Removes KTH from the copyright header. Is that a good thing to do?

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