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
