Re: [pulseaudio-discuss] Revisiting raop2 patches

[Hajime Fujita]

Anton Lundin wrote:
> On 30 October, 2016 - Hajime Fujita wrote:
>>
>>> On Oct 26, 2016, at 9:58 PM, Hajime Fujita  wrote:
>>>
 On Oct 25, 2016, at 3:47 PM, Anton Lundin  wrote:
>>>
 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.
>>
> 
> Yea, its some added cost in the discovery process, the thing that stands
> out to me is that the code paths are different. Different means
> complexity and complexity menas potential for bugs.
> 
> Yea, its a simple case, but I don't know the allocation rules for
> avahi-strings. Its better to handle all of that logic right there and
> then, and let PA own the strings from there and onwards.
> 
> It's also the case of keeping track of whats avahi-memory and whats
> pa-memory. I'd guess pa_xfree() and avahi_free() are compatible wrappers
> ontop of free(), but who knows. Some day some crazy person might change
> one of them to be a completely different beast, and things will start to
> behave strangely.
> 
> In such a code path as this one, I'd vote for clarity and robustness
> every day of the week.
> 
> 
> One can clearly see that module-raop-discover.c inherited alot of code
> from module-zeroconf-discover.c, and this odity is there to. The same
> goes for ex. that the hashmap that keeps track of all loaded raop
> modules are called tunnels.

Okay, I understand your concern really is a valid point. I don't have strong 
preference for either side, so simply follow your suggestion.


Thanks,
Hajime
 
> //Anton - who clearly hears the bikeshedding warning bells going off
> 
> 

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Hajime Fujita]

So now I have a new set of patches which (hopefully) address all of Anton's 
following comments.
Now the patches are rebased against `next`, as some of the raop patches have 
already been merged here.

I think the standard practice is to rebase patches on top of master, but for 
this case can I simply push the patches on top of next?


Thanks,
Hajime

Anton Lundin 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  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 "?
>>
> 
> 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 
> 
> rtp: Freeing ioline when disconnecting (256724cc)
> Reviewed-by: Anton Lundin 
> 
> 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 
> 
> raop: Add pulsecore/core-utils a pa_str_in_list function (6835e40a)
> Reviewed-by: Anton Lundin 
> 
> 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 
> 
> rtp: Random seq number at the beginning of the session (8e3c0047)
> Reviewed-by: Anton Lundin 
> 
> rtp: Introduce pa_rtsp_exec_ready() (5c4185ee)
> Reviewed-by: Anton Lundin 
> 
> 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 

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Anton Lundin]

On 30 October, 2016 - Hajime Fujita wrote:
> 
> > On Oct 26, 2016, at 9:58 PM, Hajime Fujita  wrote:
> > 
> >> On Oct 25, 2016, at 3:47 PM, Anton Lundin  wrote:
> > 
> >> 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.
>

Yea, its some added cost in the discovery process, the thing that stands
out to me is that the code paths are different. Different means
complexity and complexity menas potential for bugs.

Yea, its a simple case, but I don't know the allocation rules for
avahi-strings. Its better to handle all of that logic right there and
then, and let PA own the strings from there and onwards.

It's also the case of keeping track of whats avahi-memory and whats
pa-memory. I'd guess pa_xfree() and avahi_free() are compatible wrappers
ontop of free(), but who knows. Some day some crazy person might change
one of them to be a completely different beast, and things will start to
behave strangely.

In such a code path as this one, I'd vote for clarity and robustness
every day of the week.


One can clearly see that module-raop-discover.c inherited alot of code
from module-zeroconf-discover.c, and this odity is there to. The same
goes for ex. that the hashmap that keeps track of all loaded raop
modules are called tunnels.


//Anton - who clearly hears the bikeshedding warning bells going off


-- 
Anton Lundin+46702-161604
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Hajime Fujita]

Hi Anton,

> On Oct 26, 2016, at 9:58 PM, Hajime Fujita  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  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  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 "?
>>> 
>> 
>> 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 
>> 
>> rtp: Freeing ioline when disconnecting (256724cc)
>> Reviewed-by: Anton Lundin 
>> 
>> 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 
> 
> 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 
>> 
>> 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 
>> 
>> rtp: Random seq number at the beginning of the session (8e3c0047)
>> Reviewed-by: Anton Lundin 
>> 
>> rtp: Introduce pa_rtsp_exec_ready() (5c4185ee)
>> Reviewed-by: Anton Lundin 
>> 
>> 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 

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Tanu Kaskinen]

