[pulseaudio-discuss] [PATCH] Don't use tsched on unsafe ALSA plugins

David Henningsson david.henningsson at canonical.com
Tue Apr 22 07:20:31 PDT 2014

On 2014-04-22 10:18, Alexander E. Patrakov wrote:
> 22.04.2014 13:01, David Henningsson wrote:
>> On 2014-04-21 09:04, Alexander E. Patrakov wrote:
>>> 21.04.2014 07:49, David Henningsson wrote:
>>>> On 2014-04-20 21:26, Alexander E. Patrakov wrote:
>>>>> Thus, it is not possible to tell the hardware device (that can use
>>>>> rewinds) from a properly wrapped software encoder (that can't rewind
>>>>> and
>>>>> doesn't pretend to be able to rewind), because for both cases
>>>>> snd_pcm_rewindable() would return 0 at the moment PulseAudio needs to
>>>>> make a decision.
>>>> The moment PulseAudio needs to make a decision is when a rewind is
>>>> requested.
>>> No. The decision definitely needs to be at the device-open time.
>>> Otherwise this will happen:
>>> """
>>> Now I need to rewind in order to accommodate a new low-latency client.
>>> Oops, I can't, and I have so much wrong data in my hardware buffer! I
>>> should not have created such a big buffer, but now it too late to change
>>> anything.
>>> """
>> So you want to always go low-latency (and high CPU/power consumption),
>> in case rewind is not possible?
> Yes, exactly. And high CPU/power consumption will happen anyway, because
> non-rewindable things like the DTS encoder are generally very CPU-hungry
> by themselves. So the extra 1% lost due to always enabling the lowest
> possible latency does not matter.

There might be other scenarios where rewinds are disabled, where you 
cannot make the assumption.

> And BTW I was going to submit more patches in this "can't truly rewind
> => always select low latency" direction, namely for the ladspa sink,
> possibly hidden behind a module parameter.
>> This sounds like a trade-off.
>> The other possibility would be to just wait until the hw buffer is empty
>> and then continue with low latency. If "accomodate a new low latency
>> client" happens rarely, and the maximum buffer size is < 2 seconds, then
>> maybe this is not much of an issue, compared to the drawback of forcing
>> low latency when it's not needed.
> This is not only "accomodate a new low-latency client". This also
> applies to volume changes in the case when the only volume control is a
> software one (and for the DTS encoder this is true) - you really don't
> want them to apply after two seconds. This is also "new client started",
> because you want it to be heard as soon as possible, and not after two
> seconds.
> So it is IMHO a reasonable trade-off.

Fair point. But maybe a medium latency (150 - 200 ms) would be a more 
reasonable trade-off in this case, than low latency (20 - 50 ms).

>>> And indeed, the current code already has logic to choose different
>>> buffer sizes for tstamp and irq-driven modes:
>>> http://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/src/modules/alsa/alsa-util.c#n298
>>> On my hardware, the buffer sizes for these two modes differ by a factor
>>> of 1000.
>> What hardware is that? If we don't do that already, we should cap the
>> tsched buffer size to ~ 2 seconds, or we'll just use more memory than we
>> need. Divide that with 1000 and you claim to have a irq-driven buffer
>> size of 2 ms. Which is way too low.
> Haswell HDMI, the IRQ-driven buffer size is 6 ms (which indeed doesn't
> make any sense for HDMI but doesn't lead to dropouts), the tsched-driven
> buffer size is ~10s. Or maybe it is just PulseAudio claiming 10s and
> then clamping to 2s and not announcing it to the log. I will check again
> later today.

The upstream standard for HDA Intel (which includes Haswell HDMI) is 
64K, which translates to 371 ms (in 44100Hz, stereo).
And PulseAudio's standard for IRQ-driven scheduling is 4 periods * 25 ms 
= 100 ms.

Some distros tweak these numbers, but your values look a bit too weird 
to believe, unless something is buggy.

>>> So what I want is really not related to tsched. "Don't choose a big
>>> buffer size and high latency, and don't try to rewind, if we know in
>>> advance that ALSA cannot rewind or only pretends to be able to rewind"
>>> would be a better description of my patch.
>>>> Whether or not to enable tsched should not matter in this case, unless
>>>> I'm missing something. (And this is probably what Raymond is trying to
>>>> say too.)
>>>> Or, put in another way, why would it be better for the ALSA device
>>>> to be
>>>> in interrupt driven mode just because it can't rewind?
>>> I have two slightly-conflicting answers to this.
>>> First answer:
>>> Rewinds and timestamp-driven scheduling are only the means to get
>>> dynamically reconfigurable latency, which is useful for less dropouts
>>> when there are no low-latency clients, lower power usage, and possibly
>>> other good things. Due to the inability to do rewinds, the "dynamic
>>> client-driven latency" goal becomes unachievable, so there is simply no
>>> good point to use timestamp-based scheduling in this case.
>>> Of course timestamp-based scheduling will work without rewinds, but, as
>>> PulseAudio would then need (due to inability to do rewinds) to lock into
>>> the constant minimum latency, the wakeup points will be evenly spaced in
>>> time. And that's almost equivalent to the IRQ-based scheduling (with a
>>> small exception listed in the second answer).
>>> Or to put it another way. Currently, PulseAudio supports two models:
>>> "big buffer + timestamp-based scheduling + rewinds" and "small buffer +
>>> IRQ-driven scheduling + no rewinds". Intermediate models such as "small
>>> buffer + timestamp-based scheduling + no rewinds" are possible, but they
>>> would IMHO only unnecessarily inflate the test matrix.
>> Eh? Rewinds are not disabled under IRQ-driven scheduling.
> They are disabled if pa_alsa_pcm_is_hw() returns false, which is exactly
> what I am trying to achieve:
> http://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/src/modules/alsa/alsa-sink.c#n1010
> http://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/src/modules/alsa/alsa-sink.c#n2346
> And this function returning false is also one of the reasons why tsched
> can be automatically disabled:
> http://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/src/modules/alsa/alsa-util.c#n245
> So indeed, it is possible to have a "tsched disabled but rewinds
> enabled" situation (e.g. due to the BATCH flag), but the code that I
> touch disables both. Sorry again for the bad patch subject.

