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

[pl bossart]

> In your design the PA_SINK_PASSTHROUGH flag means that the sink can be
> used both for exclusive passthrough streams and for normal streams. Are
> you sure we can keep that promise for all future passthrough sinks too?
> It would be cleaner to have strict separation between passthrough sinks
> and non-passthrough sinks. I admit that your approach makes it much
> easier for the user to switch between passthrough and non-passthrough
> content, but that effect could also be achieved with a separate module
> that changes the card profile whenever a passthrough stream is being
> created.

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.

>> +[Element IEC958]
>> +switch = mute
>
> What if this element doesn't exist? Or is it guaranteed to exist? If it
> doesn't exist, then PA will use software muting, i.e. feeding zeroes to
> the device. I don't think that's good. A stream of zeroes is not a valid
> iec958 stream, AFAIK. I think we need a NOMUTE flag for sinks and
> streams. Or is alsa capable of switching between passthrough and PCM
> modes on the fly just by inspecting the data?

The IEC958 switch exists on most ALSA cards, I didn't really look into
cases where this would not be supported.
Sending zeroes is fine I guess, the receiver will understand the
samples are not valid and go to a mute mode as well.
There's currently a limitation that this control is locked, but
Takashi agrees this could be removed and it'll be fixed.

>> @@ -163,6 +163,7 @@ struct pa_alsa_path {
>>      pa_bool_t has_mute:1;
>>      pa_bool_t has_volume:1;
>>      pa_bool_t has_dB:1;
>> +    pa_bool_t is_passthrough:1;
>>
>>      long min_volume, max_volume;
>>      double min_dB, max_dB;
>
> is_passthrough doesn't seem to be set anywhere.

Right. I initially wanted to add a statement in the profile
configuration, but I ended-up matching the path name to instead.
Ideally we should extend Lennart's description rather than match
hard-coded names, but this is beyond my programming abilities.

> 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.
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?
As you have probably read, the addition of this flag breaks the
protocol, and I am really not religious on names/types. I will go with
the majority. Lennart, Colin, your guidance would be more than welcome
here. This is why I posted the patches early to gather feedback.

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

> 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?
Thanks for all your comments
Pierre