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