On Tue, 2016-10-25 at 22:47 +0200, Anton Lundin wrote:
> On 25 October, 2016 - Tanu Kaskinen wrote:
> > Are there any patches in the set that you'd be comfortable if I tagged
> > them with "Reviewed-by: Anton Lundin "?
> > 
> 
> 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 
> 
> rtp: Freeing ioline when disconnecting (256724cc)
> Reviewed-by: Anton Lundin 
> 
> 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 
> 
> raop: Add pulsecore/core-utils a pa_str_in_list function (6835e40a)
> Reviewed-by: Anton Lundin 
> 
> 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 
> 
> rtp: Random seq number at the beginning of the session (8e3c0047)
> Reviewed-by: Anton Lundin 
> 
> rtp: Introduce pa_rtsp_exec_ready() (5c4185ee)
> Reviewed-by: Anton Lundin 

Thanks a lot! I pushed the patches that got your ok to the "next"
branch ("master" is frozen at the moment).

-- 
Tanu

https://www.patreon.com/tanuk
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [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  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 "?
> 

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 

rtp: Freeing ioline when disconnecting (256724cc)
Reviewed-by: Anton Lundin 

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 

raop: Add pulsecore/core-utils a pa_str_in_list function (6835e40a)
Reviewed-by: Anton Lundin 

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 

rtp: Random seq number at the beginning of the session (8e3c0047)
Reviewed-by: Anton Lundin 

rtp: Introduce pa_rtsp_exec_ready() (5c4185ee)
Reviewed-by: Anton Lundin 

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
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Tanu Kaskinen]

On Mon, 2016-10-24 at 22:09 -0500, Hajime Fujita wrote:
> On Oct 24, 2016, at 4:05 PM, Anton Lundin  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 "?

> That sounds interesting. Not sure if it’s a good idea to essentially
> “skipping" reviews for non-raop2 patches, but it makes a lot of sense
> to focus on the patches that touch the core and other modules.
> 
> Actually as an author of the patches my expectation for the review
> was to make sure
> a) we didn’t screw up when making changes to other parts of PA
> b) naming conventions are acceptable
> c) pulsecore API usages are correct
> 
> Any other concerns from maintainer’s point of view? I think this is
> also a good opportunity for me (and potentially others) to learn
> about the review process.

Well, I don't really have a checklist in my mind when doing reviews,
but there's one point that gets easily forgotten by people who write
patches (and sometimes I also forget to check it when reviewing): the
commit message should clearly explain why the change is made, what
problem it tries to solve.

Regarding the code itself, I read the changes and try to understand why
each individual change is done. If I can't understand, I'll ask for
clarification. While figuring out how the code works, I may get ideas
about how to do it better, or weird formatting or "bad" naming may
irritate enough for me to complain. Also, I try all the time to think
of ways how the code may break - maybe a pointer dereference isn't
always safe, or maybe allocated resources aren't always freed properly
etc.

> > On the other hand, the raop2 code itself is quit the opposite. It
> > requires understanding of raop2 and pa.
> > 
> > Here comes the point: The current raop2 code is pretty much unusable.
> > There are probably very few who have such old devices that they work
> > with that code, and the changes only affects raop2 users.
> 
> This is a very valid point. I’d say this was already true three years
> ago when I started working on this project. And it was so
> disappointing to know that module-raop-sink didn’t work with most of
> the raop devices in the market despite its name. That’s the why I
> started working on this.
> 
> > I'd love to see the code merged, and even with the
> > module-raop-discover module disabled by default. That way this change
> > only affects users who really wants and uses this feature, and probably
> > need this code to get their gear working.
> 
> FYI: in my understanding it’s already disabled by default.

Yes, it is.

> > So my suggestion is to take a look at the non-raop2 patches so the
> > maintainers are happy with those, and merge the lot.

Indeed, that may be a better alternative than keeping the patches
waiting for review indefinitely.

-- 
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Hajime Fujita]

Corin, Tanu, and Anton,

Thank you folks for your responses!

> On Oct 24, 2016, at 4:05 PM, Anton Lundin  wrote:
> 
> On 24 October, 2016 - Colin Leroy wrote:
> 
>> On 24 October 2016 at 20h58, Tanu Kaskinen wrote:
>> 
>> Hi, 
>> 
>>> 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
>> 
> 
> 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 =)

