[alsa-devel] [PATCH] ALSA: usb: improve delay reporting

[Georg Chini]

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.