Closed Bug 915524 Opened 7 years ago Closed 7 years ago

Implement AudioBuffer.copyFromChannel/copyToChannel

Categories

(Core :: Web Audio, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [blocking-webaudio-])

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #803516 - Flags: review?(roc)
Whiteboard: [blocking-webaudio+]
On the list I suggested changing this to copyFromChannel(Float32Array array, unsigned long start, optional unsigned long length), and that suggestion seemed to go down well.

I also suggested implementing copyToChannel with the same signature.
Blocks: webaudio
Keywords: dev-doc-needed
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> On the list I suggested changing this to copyFromChannel(Float32Array array,
> unsigned long start, optional unsigned long length), and that suggestion
> seemed to go down well.
> 
> I also suggested implementing copyToChannel with the same signature.

Oh ok, sorry I'm way behind my email.  I'll do that then.
Summary: Implement AudioBuffer.copyChannelDataTo → Implement AudioBuffer.copyFromChannel/copyToChannel
Attached patch Patch (v2) (obsolete) — Splinter Review
Attachment #803516 - Attachment is obsolete: true
Attachment #803516 - Flags: review?(roc)
Attachment #803550 - Flags: review?(roc)
Attachment #803550 - Flags: review?(paul)
Comment on attachment 803550 [details] [diff] [review]
Patch (v2)

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

Discussion on the list seems to have converged to:
  void copyFromChannel(Float32Array destination, long channelNumber, optional unsigned long startInChannel);
  void copyToChannel(Float32Array source, long channelNumber, optional unsigned long startInChannel);
Er, those startInChannel parameters should just be default parameters defaulting to 0.
I don't think this should block shipping Web Audio. Webkit/Blink don't have it.
Attached patch Patch (v3) (obsolete) — Splinter Review
Attachment #805051 - Flags: review?(roc)
Attachment #805051 - Flags: review?(paul)
Attachment #803550 - Attachment is obsolete: true
Attachment #803550 - Flags: review?(roc)
Attachment #803550 - Flags: review?(paul)
Whiteboard: [blocking-webaudio+] → [blocking-webaudio-]
Comment on attachment 805051 [details] [diff] [review]
Patch (v3)

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

::: content/media/webaudio/AudioBuffer.cpp
@@ +120,5 @@
> +  }
> +
> +  const float* sourceData = mSharedChannels ?
> +    mSharedChannels->GetData(aChannelNumber) :
> +    JS_GetFloat32ArrayData(mJSChannels[aChannelNumber]);

I think we need to do something here to ensure that the array has not been neutered.

@@ +142,5 @@
> +    return;
> +  }
> +
> +  PodCopy(JS_GetFloat32ArrayData(mJSChannels[aChannelNumber]) + aStartInChannel,
> +          aSource.Data(), length);

I think we need to do something here to ensure that the array has not been neutered.
Attachment #805051 - Flags: review?(roc) → review-
Comment on attachment 805051 [details] [diff] [review]
Patch (v3)

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

::: content/media/webaudio/AudioBuffer.cpp
@@ +120,5 @@
> +  }
> +
> +  const float* sourceData = mSharedChannels ?
> +    mSharedChannels->GetData(aChannelNumber) :
> +    JS_GetFloat32ArrayData(mJSChannels[aChannelNumber]);

Hmm, why?  We can get the data out of mSharedChannels just fine, right?

@@ +142,5 @@
> +    return;
> +  }
> +
> +  PodCopy(JS_GetFloat32ArrayData(mJSChannels[aChannelNumber]) + aStartInChannel,
> +          aSource.Data(), length);

The call to RestoreJSChannelData() should do that.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> ::: content/media/webaudio/AudioBuffer.cpp
> @@ +120,5 @@
> > +  }
> > +
> > +  const float* sourceData = mSharedChannels ?
> > +    mSharedChannels->GetData(aChannelNumber) :
> > +    JS_GetFloat32ArrayData(mJSChannels[aChannelNumber]);
> 
> Hmm, why?  We can get the data out of mSharedChannels just fine, right?

I'm talking about the case where mSharedChannels is null and one of mJSChannels was neutered by passing it through postMessage as a Transferable or something like that.
Attached patch Patch (v4)Splinter Review
Ah I see, good point.
Attachment #805051 - Attachment is obsolete: true
Attachment #805051 - Flags: review?(paul)
Attachment #805254 - Flags: review?(roc)
Comment on attachment 805254 [details] [diff] [review]
Patch (v4)

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

::: content/media/webaudio/AudioBuffer.cpp
@@ +118,5 @@
> +    aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
> +    return;
> +  }
> +
> +  if (!mSharedChannels && JS_GetTypedArrayLength(mJSChannels[aChannelNumber]) != mLength) {

Test mJSChannels instead of !mSharedChannels.

@@ +120,5 @@
> +  }
> +
> +  if (!mSharedChannels && JS_GetTypedArrayLength(mJSChannels[aChannelNumber]) != mLength) {
> +    // The array was probably neutered
> +    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);

Throw DOM_INDEX_SIZE_ERR

@@ +146,5 @@
> +  if (!mSharedChannels && JS_GetTypedArrayLength(mJSChannels[aChannelNumber]) != mLength) {
> +    // The array was probably neutered
> +    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return;
> +  }

Test mJSChannels instead of !mSharedChannels. and throw DOM_INDEC_SIZE_ERR
Attachment #805254 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Comment on attachment 805254 [details] [diff] [review]
> Patch (v4)
> 
> Review of attachment 805254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webaudio/AudioBuffer.cpp
> @@ +118,5 @@
> > +    aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
> > +    return;
> > +  }
> > +
> > +  if (!mSharedChannels && JS_GetTypedArrayLength(mJSChannels[aChannelNumber]) != mLength) {
> 
> Test mJSChannels instead of !mSharedChannels.

There is no such a check in a meaningful way.  Whether or not we're holding on to the stolen read-only buffer is determined by testing mSharedChannels.
https://hg.mozilla.org/mozilla-central/rev/3c697b72bb6f
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
No longer depends on: 920987
Depends on: 920987
You need to log in before you can comment on or make changes to this bug.