That sounds interesting. Not sure if it’s a good idea to essentially “skipping" 
reviews for non-raop2 patches, but it makes a lot of sense to focus on the 
patches that touch the core and other modules.

Actually as an author of the patches my expectation for the review was to make 
sure
a) we didn’t screw up when making changes to other parts of PA
b) naming conventions are acceptable
c) pulsecore API usages are correct

Any other concerns from maintainer’s point of view? I think this is also a good 
opportunity for me (and potentially others) to learn about the review process.

> On the other hand, the raop2 code itself is quit the opposite. It
> requires understanding of raop2 and pa.
> 
> Here comes the point: The current raop2 code is pretty much unusable.
> There are probably very few who have such old devices that they work
> with that code, and the changes only affects raop2 users.

This is a very valid point. I’d say this was already true three years ago when 
I started working on this project. And it was so disappointing to know that 
module-raop-sink didn’t work with most of the raop devices in the market 
despite its name. That’s the why I started working on this.

> I'd love to see the code merged, and even with the
> module-raop-discover module disabled by default. That way this change
> only affects users who really wants and uses this feature, and probably
> need this code to get their gear working.

FYI: in my understanding it’s already disabled by default.

> So my suggestion is to take a look at the non-raop2 patches so the
> maintainers are happy with those, and merge the lot.
> 
> 
> //Anton
> 
> 
> -- 
> Anton Lundin  +46702-161604
> ___
> pulseaudio-discuss mailing list
> pulseaudio-discuss@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Anton Lundin]

On 24 October, 2016 - Colin Leroy wrote:

> On 24 October 2016 at 20h58, Tanu Kaskinen wrote:
> 
> Hi, 
> 
> > 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
> 

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 =)

On the other hand, the raop2 code itself is quit the opposite. It
requires understanding of raop2 and pa.

Here comes the point: The current raop2 code is pretty much unusable.
There are probably very few who have such old devices that they work
with that code, and the changes only affects raop2 users.

I'd love to see the code merged, and even with the
module-raop-discover module disabled by default. That way this change
only affects users who really wants and uses this feature, and probably
need this code to get their gear working.


So my suggestion is to take a look at the non-raop2 patches so the
maintainers are happy with those, and merge the lot.


//Anton


-- 
Anton Lundin+46702-161604
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Colin Leroy]

On 24 October 2016 at 20h58, Tanu Kaskinen wrote:

Hi, 

> 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

-- 
Colin


pgp7e38WNLNgR.pgp
Description: OpenPGP digital signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Tanu Kaskinen]

On Sun, 2016-10-23 at 21:48 -0500, Hajime Fujita wrote:
> Hello,
> 
> This is to revisit the raop2 support patches for module-raop-sink, to
> extend the existing module-raop-sink so it supports much recent protocol
> (and thus much broader range of devices.)
> 
> General background can be seen here:
> https://lists.freedesktop.org/archives/pulseaudio-discuss/2016-January/025322.html
> 
> I truly understand that there's a huge shortage in manpower to review the
> patches, and that raop is not a high priority thing in the project.
> 
> However, there does exist many users who have been waiting for this feature
> to become available in PulseAudio, and actually some of them have been
> using this patch for daily basis. Having these patches upstreamed will
> surely benefit these users and make PulseAudio more competent software
> product.

Yes, it would be great to get this stuff merged. Thanks for not giving up!

> If there's anything I can help to make the review process easier and faster
> (other than rebasing the existing patches to the latest master), please do
> let me know.

The problem is that only me and Arun do reviews. I'm currently
concentrating on fixing the release blocker bugs, and Arun has been
busy with non-PA things lately (I hope that won't be a permanent
situation). After we get the release candidate out, I can start doing
reviews again, but I can't promise any particular timeline for
reviewing your patches. I can only promise not to forget about them.

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...

If you (or anyone else reading this!) would be interested in becoming a
regular reviewer, I think we can make that happen.

-- 
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Revisiting raop2 patches

[Colin Leroy]

Hello everybody, Hajime,

> I truly understand that there's a huge shortage in manpower to review
> the patches, and that raop is not a high priority thing in the
> project.

Thanks a lot, again, for your persistence with this patchset :) I still
use it daily and with no adverse effect rebased against PA 9.0 if I
remember correctly.

I would love to help getting this upstreamed but I don't really know
what more I could do?

Thanks,
-- 
Colin
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss