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
