[pulseaudio-discuss] [PATCH v4 1/2] client audio: Support memfd transport

[Arun Raghavan]

On 25 April 2016 at 18:20, Ahmed S. Darwish <darwish.07 at gmail.com> wrote:
> Hi Arun :-)
>
> On Mon, Apr 25, 2016 at 04:30:22PM +0530, Arun Raghavan wrote:
>> On Fri, 2016-04-15 at 23:06 +0200, Ahmed S. Darwish wrote:
> ...
>> > @@ -482,6 +487,7 @@ static void setup_complete_callback(pa_pdispatch *pd, uint32_t command, uint32_t
>> >          case PA_CONTEXT_AUTHORIZING: {
>> >              pa_tagstruct *reply;
>> >              bool shm_on_remote = false;
>> > +            bool memfd_on_remote = false;
>> >
>> >              if (pa_tagstruct_getu32(t, &c->version) < 0 ||
>> >                  !pa_tagstruct_eof(t)) {
>> > @@ -500,7 +506,15 @@ static void setup_complete_callback(pa_pdispatch *pd, uint32_t command, uint32_t
>> >                 not. */
>> >              if (c->version >= 13) {
>> >                  shm_on_remote = !!(c->version & 0x80000000U);
>> > -                c->version &= 0x7FFFFFFFU;
>> > +
>> > +                /* Starting with protocol version 31, the second MSB of the version
>> > +                 * tag reflects whether memfd is supported on the other PA end. */
>> > +                if (c->version >= 31)
>>
>> You should be using (c->version & 0x40000000U) for the comparison to
>> get the actual protocol version.
>>
>> Not a comment for this patch, but we should probably introduce some
>> macros to deal with this flag setting to keep things cleaner.
>>
>
> Hmm.. if there are any flags in the most-significant bytes, then
> this check shall pass and we can succesfully check for memfd
> availability by checking the second MSB. That's why current patch
> works as expected (that is, correctly deduce memfd availability
> or non-availability) with both < 31 and > 31 PA endpoints. That's
> the case too because the two most-significant bytes are force
> cleared afterwards.
>
> Nonetheless, of course what you're proposing is the more correct
> logic. So if this is the only problem, let's merge and I'll create
> a follow-up patch this week that introduces the desired macros and
> fixes up this version-check issue in the process.

Not to worry, I actually ended up fixing it locally. Still testing
things, but I found an interesting breakage that I can't yet explain.
I merged your patches and built the code, and the did something like:

  $> PULSE_LOG=4 LD_LIBRARY_PATH=$PWD/build/src/.libs gst-launch-1.0
playbin uri=file:///path/to/some/file

which works fine (and I see logs about memfd being selected).

But,

  $> PULSE_LOG=4 LD_LIBRARY_PATH=$PWD/build/src/.libs rhythmbox (or totem)

seems to fail with the following error:

  Format info property 'format.sample_format' type is not string.

Which is weird because the two lines should be doing the same thing.
I'll try to pin this down later tonight or tomorrow. Hopefully just a
glitch in my setup.

Cheers,
Arun