Implement the playbackRate property of AudioBufferSourceNode

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

unspecified
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

6 years ago
This is needed for the doppler effect for the AudioPannerNode, since the doppler shift is applied by changing the |playbackRate| of the AudioBufferSourceNode.
Assignee

Updated

6 years ago
Depends on: 852366

Comment 1

6 years ago
Note that the doppler shift is an additional rate multiplier and its value should not be reflected through the playbackRate attribute to web content, but this plan makes sense.
Blocks: webaudio
Assignee

Comment 2

6 years ago
This implement the |playbackRate| member of the AudioBufferSourceNode.

I'm not sure about the |ConvertAudioParamToTicks| part, but it seems correct
when I try, for example, to do a
|sourceNode.playbackRate.linearRampToValueAtTime(2, 37.0)|.
Attachment #733770 - Flags: review?(ehsan)

Comment 3

6 years ago
Comment on attachment 733770 [details] [diff] [review]
Implement the playbackRate property of AudioBufferSourceNode. r=

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

r=me with the below fixed.

::: content/media/webaudio/AudioBufferSourceNode.cpp
@@ +299,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.

Please include a URL to the spec bug here.

@@ +487,5 @@
> +AudioBufferSourceNode::SendPlaybackRateToStream(AudioNode* aNode)
> +{
> +  AudioBufferSourceNode* This = static_cast<AudioBufferSourceNode*>(aNode);
> +  AudioNodeStream* ns = static_cast<AudioNodeStream*>(This->mStream.get());
> +  ns->SetTimelineParameter(AudioBufferSourceNodeEngine::PLAYBACKRATE, *This->mPlaybackRate);

You can use AudioNode::SendTimelineParameterToStream like this:

  AudioBufferSourceNode* This = static_cast<AudioBufferSourceNode*>(aNode);
  SendTimelineParameterToStream(This, AudioBufferSourceNodeEngine::PLAYBACKRATE, *This->mPlaybackRate);

::: content/media/webaudio/AudioBufferSourceNode.h
@@ +56,5 @@
> +  }
> +  void SetPlaybackRate(NonNull<AudioParam> aPlaybackRate)
> +  {
> +    mPlaybackRate = aPlaybackRate.get();
> +  }

Drop the setter please.

::: content/media/webaudio/WebAudioUtils.cpp
@@ +28,5 @@
> +      GraphTime graphTime = This->mSourceStream->StreamTimeToGraphTime(streamTime);
> +      destinationStreamTime = This->mDestinationStream->GraphTimeToStreamTime(graphTime);
> +    } else {
> +      destinationStreamTime = This->mDestinationStream->GetCurrentPosition();
> +    }

You should rebase this on top of my patch in bug 857790.

@@ +46,5 @@
>  }
>  
> +void
> +WebAudioUtils::ConvertAudioParamToTicks(AudioParamTimeline& aParam,
> +                                        AudioNodeStream* aDest)

I'm not sure why you need this override.  Just pass nullptr to the source argument in the other override and get rid of this one please.

::: dom/webidl/AudioBufferSourceNode.webidl
@@ +25,3 @@
>      attribute AudioBuffer? buffer;
>  
> +    attribute AudioParam playbackRate;

Argh, this should be a read-only attribute.  I fixed the spec.
Attachment #733770 - Flags: review?(ehsan) → review+

Comment 5

6 years ago
Backed out because of mochitest-1 crashes: https://hg.mozilla.org/integration/mozilla-inbound/rev/8cf0185087b2
https://hg.mozilla.org/mozilla-central/rev/9ac72a22012f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Comment 8

6 years ago
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
You need to log in before you can comment on or make changes to this bug.