ALSA: usb: improve delay reporting
PATCH 1/1 ALSA: usb: improve delay reporting
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.
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(-)
sound/usb/card.h +1 -0
@@ -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;
sound/usb/pcm.c +31 -16
89 lines not on this page · raw message
Signature · 1 line
2.16.2
Quoted 4 lines · Georg Chini
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.
Quoted 8 lines
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.
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.
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
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.
Quoted 24 lines
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(-)sound/usb/card.h +1
-0@@ -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;sound/usb/pcm.c +0
-0
sound/usb/pcm.c snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs, · line 51
Quoted 6 lines
sound/usb/pcm.c +0
-3@@ -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.
Quoted 9 lines
sound/usb/pcm.c +2
-0+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 ;
sound/usb/pcm.c snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs, · line 66
Quoted 10 lines
sound/usb/pcm.c +7
-3-/* 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
Quoted 21 lines
sound/usb/pcm.c +10
-0+/* 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 ;
sound/usb/pcm.c static void retire_capture_urb(struct snd_usb_substream *subs, · line 1337
Quoted 4 lines
sound/usb/pcm.c +0
-1@@ -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.
sound/usb/pcm.c static void prepare_playback_urb(struct snd_usb_substream *subs, · line 1553
Quoted 11 lines
sound/usb/pcm.c +3
-1+ +/* 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
sound/usb/pcm.c static void retire_playback_urb(struct snd_usb_substream *subs, · line 1618
Quoted 12 lines
sound/usb/pcm.c +5
-0+ +/* 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..
Quoted 9 lines
sound/usb/pcm.c +4
-0+ +/* 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",
sound/usb/pcm.c static void retire_playback_urb(struct snd_usb_substream *subs, · line 1617
Quoted 10 lines
sound/usb/pcm.c +0
-8est_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.
Quoted 3 lines
sound/usb/pcm.c +0
-0out:spin_unlock_irqrestore (&subs->lock, flags); }
Quoted 11 lines · Pierre-Louis Bossart
Quoted 9 lines · Georg Chini
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.
Quoted 4 lines
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 2 lines
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.
Quoted 28 lines
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.
Quoted 25 lines
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(-)sound/usb/card.h +0
-0@@ -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 whenlast_delaywas updated */
struct {
int marker;sound/usb/pcm.c +0
-0@@ -46,9 +46,8 @@ snd_pcm_uframes_t snd_usb_pcm_delay(structsnd_usb_substream *subs, int current_frame_number;
Quoted 8 lines
sound/usb/pcm.c snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs, · line 51
Quoted 5 lines
sound/usb/pcm.c +0
-3int 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 setslast_delaythere 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.
Quoted 12 lines
Quoted 12 lines
sound/usb/pcm.c +3
-2+ 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(structsnd_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 */
Quoted 8 lines
sound/usb/pcm.c snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs, · line 66
Quoted 7 lines
sound/usb/pcm.c +6
-0+ 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.
Quoted 19 lines
Quoted 19 lines
sound/usb/pcm.c +8
-0+ /* 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; elsesound/usb/pcm.c +2
-0@@ -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;
Quoted 8 lines
Quoted 6 lines
sound/usb/pcm.c +0
-0subs->last_frame_number = 0; runtime->delay = 0;@@ -1334,7 +1347,9 @@ static void retire_capture_urb(struct
snd_usb_substream*subs, /* realignlast_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.
Quoted 38 lines
sound/usb/pcm.c static void prepare_playback_urb(struct · line 1553
Quoted 11 lines
+
+ /* Take time stamp for delay accounting */ +snd_pcm_gettime(runtime, &subs->last_update_time);spin_unlock_irqrestore(&subs->lock, flags); /* copy a data chunk */sound/usb/pcm.c +0
-1@@ -1550,7 +1565,9 @@ static void prepare_playback_urb(structsnd_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
Quoted 11 lines
sound/usb/pcm.c +3
-0+ + /* 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(structsnd_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..
Quoted 12 lines
sound/usb/pcm.c +4
-0+ + /* 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 reliessound/usb/pcm.c +0
-0@@ -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) {
Quoted 8 lines
sound/usb/pcm.c static void retire_playback_urb(struct snd_usb_substream *subs, · line 1617
Quoted 7 lines
sound/usb/pcm.c +0
-7- /* 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.
Quoted 3 lines
Quoted 3 lines
out:
spin_unlock_irqrestore(&subs->lock, flags);
}
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 12 lines · Georg Chini
Quoted 11 lines · Pierre-Louis Bossart
Quoted 9 lines · Georg Chini
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.
Quoted 8 lines
Quoted 4 lines
Quoted 3 lines
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.
Quoted 5 lines
Quoted 3 lines
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.
Define 'better'. What is the goal and the expected benefit for applications?
Quoted 16 lines
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_ptris 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.
Quoted 1 line
Quoted 5 lines
Quoted 1 line
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.
Exactly, why not do this for HDAudio then...
Quoted 38 lines
Quoted 29 lines
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.
Quoted 25 lines
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(-)sound/usb/card.h +0
-0@@ -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 whenlast_delaywas updated */
struct {
int marker;sound/usb/pcm.c +0
-0@@ -46,9 +46,8 @@ snd_pcm_uframes_t snd_usb_pcm_delay(structsnd_usb_substream *subs, int current_frame_number;Quoted 8 lines
sound/usb/pcm.c snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs, · line 51
Quoted 5 lines
sound/usb/pcm.c +0
-3int 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 setslast_delaythere is no reason to take this out.
Quoted 4 lines
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()setslast_delayto 0. Does the problem that was fixed in the commit only apply to playback streams? Then I could change the if condition accordingly.
That's precisely the issue: the code was contributed by multiple people to work on multiple platforms. What parts do what exactly is complicated to reverse-engineer.
Quoted 23 lines
Quoted 14 lines
Quoted 14 lines
sound/usb/pcm.c +3
-2+ 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(structsnd_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 */ Quoted 8 lines
sound/usb/pcm.c snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs, · line 66
Quoted 7 lines
sound/usb/pcm.c +6
-0+ 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
Quoted 4 lines
Because
time_diffis used to interpolate between frames and frames are updated every millisecond, it is perfectly normal iftime_diffis for example 900 microseconds larger thanframe_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.
Yeah, well, given that the actual playback rate is not aligned with the frame counter for async interfaces, this test is questionable.
Quoted 29 lines
Quoted 20 lines
Quoted 20 lines
sound/usb/pcm.c +8
-0+ /* 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; elsesound/usb/pcm.c +2
-0@@ -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;Quoted 8 lines
Quoted 6 lines
sound/usb/pcm.c +0
-0subs->last_frame_number = 0; runtime->delay = 0;@@ -1334,7 +1347,9 @@ static void retire_capture_urb(struct
snd_usb_substream*subs, /* realignlast_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.
Quoted 4 lines
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.
the number of valid bits varies in different controllers so the goal was to align on the common denominator - 8 bits only.
Quoted 48 lines
Quoted 39 lines
sound/usb/pcm.c static void prepare_playback_urb(struct · line 1553
Quoted 12 lines
+
+ /* Take time stamp for delay accounting */ +snd_pcm_gettime(runtime, &subs->last_update_time);spin_unlock_irqrestore(&subs->lock, flags); /* copy a data chunk */sound/usb/pcm.c +0
-1@@ -1550,7 +1565,9 @@ static void prepare_playback_urb(structsnd_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
Quoted 11 lines
sound/usb/pcm.c +3
-0+ + /* 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(structsnd_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..
Quoted 12 lines
sound/usb/pcm.c +4
-0+ + /* 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 reliessound/usb/pcm.c +0
-0@@ -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) {Quoted 8 lines
sound/usb/pcm.c static void retire_playback_urb(struct snd_usb_substream *subs, · line 1617
Quoted 7 lines
sound/usb/pcm.c +0
-7- /* 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.
Quoted 2 lines
This is removed because the
last_frame_numberis now always updated, so it is no longer necessary.
makes no sense to me.
Quoted 10 lines
Quoted 4 lines
Quoted 4 lines
out:
spin_unlock_irqrestore(&subs->lock, flags);
}_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Quoted 18 lines · Pierre-Louis Bossart
Quoted 17 lines · Georg Chini
Quoted 15 lines · Pierre-Louis Bossart
Quoted 4 lines · Georg Chini
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.
Quoted 8 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
@@ -1334,7 +1347,9 @@ static void retire_capture_urb(struct
snd_usb_substream*subs, /* realignlast_frame_number*/
subs->last_frame_number=current_frame_number;
-subs->last_frame_number&=0xFF; /* keep 8 LSBs */
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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 insnd_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.
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel