[PATCH] speaker-test: Fix dropped samples at the end of test

[PATCH] speaker-test: Fix dropped samples at the end of test

[Jarkko Nikula]

Commit 6d1673526b0f ("Avoid unnecessary drain/restart in speaker-test")
drains only when buffer is bigger than audio sample. This has a drawback
that up to buffer size amount of data may not be heard at the end of audio
sample.

This was noted with "speaker-test -c 2 -t wav -s 2" test on a
hardware that has a buffer size of 24000 samples and 48 kHz sample rate.
Instead of playing "front right" it played something like "front ra".

Reverse buffer size vs sample size test wouldn't work either since then
samples smaller than buffer are dropped.

Fix this by removing buffer_size tests from write_loop() and do
drain/restart always when not aborting.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Reported-by: Vidal, Guillaume-florianX <guillaume-florianx.vidal@intel.com>
---
This was originally noted on Baytrail ADSP hw (default buffer size 24000)
but can be heard also on Intel HDA (default buffer size 8192) when audio
sample is small enough but bigger than buffer. For instance 100 ms sample
finishes too shortly (buffer size 8192, sample size 9600) but 50 ms plays
ok (buffer size 8192, sample size 4800).

I guess some optimization can be done for snd_pcm_prepare() when not looping
but that's not necessary for this fix.
---
 speaker-test/speaker-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/speaker-test/speaker-test.c b/speaker-test/speaker-test.c
index 61396f296c65..836fd26d35b1 100644
--- a/speaker-test/speaker-test.c
+++ b/speaker-test/speaker-test.c
@@ -942,7 +942,7 @@ static int write_loop(snd_pcm_t *handle, int channel, int periods, uint8_t *fram
 			      snd_pcm_bytes_to_frames(handle, err * channels))) < 0)
 	break;
     }
