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

[Georg Chini]

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.

>
> The frame counter is already an approximation of the actual delay 
> which doesn't really account for the type of USB interface (Isoc, 
> Async) used on the device side. You'd be adding a lot of precision 
> without really solving the fact that you don't know what the actual 
> playback rate is. You'd really need a time smoother that models 
> playback rate v. system time if you want to be fully accurate below 
> the ms level for all types of USB audio devices.

The goal is not to be fully accurate, only to be somewhat better than 
the current estimate.
I'll copy here what I already said to Takashi in private communication:

The mismatch between sound card time and system time should not matter on the
timescale we are talking about. Let's say we are using 10 USB frames (which is
more than I have seen while debugging), so hw_ptr is updated every 10 ms. Assume
that the deviation between sound card and system time is 1% (which again should
be more than you will ever see). Then using system time, the computed delay can
be off by 100 microseconds at maximum, while the delay computed using USB frames
can be off by 2ms. Therefore using system time should be acceptable in that
situation because it delivers higher precision results as can be achieved using
USB frames.

For the rare cases where system time and sound card time are completely 
different,
the patch falls back to the original method.

>
> In addition I've only seen drivers report the delay based on hardware 
> counters. If we start doing interpolations based on system time, then 
> why not do it for other devices as well with a fix at the core level

I could not do that, I don't have enough ALSA knowledge. A motivation to 
improve
the delay reports for USB devices only could be that they are probably 
the most
common devices apart from HDA.