Ah, okay.
I wonder if the BATCH stuff is correct though. But that's subject for 
another discussion.

> In fact, if you look through the git history behind the uses of
> pa_alsa_pcm_is_hw() function, you'll notice that one of them was added
> because of the AC3 encoder which has the same limitation: no rewinding.
> In particular, "git show cb55b00ccd25d965b1222e74375aee05427a449b" shows
> the commit that I am attempting to fix up for the DTS case, because the
> existing check in pa_alsa_pcm_is_hw() does not catch it. If you are
> still objecting to my commit, and think that the same objections don't
> apply to cb55b00ccd25d965b1222e74375aee05427a449b, then I would like to
> see why.
> See also d5f43bd4c6a7eecff7bc0c4ff1be9152b33cb1e0 and
> e3f15104cf0386a0e0a782037e8c0323629be749.

Well, I think that some existing code should be replaced with code that 
uses snd_pcm_rewindable, once we have confirmed that snd_pcm_rewindable 
is actually working in the ALSA layer. The code that needs updating 
probably includes the code added by commit cb55b00ccd25d ("alsa: disable 
rewinds when using ALSA plugins").

>>> Second answer:
>>> Well, it is not better. In timestamp-based scheduling mode, we can
>>> dynamically adjust latency. The limitation is that, without rewinds, our
>>> decisions to reduce latency (e.g. due to a new client) would apply too
>>> late. But even with this limitation, it means that we can try to keep as
>>> low latency as it actually works on the given hardware (similar to the
>>> current watermark logic), disregarding any client-specified latency.
>>> The problem is that, if one wants to use timestamp-based scheduling
>>> without rewinds, one needs to decouple the current watermark logic, the
>>> buffer size choice logic, and the "don't use latency lower than
>>> requested by any client" logic, because the later only makes sense when
>>> rewinds are possible.
>> I think we can still dynamically switch latency even without rewinds.
>> It'll just take slightly longer to start new streams.
>> Anyway, here's another idea:
>> During PulseAudio's first five seconds, all streams are running as some
>> kind of "startup test". How about we use that to probe the rewind too?
>> And if snd_pcm_rewindable says we can't rewind even with a full buffer,
>> then we choose a medium latency as our highest latency, e g 150 - 200
>> ms, instead of the 2 seconds?
> I will think a bit more about this. In fact, we could use a special
> probe stream at startup for this, write the full buffer and immediately
> attempt to rewind it, without wasting much time.
> Alternatively, we can use snd_pcm_forwardable() with an empty buffer
> (and then rewinding to undo the effect), wasting no time at all, because
> forwards and rewinds are related. Do you also consider this a valid
> solution?

I'm not sure what just moving the pointer forward without writing 
anything actually means, but it seems like the ALSA devices allowing 
forwarding and those allowing rewinding could be different.

David Henningsson, Canonical Ltd.

More information about the pulseaudio-discuss mailing list