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

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.
https://launchpad.net/~diwic