937057 - Web Audio buffer playback ends with a crack/snap noise.

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Jukka Jylänki]

1. Visit https://dl.dropboxusercontent.com/u/40949268/Bugs/short_beep.html
2. Press Start audio.

Result: When the audio clip finishes after 2 seconds, a crack/pop sound always occurs at the end of the clip. Tested on FF25.0 and Nightly 2013-11-10.

Expected: The page should play back a glitch-free 2 second sine wave.

Note: This is similar to the bug https://bugzilla.mozilla.org/show_bug.cgi?id=937054 . In that bug, some kind of audio timing causes cracks at random times in the middle of an audio playback, but in this one, the crack is audible always when the audio clip finishes.

Comment 1

[Maire Reavy [:mreavy]]

Hi Karl & Paul -- Can you look into this report?  Jukka also reported Bug 937054 and Bug 937061 -- which are related to this one.

Comment 2

[Karl Tomlinson (:karlt)]

The web audio implementation should play the buffer as provided.  It doesn't and shouldn't modify the samples to remove high frequency effects at the end of the buffer.

However, I think there is a bug here worth fixing.  When duration * frequency is integer, then the buffer should be ending at a zero-crossing, and so the click should be significantly less obvious.  In this testcase the click is only less obvious when the sample rate matches that of the AudioContext.

I think latency from the resampler is being exposed here because the signal is truncated early.

Comment 3

[(no longer active)]

(In reply to comment #2)
> The web audio implementation should play the buffer as provided.  It doesn't
> and shouldn't modify the samples to remove high frequency effects at the end of
> the buffer.
> 
> However, I think there is a bug here worth fixing.  When duration * frequency
> is integer, then the buffer should be ending at a zero-crossing, and so the
> click should be significantly less obvious.  In this testcase the click is only
> less obvious when the sample rate matches that of the AudioContext.
> 
> I think latency from the resampler is being exposed here because the signal is
> truncated early.

What do you mean?  We should output enough additional frames to account for the resampler latency, I'm pretty sure...

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: remove resampler latency from BufferSource playback

Applies on top of patches in bug 937475.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: remove resampler latency from BufferSource playback v2

Comment 6

[Karl Tomlinson (:karlt)]

This will be tested in the test I'll propose in bug 938450.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: provide a null ptr instead of a zero buffer when processing resampler tail

There is similar existing code where we can save some memory allocation and temporary buffers of zeros, but passing null for input.

Comment 8

[Paul Adenot (:padenot)]

Comment on attachment 8340924 [details] [diff] [review]
remove resampler latency from BufferSource playback v2

Review of attachment 8340924 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I have a couple questions.

::: content/media/webaudio/AudioBufferSourceNode.cpp
@@ +191,2 @@
>                                           uint32_t& aFramesWritten) {
> +    // TODO: adjust for mStop

Isn't this taken care of in `ProcessAudioBlock`? Maybe I'm missing something.

@@ +222,5 @@
> +          if (inSamples == aAvailableInInputBuffer && !mLoop) {
> +            // If the available output space were unbounded then the input
> +            // latency would always be the correct amount of extra input to
> +            // provide in order to advance the output position to the align
> +            // with the final point in the buffer.  However, when the output

english problem here (to the align...)

@@ +239,1 @@
>      }

nit: to make it obvious that the if(aAvailableInInputBuffer) and the rest are mutually exclusive paths, I'd rather have an else clause rather than a return, here.

@@ +250,2 @@
>        WebAudioUtils::SpeexResamplerProcess(resampler, i,
> +                                           (AudioDataValue*)nullptr, &inSamples,

nit: static_cast<> for consistency.

@@ +252,3 @@
>                                             outputData, &outSamples);
> +      if (++i == aChannels) {
> +        mRemainingResamplerTail -= inSamples;

Could we release the resampler and switch to taking the normal "copy" path when resampling is not needed anymore and we have consumed all the resampled data in the resampler?

@@ +446,5 @@
>    TrackTicks mStart;
>    TrackTicks mStop;
>    nsRefPtr<ThreadSharedFloatArrayBufferList> mBuffer;
>    SpeexResamplerState* mResampler;
> +  int mRemainingResamplerTail;

Add a comment saying that this is in the same unit as |mPosition| (i.e. input buffer samples).

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8340926 [details] [diff] [review]
provide a null ptr instead of a zero buffer when processing resampler tail

https://hg.mozilla.org/integration/mozilla-inbound/rev/c995ea111b8b

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: remove resampler latency from BufferSource playback v2.1

(In reply to Paul Adenot (:padenot) from comment #8)
> ::: content/media/webaudio/AudioBufferSourceNode.cpp
> @@ +191,2 @@
> >                                           uint32_t& aFramesWritten) {
> > +    // TODO: adjust for mStop
> 
> Isn't this taken care of in `ProcessAudioBlock`? Maybe I'm missing something.

The code in ProcessAudioBlock() is still relying on CopyFromBuffer() not to
advance streamPosition beyond mStop.  CopyFromBuffer() checks mStop if not
resampling.

> >                                             outputData, &outSamples);
> > +      if (++i == aChannels) {
> > +        mRemainingResamplerTail -= inSamples;
> 
> Could we release the resampler and switch to taking the normal "copy" path
> when resampling is not needed anymore and we have consumed all the resampled
> data in the resampler?

Yes, we could release the resampler when we know that we'll take the
FillWithZeroes() path, but this is not the right place to release the resampler
because it doesn't destroy when mStop is passed.  It would be better to clean
up when setting aFinished, if such cleanup is worthwhile.

Comment 11

[Paul Adenot (:padenot)]

(In reply to Karl Tomlinson (:karlt) from comment #10)
> Created attachment 8341591 [details] [diff] [review]
> remove resampler latency from BufferSource playback v2.1
> 
> (In reply to Paul Adenot (:padenot) from comment #8)
> > ::: content/media/webaudio/AudioBufferSourceNode.cpp
> > @@ +191,2 @@
> > >                                           uint32_t& aFramesWritten) {
> > > +    // TODO: adjust for mStop
> > 
> > Isn't this taken care of in `ProcessAudioBlock`? Maybe I'm missing something.
> 
> The code in ProcessAudioBlock() is still relying on CopyFromBuffer() not to
> advance streamPosition beyond mStop.  CopyFromBuffer() checks mStop if not
> resampling.

Ah, yes, sorry.

> > >                                             outputData, &outSamples);
> > > +      if (++i == aChannels) {
> > > +        mRemainingResamplerTail -= inSamples;
> > 
> > Could we release the resampler and switch to taking the normal "copy" path
> > when resampling is not needed anymore and we have consumed all the resampled
> > data in the resampler?
> 
> Yes, we could release the resampler when we know that we'll take the
> FillWithZeroes() path, but this is not the right place to release the
> resampler
> because it doesn't destroy when mStop is passed.  It would be better to clean
> up when setting aFinished, if such cleanup is worthwhile.

We can deal with that in another bug if this proves to be a problem.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c995ea111b8b

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8341591 [details] [diff] [review]
remove resampler latency from BufferSource playback v2.1

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1da46deff2

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b1da46deff2

Comment 15

[Karl Tomlinson (:karlt)]

Test was added in bug 938450
https://hg.mozilla.org/integration/mozilla-inbound/rev/3296ae716782