>
> And last, I think you have an issue for the pause case and you cannot 
> remove the code that limits the frame counter to 8 bits, this will not 
> work across multiple versions of [e|x]HCI controllers, see below.
>
>>
>> As a downside, the possible worst case error increases from 2ms to 3ms.
>> ---
>>   sound/usb/card.h |  1 +
>>   sound/usb/pcm.c  | 47 +++++++++++++++++++++++++++++++----------------
>>   2 files changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/sound/usb/card.h b/sound/usb/card.h
>> index ed87cc83eb47..8835d9628cac 100644
>> --- a/sound/usb/card.h
>> +++ b/sound/usb/card.h
>> @@ -149,6 +149,7 @@ struct snd_usb_substream {
>>         int last_frame_number;          /* stored frame number */
>>       int last_delay;                 /* stored delay */
>> +    struct timespec last_update_time;    /* time when last_delay was 
>> updated */
>>         struct {
>>           int marker;
>> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
>> index 3cbfae6604f9..6d40cb0b8777 100644
>> --- a/sound/usb/pcm.c
>> +++ b/sound/usb/pcm.c
>> @@ -46,9 +46,8 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct 
>> snd_usb_substream *subs,
>>       int current_frame_number;
>>       int frame_diff;
>>       int est_delay;
>> -
>> -    if (!subs->last_delay)
>> -        return 0; /* short path */
>
> NAK. This was added on purpose by 48779a0b8ffc ('ALSA: usb-audio: fix 
> delay account during pause')
> And since you kept the code that sets last_delay there is no reason to 
> take this out.

OK, I did not know that this is needed. I took it out because otherwise 
the delay
reported for capture will always be 0. retire_capture_urb() sets 
last_delay to 0.
Does the problem that was fixed in the commit  only apply to playback 
streams?
Then I could change the if condition accordingly.


>> +    int time_diff;
>> +    struct timespec now;
>>         current_frame_number = usb_get_current_frame_number(subs->dev);
>>       /*
>> @@ -58,9 +57,21 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct 
>> snd_usb_substream *subs,
>>        */
>>       frame_diff = (current_frame_number - subs->last_frame_number) & 
>> 0xff;
>>   -    /* Approximation based on number of samples per USB frame (ms),
>> -       some truncation for 44.1 but the estimate is good enough */
>> -    est_delay =  frame_diff * rate / 1000;
>> +    /* Estimate time since last update of last_delay */
>> +    snd_pcm_gettime(subs->pcm_substream->runtime, &now);
>> +    time_diff = (now.tv_sec - subs->last_update_time.tv_sec) * 
>> 1000000000;
>> +    time_diff += (int)now.tv_nsec - subs->last_update_time.tv_nsec;
>> +    time_diff /= 1000;
>> +
>> +    if (abs(time_diff - frame_diff * 1000) < 1000) {
> that's a 1ms error, this is quite high IMO to assess a divergence 
> between system and frame deltas

Because time_diff is used to interpolate between frames and frames are 
updated every millisecond,
it is perfectly normal if time_diff is for example 900 microseconds 
larger than frame_diff * 1000.
Only when the difference is larger than 1 ms we can conclude that 
something is wrong.
That is the reason why the patch increases the worst case error from 2 
ms to 3 ms.

>
>> +        /* System time and sound card time do not differ
>> +         * too much. Use system time to calculate delay */
>> +        est_delay =  time_diff * rate / 1000000;
>> +    } else {
>> +        /* Approximation based on number of samples per USB frame (ms),
>> +           some truncation for 44.1 but the estimate is good enough */
>> +        est_delay =  frame_diff * rate / 1000;
>> +    }
>>       if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
>>           est_delay = subs->last_delay - est_delay;
>>       else
>> @@ -853,6 +864,8 @@ static int snd_usb_pcm_prepare(struct 
>> snd_pcm_substream *substream)
>>       subs->hwptr_done = 0;
>>       subs->transfer_done = 0;
>>       subs->last_delay = 0;
>> +    subs->last_update_time.tv_sec = 0;
>> +    subs->last_update_time.tv_nsec = 0;
>>       subs->last_frame_number = 0;
>>       runtime->delay = 0;
>>   @@ -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.

If the high order bits are not arbitrary, 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.

>
>> +
>> +        /* Take time stamp for delay accounting */
>> +        snd_pcm_gettime(runtime, &subs->last_update_time);
>>             spin_unlock_irqrestore(&subs->lock, flags);
>>           /* copy a data chunk */
>> @@ -1550,7 +1565,9 @@ static void prepare_playback_urb(struct 
>> snd_usb_substream *subs,
>>         /* realign last_frame_number */
>>       subs->last_frame_number = usb_get_current_frame_number(subs->dev);
>> -    subs->last_frame_number &= 0xFF; /* keep 8 LSBs */
> same here, can't do this
>
>> +
>> +    /* Take time stamp for delay accounting */
>> +    snd_pcm_gettime(runtime, &subs->last_update_time);
>>         if (subs->trigger_tstamp_pending_update) {
>>           /* this is the first actual URB submitted,
>> @@ -1597,6 +1614,12 @@ static void retire_playback_urb(struct 
>> snd_usb_substream *subs,
>>           subs->last_delay -= processed;
>>       runtime->delay = subs->last_delay;
>>   +    /* realign last_frame_number */
>> +    subs->last_frame_number = usb_get_current_frame_number(subs->dev);
>
> need to filter on 8 bits..
>> +
>> +    /* Take time stamp for delay accounting */
>> +    snd_pcm_gettime(runtime, &subs->last_update_time);
>> +
>>       /*
>>        * Report when delay estimate is off by more than 2ms.
>>        * The error should be lower than 2ms since the estimate relies
>> @@ -1607,14 +1630,6 @@ static void retire_playback_urb(struct 
>> snd_usb_substream *subs,
>>               "delay: estimated %d, actual %d\n",
>>               est_delay, subs->last_delay);
>>   -    if (!subs->running) {
>> -        /* update last_frame_number for delay counting here since
>> -         * prepare_playback_urb won't be called during pause
>> -         */
>> -        subs->last_frame_number =
>> -            usb_get_current_frame_number(subs->dev) & 0xff;
>> -    }
>> -
> and why is this removed.

This is removed because the last_frame_number is now always updated,
so it is no longer necessary.

>>    out:
>>       spin_unlock_irqrestore(&subs->lock, flags);
>>   }