Closed Bug 937057 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Web Audio, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jujjyl, Assigned: karlt)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Hi Karl & Paul -- Can you look into this report?  Jukka also reported Bug 937054 and Bug 937061 -- which are related to this one.
Flags: needinfo?(paul)
Flags: needinfo?(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.
Assignee: nobody → karlt
Blocks: 913854
Flags: needinfo?(paul)
Flags: needinfo?(karlt)
(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...
Applies on top of patches in bug 937475.
Attachment #831383 - Attachment is obsolete: true
Attachment #8340924 - Flags: review?(paul)
This will be tested in the test I'll propose in bug 938450.
Flags: in-testsuite?
Attachment #8340924 - Attachment is patch: true
There is similar existing code where we can save some memory allocation and temporary buffers of zeros, but passing null for input.
Attachment #8340926 - Flags: review?(paul)
Blocks: 938450
Depends on: 937475
Attachment #8340926 - Flags: review?(paul) → review+
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).
Attachment #8340924 - Flags: review?(paul)
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
Attachment #8340926 - Flags: checkin+
(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.
Attachment #8340924 - Attachment is obsolete: true
Attachment #8341591 - Flags: review?(paul)
(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.
Attachment #8341591 - Flags: review?(paul) → review+
https://hg.mozilla.org/mozilla-central/rev/c995ea111b8b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8341591 [details] [diff] [review]
remove resampler latency from BufferSource playback v2.1

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1da46deff2
Attachment #8341591 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/9b1da46deff2
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 947910
Test was added in bug 938450
https://hg.mozilla.org/integration/mozilla-inbound/rev/3296ae716782
Flags: in-testsuite? → in-testsuite+
Depends on: 966636
Depends on: 967924
You need to log in before you can comment on or make changes to this bug.