1153783 - Implement the `detune` AudioParam for the AudioBufferSourceNode.

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Paul Adenot (:padenot)]

Spec: https://github.com/WebAudio/web-audio-api/issues/333

This is k-rate parameter that is mixed with the `playbackRate`.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Implement the `detune` AudioParam for the AudioBufferSourceNode. r=

This adds test for both .playbackRate and .detune, because it's kind of the
same anyway, and we did not have a proper test for .playbackRate.

Comment 2

[(no longer active)]

Comment on attachment 8591715 [details] [diff] [review]
Implement the `detune` AudioParam for the AudioBufferSourceNode.  r=

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

(This is r- if I you're going to do anything besides removing the static d variable and the corresponding printf.)

::: dom/media/webaudio/AudioBufferSourceNode.cpp
@@ +412,5 @@
> +    float computedPlaybackRate = aPlaybackRate * pow(2, aDetune / 1200.f);
> +    if (d != computedPlaybackRate) {
> +      printf("computedPlaybackRate: %f, aPlaybackRate: %f, aDetune: %f\n", computedPlaybackRate, aPlaybackRate, aDetune);
> +      d = computedPlaybackRate;
> +    }

What is this code?  Left over debugging stuff?

@@ -441,5 @@
>      }
>  
> -    // WebKit treats the playbackRate as a k-rate parameter in their code,
> -    // despite the spec saying that it should be an a-rate parameter. We treat
> -    // it as k-rate. Spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=21592

Finally!  ;-)

@@ +563,5 @@
>    , mLoopStart(0.0)
>    , mLoopEnd(0.0)
>    // mOffset and mDuration are initialized in Start().
>    , mPlaybackRate(new AudioParam(this, SendPlaybackRateToStream, 1.0f))
> +  , mDetune(new AudioParam(this, SendDetuneToStream, 0.0f))

Please pass in a name if you land your other patch first and add the test for it (obviously!).

Comment 3

[Paul Adenot (:padenot)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2)
> Comment on attachment 8591715 [details] [diff] [review]
> Implement the `detune` AudioParam for the AudioBufferSourceNode.  r=
> 
> Review of attachment 8591715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (This is r- if I you're going to do anything besides removing the static d
> variable and the corresponding printf.)
> 
> ::: dom/media/webaudio/AudioBufferSourceNode.cpp
> @@ +412,5 @@
> > +    float computedPlaybackRate = aPlaybackRate * pow(2, aDetune / 1200.f);
> > +    if (d != computedPlaybackRate) {
> > +      printf("computedPlaybackRate: %f, aPlaybackRate: %f, aDetune: %f\n", computedPlaybackRate, aPlaybackRate, aDetune);
> > +      d = computedPlaybackRate;
> > +    }
> 
> What is this code?  Left over debugging stuff?

Yes sorry, I forgot to remove this.

> @@ +563,5 @@
> >    , mLoopStart(0.0)
> >    , mLoopEnd(0.0)
> >    // mOffset and mDuration are initialized in Start().
> >    , mPlaybackRate(new AudioParam(this, SendPlaybackRateToStream, 1.0f))
> > +  , mDetune(new AudioParam(this, SendDetuneToStream, 0.0f))
> 
> Please pass in a name if you land your other patch first and add the test
> for it (obviously!).

Those are added in the patch that adds the name.

Comment 4

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06ecc22ef9d5

Comment 5

[(no longer active)]

Can you please post an intent to implement and ship for this?  Thanks!

Comment 6

[Paul Adenot (:padenot)]

Yes, sorry.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06ecc22ef9d5

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

Release Note Request (optional, but appreciated)
[Why is this notable]: From Paul's post on dev-platform: "This new attribute allows authors to modulate the
`.playbackRate` property of the AudioBufferSourceNode in cents (which is a scale that is more useful in a musical context, because logarithmic instead of linear). This also aligns the AudioBufferSourceNodes with other source nodes, that previously had a `.detune` parameter."
[Suggested wording]: Implemented AudioBufferSourceNode.detune to modulate playback in cents.
[Links (documentation, blog post, etc)]:
http://webaudio.github.io/web-audio-api/#widl-AudioBufferSourceNode-detune

Comment 9

[Paul Adenot (:padenot)]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #8)
> [Suggested wording]: Implemented AudioBufferSourceNode.detune to modulate
> playback in cents.

One correction: s/playback/playback rate/, this changes the meaning completely.

Comment 10

[Chris Mills [:cmills]]

Property documented at:

https://developer.mozilla.org/en-US/docs/Web/API/AudioBufferSourceNode/detune

Also confirming that it's covered on the release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/40#Web_Audio_API

A quick tech review would be great. Thanks!