[pulseaudio-discuss] review+pull-request: Passthrough support

[Arun Raghavan]

On Sun, 2011-04-24 at 20:52 +0100, Colin Guthrie wrote:
> 'Twas brillig, and Arun Raghavan at 13/04/11 13:53 did gyre and gimble:
> > Hey folks,
> > My passthrough work is now at a point where I think it's good to pull to
> > master. All the core code for clients to signal that they are providing
> > a compressed format, and sinks to signal what formats they support is
> > there. The API is essentially the same as discussed earlier on-list,
> > with some small tweaks along the way as necessary.
> > 
> > Still pending are proper detection/signalling of available formats on
> > ALSA devices is to be done (Colin's looking at some of the plumbing for
> > S/PDIF, and HDMI ELD/EDID-based lookups are something that we need as
> > well), and better handling of monitors (we suspend them for now, which
> > works), and volumes (Tanu's got some on-going work on this).
> > 
> > NOTE: given the above about ALSA, topmost patch should probably not be
> > pulled, but is handy to have for testing.
> > 
> > In addition to the passthrough branch, there is a passthrough-bt branch
> > which has changes to the bluetooth sink to support streaming MP3 to
> > headsets that support it. This also needs some work before being ready
> > to pull.
> > 
> > The changes needed to actually use this in GStreamer are also done, but
> > not upstream yet. I need to do some rebasing to make this stuff good to
> > push out, so details on this in a following mail.
> > 
> > Comments, feedback will be very much appreciated. :)
> 
> 
> > are available in the git repository at:
> >   git://git.collabora.co.uk/git/user/arun/pulseaudio.git passthrough
> 
> 
> Not tried the code (yet), but will do this week. I have reviewed all the
> code yet!
> 
> But I did review all the commits! Most of my comments are trivial tho'
> (and some are already fixed it seems!)

Thanks! :)

> In 4a839b3767ae5571c69bd591b5a59d7307cba13e
> 
> pa_format_info_to_sample_spec() and pa_format_info_to_sample_spec_fake()
> some pa_assert's should pa_assert_se's. (Update: I think these are all
> rectified in 38309769c5a630ee78ae73be6fb407624c1509fc)

Yup, that bit's been rewritten without a lot of asserts.

> In 7e8b65bc4f490628f20ab420a6c80bfa3a760bc6
> 
> Shouldn't pa_create_stream_callback() call
> pa_tagstruct_get_format_info() up to n_format times and only bail if all
> are invalid?

The format we have at this point is the negotiated format, so there is
only one.

> Minor typo: "Send back the format me negotiated" s/me/we/
> 
> Minor typo: "We're working not working with the extended API" s/working
> not/not/
> 
> pa_sink_input_new_data_set_formats(): Please drop the last "else" and
> just put the "return TRUE" as the last statement. It's trivial but I
> believe this looks neater to always have a return at the top level of
> the function.

Ack on all 3 counts.

> In d885a39b9f1085e759ad69afcc939caffb1fbc5a
> 
> Minor typo: "but this is can very well be incorrect" s/this is/this/

Also fixed.

> In e193c2bf55326a48e2297bcacadc9d1848a40d7d and
> 948d0f19bef353208ffb5b1b8c520b6b489b94a6
> 
> Can you make sure that pactl and pacmd stay as in-sync as possible?

I held off because I thought that pacmd was going to be dropped before
too long. Is this not the case? Sink port information seems to have not
been added, I'll sync that as well if required.

> In bb7cc499f1815de1c90b0ef1850152224df96ff9
> 
> I don't see why this asserts in the current form nor what has actually
> changed.

It should not assert since we want to gracefully fail (that is the
original code should not have been an assert).

> In 9d5e8867066e392fa40add0f0380374131dfc2de
> 
> Minor: pa_streq has a space before the (.

Check.

> In d9e0f3bc0286249ced30a4728e4467f7a46f37af
> 
> Already merged in 837e0a960630251ce30c124da5e65079b748d978
> 
> 
> 
> 
> Should we rebase before merge or is it wise to keep your tree as is for
> ease of working with existing checkouts?

I've done a rebase against current master and pushed to the same
location.

> General Question:
> 
> Has this broken tunnels? (we manage to do this quite often with stream
> protocol changes...

Indeed, it does. I've put fixing this on my TODO list. Will try to get
to it soon.

Cheers,
Arun