Closed Bug 886653 Opened 9 years ago Closed 9 years ago

AudioBufferSourceNodeEngine::UpdateSampleRateIfNeeded() calls speex_resampler_skip_zeros() midstream

Categories

(Core :: Web Audio, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox23 --- unaffected
firefox24 + fixed
firefox25 + fixed

People

(Reporter: karlt, Assigned: padenot)

References

Details

(Whiteboard: [blocking-webaudio+][qa-])

Attachments

(1 file)

http://hg.mozilla.org/mozilla-central/annotate/c87a950e1a09/content/media/webaudio/AudioBufferSourceNode.cpp#l346

From jmspeex on irc:
  You should only ever call skip zeros before sending any samples
  if you merely change the rate, you should not call it again.

It may be necessary to change the dopper effect through a PannerNode to cause this.
See also the first paragraph of bug 886381 comment 2.
I suspect calling speex_resampler_skip_zeros() after speex_resampler_init() would be a more appropriate place.
(In reply to Karl Tomlinson (:karlt) from comment #0)
> http://hg.mozilla.org/mozilla-central/annotate/c87a950e1a09/content/media/
> webaudio/AudioBufferSourceNode.cpp#l346
> 
> From jmspeex on irc:
>   You should only ever call skip zeros before sending any samples
>   if you merely change the rate, you should not call it again.

So, should we just move this call to AudioBufferSourceNodeEngine::Resampler() right after we create a resampler?

> It may be necessary to change the dopper effect through a PannerNode to
> cause this.

Not sure what this means...
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> So, should we just move this call to
> AudioBufferSourceNodeEngine::Resampler() right after we create a resampler?

Yes.

> > It may be necessary to change the dopper effect through a PannerNode to
> > cause this.
> 
> Not sure what this means...

This comment was meant to be about reproducing/testing.
I suspect speex_resampler_skip_zeros() is currently only called midstream if the output sample rate changes, and the only way I know for that to happen is through changing velocity vectors.
(In reply to comment #3)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > So, should we just move this call to
> > AudioBufferSourceNodeEngine::Resampler() right after we create a resampler?
> 
> Yes.

Sounds good to me!  Would you like to take this?

> > > It may be necessary to change the dopper effect through a PannerNode to
> > > cause this.
> > 
> > Not sure what this means...
> 
> This comment was meant to be about reproducing/testing.

Ah, I see!

> I suspect speex_resampler_skip_zeros() is currently only called midstream if
> the output sample rate changes,

Correct.

> and the only way I know for that to happen is
> through changing velocity vectors.

The other way you can trigger that is by changing AudioBufferSourceNode.playbackRate.
Happy to take this, but I'm prioritizing HRTF now.  Just leaving notes so things don't get forgotten.
Assignee: nobody → karlt
(In reply to comment #5)
> Happy to take this, but I'm prioritizing HRTF now.  Just leaving notes so
> things don't get forgotten.

Sounds good, thanks!
With the code as it is, and when we are using automation on playbackRate (for
example, on the wubwubwub demo), we have a terrible sound quality. Merely
removing the call make everything work just fine.
Attachment #774632 - Flags: review?(ehsan)
Assignee: karlt → paul
Whiteboard: [blocking-webaudio+]
Attachment #774632 - Flags: review?(ehsan) → review+
checkin-needed for Aurora, a=webaudio.
Keywords: checkin-needed
I'm not going to get to this today and mcMerge is going to clobber then checkin-needed when this lands on m-c.
Whiteboard: [blocking-webaudio+] → [blocking-webaudio+][checkin-needed-aurora]
https://hg.mozilla.org/mozilla-central/rev/d29588812b73
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/853f08e10baf
Keywords: checkin-needed
Whiteboard: [blocking-webaudio+][checkin-needed-aurora] → [blocking-webaudio+]
Assuming this doesn't need QA.
Whiteboard: [blocking-webaudio+] → [blocking-webaudio+][qa-]
You need to log in before you can comment on or make changes to this bug.