Implement AudioBuffer.copyFromChannel/copyToChannel

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({dev-doc-complete})

Trunk
mozilla27
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [blocking-webaudio-])

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

6 years ago
No description provided.
Assignee

Comment 1

6 years ago
Posted patch Patch (v1) (obsolete) — Splinter Review
Attachment #803516 - Flags: review?(roc)
Assignee

Updated

6 years ago
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.
Assignee

Updated

6 years ago
Blocks: webaudio
Keywords: dev-doc-needed
Assignee

Comment 3

6 years ago
(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
Assignee

Comment 4

6 years ago
Posted patch Patch (v2) (obsolete) — Splinter Review
Attachment #803516 - Attachment is obsolete: true
Attachment #803516 - Flags: review?(roc)
Attachment #803550 - Flags: review?(roc)
Assignee

Updated

6 years ago
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.
Assignee

Comment 8

6 years ago
Posted patch Patch (v3) (obsolete) — Splinter Review
Attachment #805051 - Flags: review?(roc)
Attachment #805051 - Flags: review?(paul)
Assignee

Updated

6 years ago
Attachment #803550 - Attachment is obsolete: true
Attachment #803550 - Flags: review?(roc)
Attachment #803550 - Flags: review?(paul)
Assignee

Updated

6 years ago
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-
Assignee

Comment 10

6 years ago
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.
Assignee

Comment 12

6 years ago
Posted 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+
Assignee

Comment 14

6 years ago
(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
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 920987
No longer depends on: 920987
Assignee

Updated

6 years ago
Depends on: 920987
Depends on: 922458
You need to log in before you can comment on or make changes to this bug.