-    if (buffer_size > n && !in_aborting) {
+    if (!in_aborting) {
       snd_pcm_drain(handle);
       snd_pcm_prepare(handle);
     }
@@ -964,7 +964,7 @@ static int write_loop(snd_pcm_t *handle, int channel, int periods, uint8_t *fram
     if ((err = write_buffer(handle, frames, period_size)) < 0)
       return err;
   }
-  if (buffer_size > n * period_size && !in_aborting) {
+  if (!in_aborting) {
     snd_pcm_drain(handle);
     snd_pcm_prepare(handle);
   }
-- 
2.1.0

Re: [PATCH] speaker-test: Fix dropped samples at the end of test

[Takashi Iwai]

At Fri, 12 Sep 2014 15:14:28 +0300,
Jarkko Nikula wrote:
> 
> Commit 6d1673526b0f ("Avoid unnecessary drain/restart in speaker-test")
> drains only when buffer is bigger than audio sample. This has a drawback
> that up to buffer size amount of data may not be heard at the end of audio
> sample.
> 
> This was noted with "speaker-test -c 2 -t wav -s 2" test on a
> hardware that has a buffer size of 24000 samples and 48 kHz sample rate.
> Instead of playing "front right" it played something like "front ra".
> 
> Reverse buffer size vs sample size test wouldn't work either since then
> samples smaller than buffer are dropped.
> 
> Fix this by removing buffer_size tests from write_loop() and do
> drain/restart always when not aborting.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Reported-by: Vidal, Guillaume-florianX <guillaume-florianx.vidal@intel.com>
> ---
> This was originally noted on Baytrail ADSP hw (default buffer size 24000)
> but can be heard also on Intel HDA (default buffer size 8192) when audio
> sample is small enough but bigger than buffer. For instance 100 ms sample
> finishes too shortly (buffer size 8192, sample size 9600) but 50 ms plays
> ok (buffer size 8192, sample size 4800).
> 
> I guess some optimization can be done for snd_pcm_prepare() when not looping
> but that's not necessary for this fix.

Won't it suffice by just putting snd_pcm_drain() at the end of the
whole operation like below?  Doing snd_pcm_drain() and
snd_pcm_prepare() at each time causes often undesired pop noises or
such.

OTOH, doing drain there is good for showing the text at the right
time.  So, we'll likely want to have both options managed by a command
line option.


Takashi

---
diff --git a/speaker-test/speaker-test.c b/speaker-test/speaker-test.c
index 61396f296c65..362efa7ffc0d 100644
--- a/speaker-test/speaker-test.c
+++ b/speaker-test/speaker-test.c
@@ -1307,6 +1307,7 @@ int main(int argc, char *argv[]) {
     }
   }
 
+  snd_pcm_drain(handle);
 
   free(frames);
 #ifdef CONFIG_SUPPORT_CHMAP

Re: [PATCH] speaker-test: Fix dropped samples at the end of test

[Jie, Yang]

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org
> [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Takashi Iwai
> Sent: Monday, September 15, 2014 11:24 PM
> To: Jarkko Nikula
> Cc: alsa-devel@alsa-project.org; Vidal, Guillaume-florianX
> Subject: Re: [alsa-devel] [PATCH] speaker-test: Fix dropped samples at the end
> of test
> 
> At Fri, 12 Sep 2014 15:14:28 +0300,
> Jarkko Nikula wrote:
> >
> > Commit 6d1673526b0f ("Avoid unnecessary drain/restart in
> > speaker-test") drains only when buffer is bigger than audio sample.
> > This has a drawback that up to buffer size amount of data may not be
> > heard at the end of audio sample.
> >
> > This was noted with "speaker-test -c 2 -t wav -s 2" test on a hardware
> > that has a buffer size of 24000 samples and 48 kHz sample rate.
> > Instead of playing "front right" it played something like "front ra".
> >
> > Reverse buffer size vs sample size test wouldn't work either since
> > then samples smaller than buffer are dropped.
> >
> > Fix this by removing buffer_size tests from write_loop() and do
> > drain/restart always when not aborting.
> >
> > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Reported-by: Vidal, Guillaume-florianX
> > <guillaume-florianx.vidal@intel.com>
> > ---
> > This was originally noted on Baytrail ADSP hw (default buffer size
> > 24000) but can be heard also on Intel HDA (default buffer size 8192)
> > when audio sample is small enough but bigger than buffer. For instance
> > 100 ms sample finishes too shortly (buffer size 8192, sample size
> > 9600) but 50 ms plays ok (buffer size 8192, sample size 4800).
> >
> > I guess some optimization can be done for snd_pcm_prepare() when not
> > looping but that's not necessary for this fix.
> 
> Won't it suffice by just putting snd_pcm_drain() at the end of the whole
> operation like below?  Doing snd_pcm_drain() and
> snd_pcm_prepare() at each time causes often undesired pop noises or such.
> 
> OTOH, doing drain there is good for showing the text at the right time.  So,
> we'll likely want to have both options managed by a command line option.
> 
> 
> Takashi
> 
> ---
> diff --git a/speaker-test/speaker-test.c b/speaker-test/speaker-test.c index
> 61396f296c65..362efa7ffc0d 100644
> --- a/speaker-test/speaker-test.c
> +++ b/speaker-test/speaker-test.c
> @@ -1307,6 +1307,7 @@ int main(int argc, char *argv[]) {
>      }
>    }
> 
> +  snd_pcm_drain(handle);
[Keyon] here I verified Takashi's fix also works.
> 
>    free(frames);
>  #ifdef CONFIG_SUPPORT_CHMAP
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] speaker-test: Fix dropped samples at the end of test

[Jarkko Nikula]

On 09/15/2014 06:23 PM, Takashi Iwai wrote:
> At Fri, 12 Sep 2014 15:14:28 +0300,
> Jarkko Nikula wrote:
>> Commit 6d1673526b0f ("Avoid unnecessary drain/restart in speaker-test")
>> drains only when buffer is bigger than audio sample. This has a drawback
>> that up to buffer size amount of data may not be heard at the end of audio
>> sample.
>>
>> This was noted with "speaker-test -c 2 -t wav -s 2" test on a
>> hardware that has a buffer size of 24000 samples and 48 kHz sample rate.
>> Instead of playing "front right" it played something like "front ra".
>>
>> Reverse buffer size vs sample size test wouldn't work either since then
>> samples smaller than buffer are dropped.
>>
>> Fix this by removing buffer_size tests from write_loop() and do
>> drain/restart always when not aborting.
>>
>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> Reported-by: Vidal, Guillaume-florianX <guillaume-florianx.vidal@intel.com>
>> ---
>> This was originally noted on Baytrail ADSP hw (default buffer size 24000)
>> but can be heard also on Intel HDA (default buffer size 8192) when audio
>> sample is small enough but bigger than buffer. For instance 100 ms sample
>> finishes too shortly (buffer size 8192, sample size 9600) but 50 ms plays
>> ok (buffer size 8192, sample size 4800).
>>
>> I guess some optimization can be done for snd_pcm_prepare() when not looping
>> but that's not necessary for this fix.
> Won't it suffice by just putting snd_pcm_drain() at the end of the
> whole operation like below?  Doing snd_pcm_drain() and
> snd_pcm_prepare() at each time causes often undesired pop noises or
> such.
Yeah, that works too and pop avoidance is a good argument.
> OTOH, doing drain there is good for showing the text at the right
> time.  So, we'll likely want to have both options managed by a command
> line option.
>
I see slight difference between text print and sample starts time on 
Baytrail ADSP when doing loop test with your patch. Perhaps about half 
second since buffer size is 24000 and SR 48 kHZ but I don't think that 
matters much since it's hardly noticeable.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH] speaker-test: Fix dropped samples at the end of test

[Takashi Iwai]

At Tue, 16 Sep 2014 10:51:32 +0300,
Jarkko Nikula wrote:
> 
> On 09/15/2014 06:23 PM, Takashi Iwai wrote:
> > At Fri, 12 Sep 2014 15:14:28 +0300,
> > Jarkko Nikula wrote:
> >> Commit 6d1673526b0f ("Avoid unnecessary drain/restart in speaker-test")
> >> drains only when buffer is bigger than audio sample. This has a drawback
> >> that up to buffer size amount of data may not be heard at the end of audio
> >> sample.
> >>
> >> This was noted with "speaker-test -c 2 -t wav -s 2" test on a
> >> hardware that has a buffer size of 24000 samples and 48 kHz sample rate.
> >> Instead of playing "front right" it played something like "front ra".
> >>
> >> Reverse buffer size vs sample size test wouldn't work either since then
> >> samples smaller than buffer are dropped.
> >>
> >> Fix this by removing buffer_size tests from write_loop() and do
> >> drain/restart always when not aborting.
> >>
> >> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> >> Reported-by: Vidal, Guillaume-florianX <guillaume-florianx.vidal@intel.com>
> >> ---
> >> This was originally noted on Baytrail ADSP hw (default buffer size 24000)
> >> but can be heard also on Intel HDA (default buffer size 8192) when audio
> >> sample is small enough but bigger than buffer. For instance 100 ms sample
> >> finishes too shortly (buffer size 8192, sample size 9600) but 50 ms plays
> >> ok (buffer size 8192, sample size 4800).
> >>
> >> I guess some optimization can be done for snd_pcm_prepare() when not looping
> >> but that's not necessary for this fix.
> > Won't it suffice by just putting snd_pcm_drain() at the end of the
> > whole operation like below?  Doing snd_pcm_drain() and
> > snd_pcm_prepare() at each time causes often undesired pop noises or
> > such.
> Yeah, that works too and pop avoidance is a good argument.
> > OTOH, doing drain there is good for showing the text at the right
> > time.  So, we'll likely want to have both options managed by a command
> > line option.
> >
> I see slight difference between text print and sample starts time on 
> Baytrail ADSP when doing loop test with your patch. Perhaps about half 
> second since buffer size is 24000 and SR 48 kHZ but I don't think that 
> matters much since it's hardly noticeable.
> 
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

OK, I pushed the fix.  Thanks for checking.


Takashi