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

[Alexander E. Patrakov]

22.04.2014 20:20, David Henningsson wrote:
> On 2014-04-22 10:18, Alexander E. Patrakov wrote:
>> 22.04.2014 13:01, David Henningsson wrote:
>>>
>>> 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.

For ALSA outputs, I don't know such scenarios (except the iec958 plugin, 
where the rewind support looks fixable). However, this looks like a 
valid point, if we don't limit the discussion to ALSA outputs.

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

IMHO it would be a good idea to have some generic, non-ALSA-specific, 
code that limits the sink latency to medium-range values (~150-200 ms) 
if it is known (preferably in advance) that it cannot rewind.

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

Gentoo prints a warning if one builds PulseAudio with snd-hda-intel 
buffer size on the running kernel less than 2048 KB. My kernel does 
trigger a warning, but I have specifically increased the buffer size 
from the default due to another copy of this recommendation (cannot 
remember where it came from). As for the 6 ms vs 100 ms discrepancy, 
this may need further investigation, but it is a different discussion.

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

OK, now I see your viewpoint in a larger context. Thanks!

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

Well, I would still much prefer if this information was provided 
statically by ALSA, because this would simplify the code in PulseAudio 
(by removing the logic related to the probing period). I am going to 
write the summary of our discussion to alsa-devel and ask for their 
opinion. Independently, I will try to fix or disable the rewind support 
in ALSA plugins where reasonable (i.e. I will not touch the rate plugin).

-- 
Alexander E. Patrakov