[pulseaudio-discuss] {PATCH][RFC] AC3 passthrough support

[Tanu Kaskinen]

On Thu, 2010-07-08 at 10:06 -0500, pl bossart wrote:
> First this passthrough mode is only valid for S/PDIF and HDMI outputs.
> In both cases, the receiver may or may not support encoded content,
> and in the SPDIF case you have no way of querying what the receiver
> capabilities are. You absolutely do not want to send passthrough
> streams without making sure the configuration makes sense.
> So somehow user interaction is required to explicitly tell PulseAudio
> the receiver supports AC3/DTS passthrough. To be more self-explanatory
> we should have a check-box in pavucontrol than a different profile.
> I really did not want to switch configurations depending on the type
> of content, ALSA/iec958 accepts both.

I agree that it's not really a good solution to require the user to
change the card profile manually to enable passthrough via spdif. But as
I said, a separate module could take care switching between the profiles
as needed.

Having thought this for a while, the only thing that really irritates me
in your design is that the semantics of the PA_SINK_PASSTHROUGH become
messy: the flag says that "in addition to normal PCM streams, this sink
supports also passthrough streams, except at times when there already
exist PCM streams, and btw, while a passthrough stream is playing, PCM
streams cease to be supported". I guess that's not a very practical
problem, though.

Ah, but now a better argument popped into my mind. If a sink can enter
the passthrough mode on the fly, then that has a consequence that
affects any application that is interested in sink volumes: whether a
sink has adjustable volume can change any time. That's not very nice.

> > Previously I claimed that since the the stream is compressed, we
> > inevitably will have to extend the code in pa_bytes_to_usec(), but I
> > realized now that as long as we pretend that the stream really is a
> > regular stereo stream, all calculations end up correct. Now the only
> > problem is that the user is given sort of wrong information, but I don't
> > think that's a big problem, so I agree with your approach of leaving the
> > sample spec alone and relying only on the sink and sink input flags. The
> > code can be improved later, if needed.
> 
> 
> > About flag names: I would not use names like PA_STREAM_NONAUDIO_IEC958.
> > First, "non-audio" is not really correct, because the data is still
> > audio, but the main issue is the "iec958" part. There can potentially be
> > many formats that need to be passed through unchanged, so in the future
> > there could be need for PA_STREAM_NONAUDIO_FOO and
> > PA_STREAM_NONAUDIO_BAR. At that point flags aren't the best place to put
> > the format information - the format should have its own variable. Until
> > new formats are introduced, you can still rely on a single flag, though.
> > I propose that you just rename the flags:
> >
> >  - PA_STREAM_NONAUDIO_IEC958 to PA_STREAM_PASSTHROUGH,
> >  - PA_SINK_INPUT_NONAUDIO_IEC958 to PA_SINK_INPUT_PASSTHROUGH and
> >  - PA_SINK_PASSTHROUGH_IEC958 to PA_SINK_PASSTHROUGH.
> 
> I really don't think there are that many formats. The only use cases I
> can think of are AC3/DTS a la iec958, or MP3 for BT headsets/low-power
> audio. Adding flags on the sinks is no big deal.

Even if we have only two formats, iec958 and mp3, I don't see why the
sink flags would the proper place to specify the format. I believe most
of the passthrough related code is only interested in the fact that the
stream data shall not be touched. If there are multiple flags for all
the different passthrough formats, then all that code will have to check
for two flags instead of one.

> The real problem is at the pa_stream level. We don't want to have to
> extend the protocol several times. I added a flag but if we have more
> than one format we will need more room for subtypes. We should
> probably extend the buff_attr attributes for this?

I haven't though about this much, but I have a feeling that more complex
formats (like mp3) are going to need more metadata than currently
available (stuff like bitrate, CBR vs VBR etc.), and if support for
other codecs is going to be needed in the future, the set of metadata
items might vary between codecs. I wouldn't care about all that complex
stuff before it's implemented - in other words, I don't see it as a big
problem if the protocol will have to be extended multiple times for
different codecs. (Of course it would be nice to avoid that, but I
wouldn't trust at least myself to get it right at first try without
actually implementing the mp3 passthrough functionality at the same
time.)

> > I think the most natural place for input compatibility checking is in
> > the sink object. At least it achieves better encapsulation, because then
> > you don't need to poke the sink's "private-ish" data from outside the
> > sink object. That makes future changes easier to do.
> 
> I disagree. I think you want a common logic in the core to make sure
> that things work the same way no matter what configuration you use.
> And its make sure we don't have to modify all existing sinks to
> perform the same checks.

There would of course be a standard implementation for sinks that don't
do any "strange" stuff. I thought individual sink implementations could
choose to amend the standard logic, but actually I'm not sure how it
could be done without duplicating code.

Giving it a second thought, the "can this sink input be connected to
this sink" question can be thought equally well as a method of sinks or
sink inputs, or even as a plain old independent function. So, doing it
like you do it in your patch is fine by me (just don't use "iec958"
where the logic isn't really bound to the exact format).

> > The NOVOLUME flags can be exported to clients, so they can choose to not
> > show any volume controls. Legacy clients should get the
> > PA_ERR_NOTSUPPORTED error when trying to change the volume of streams or
> > sinks that have the NOVOLUME flag.
> 
> Yes the volume needs more work. I don't really understand all this
> code through, so for now I disabled the set-volume() routines without
> notifying the client. You end-up in the situation where pavucontrol
> can change the volume but nothing happens, this is not user-friendly.
> Is this NOVOLUME flag an existing one, or something that would need to be added?

It needs to be added.

-- 
Tanu Kaskinen