Closed Bug 1153783 Opened 5 years ago Closed 5 years ago

Implement the `detune` AudioParam for the AudioBufferSourceNode.

Categories

(Core :: Web Audio, defect)

32 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed
relnote-firefox --- 40+

People

(Reporter: padenot, Assigned: padenot)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

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

This is k-rate parameter that is mixed with the `playbackRate`.
Summary: Implement the `detune` parameter for the AudioBufferSourceNode. → Implement the `detune` AudioParam for the AudioBufferSourceNode.
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.
Attachment #8591715 - Flags: review?(ehsan)
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!).
Attachment #8591715 - Flags: review?(ehsan) → review+
(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.
Can you please post an intent to implement and ship for this?  Thanks!
Yes, sorry.
Flags: needinfo?(padenot)
https://hg.mozilla.org/mozilla-central/rev/06ecc22ef9d5
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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
relnote-firefox: --- → ?
(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.
Flags: needinfo?(padenot)
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!
You need to log in before you can comment on or make changes to this bug.