On Wed, 2010-07-07 at 19:37 -0500, pl bossart wrote:
> here's a first work-in-progress set of patches. I managed to route
> iec958 formatted data through PulseAudio, works fine but there's still
> a lot to do.
Sounds very good! Thanks for your work!
> There's some logic to prevent silly connections but
> still allow for the same use cases as before:
> you can still mix and play PCM streams on a passthrough output,
> however you cannot connect nonaudio data to a
> regular sink, and likewise if a sink has already a connection you can
> only connect if both the existing inputs and the new one are PCM.
> Volume control is disabled only when a nonaudio stream is connected.
So, do you think that if the card profile is set to Digital Passthrough
(IEC958), the sink should still accept normal PCM streams? I don't think
that's a good idea. You apparently do, and I'm not sure I'm able to
convince you otherwise, but I'll try.
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.
> I still have to do some work on sample-rate and channel map
> adaptation, but comments are welcome at this point.
Ok, I'll comment.
> +[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?
> @@ -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.
Other comments:
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.
You can put in the client documentation that if the
PA_STREAM_PASSTHROUGH flag is specified, the stream is currently assumed
to be iec958 formatted, and the sample spec must be stereo s16le with
sample rate of 32000, 44100 or 48000 Hz. In the future there may be more
options.
I still hold on to a couple of my previous suggestions: create
pa_sink_accept_input() and use NOVOLUME flags in sinks and sink inputs.
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.
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.
--
Tanu Kaskinen
