(no longer active) Assignee
	Description • 11 years ago

      No description provided.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 2 • 11 years ago

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.

	(no longer active) Assignee
	Comment 3 • 11 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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 5 • 11 years ago

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);

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 6 • 11 years ago

Er, those startInChannel parameters should just be default parameters defaulting to 0.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 7 • 11 years ago

I don't think this should block shipping Web Audio. Webkit/Blink don't have it.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 9 • 11 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]);

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.

	(no longer active) Assignee
	Comment 10 • 11 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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 11 years ago

(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.

	(no longer active) Assignee
	Comment 12 • 11 years ago

Ah I see, good point.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 13 • 11 years ago

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

	(no longer active) Assignee
	Comment 14 • 11 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.

	(no longer active) Assignee
	Comment 15 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c697b72bb6f

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/3c697b72bb6f

	Jean-Yves Perrier [:teoli]
	Comment 17 • 10 years ago

Doc is up-to-date:
https://developer.mozilla.org/en-US/docs/Web/API/AudioBuffer.copyFromChannel
https://developer.mozilla.org/en-US/docs/Web/API/AudioBuffer.copyToChannel
https://developer.mozilla.org/en-US/docs/Web/API/AudioBuffer
and
https://developer.mozilla.org/en-US/Firefox/Releases/27