[alsa-devel] [PATCH] ALSA: usb: improve delay reporting
Georg Chini
georg at chini.tk
Thu Apr 5 16:55:28 CEST 2018
On 05.04.2018 16:13, Pierre-Louis Bossart wrote:
> On 4/5/18 6:49 AM, Georg Chini wrote:
>> On 04.04.2018 21:24, Pierre-Louis Bossart wrote:
>>> On 04/04/2018 08:57 AM, Georg Chini wrote:
>>>
>>>> Currently, the kernel uses only the number of USB frames to
>>>> interpolate the delay
>>>> between sending or receiving URB's. This limits the granuarity of
>>>> the delay reports
>>>> to one millisecond because the frame counter is updated in a
>>>> millisecond interval.
>>>>
>>>> This patch additionally uses time stamps to achieve higher
>>>> precision of the reported
>>>> delay. If the time difference computed using time stamps does not
>>>> deviate more than
>>>> one millisecond from the difference computed using frames, it is
>>>> assumed that the
>>>> system time difference delivers the more precise delay estimate.
>>>>
>>>> This will significantly increase the precision of the reported
>>>> delay in the majority
>>>> of cases where sound card clock and system clock are almost in sync
>>>> while falling back
>>>> to the old method for the rare cases where the two times differ too
>>>> much.
>>> I am all for better delay estimates but I have mixed feelings about
>>> this - and some hardware-related parts are broken.
>>
>> Well, I only stumbled into this because of the recent delay
>> estimation issues in pulseaudio,
>> so forgive my ignorance about ALSA. If the general idea of the patch
>> is not right, just ignore
>> my other comments and drop the patch.
>
> It's complicated to change this part of the code. When I added the use
> of the frame counter some 5 years back, it created all sorts of issues
> on various platforms that I was not even aware of.
>
> Your implementation raises obvious questions and would need quite a
> bit of revalidation. In its current state it's not ready for prime time.
OK, so as I said, just forget about it.
>
>>>> @@ -1334,7 +1347,9 @@ static void retire_capture_urb(struct
>>>> snd_usb_substream *subs,
>>>> /* realign last_frame_number */
>>>> subs->last_frame_number = current_frame_number;
>>>> - subs->last_frame_number &= 0xFF; /* keep 8 LSBs */
>>>
>>> You can't remove this, this models the fact that only 8 bits are
>>> valid in the frame counter.
>>
>> This is not necessary because the value is
>> only used in snd_usb_pcm_delay() and (a - b) & 0xff = (a - b & 0xff)
>> & 0xff. You can
>> prove this by writing a and b as a = 256 * a1 + a2 and b = 256 * b1 +
>> b2 with a2
>> and b2 smaller than 256.
>
> the number of valid bits varies in different controllers so the goal
> was to align on the common denominator - 8 bits only.
The common denominator will still be 8 bits, using & 0xff once in
snd_usb_pcm_delay()
when the frame difference is calculated has exactly the same effect, so
doing it here
is unnecessary.
More information about the Alsa-devel
mailing list