Send all AudioBufferSourceNode param changes to the engine

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: padenot)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Currently we don't do this for many params, such as the buffer, looping attributes, etc.
Depends on: 864322
Assignee

Comment 2

6 years ago
Comment on attachment 740351 [details] [diff] [review]
Part 1: Send the AudioBufferSourceNode loop parameter changes to the stream

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

::: content/media/webaudio/test/test_audioBufferSourceNodeLoop.html
@@ +36,5 @@
> +  source.connect(sp);
> +  sp.connect(context.destination);
> +  sp.onaudioprocess = function(e) {
> +    is(e.inputBuffer.numberOfChannels, 1, "input buffer must have only one channel");
> +    dump(e.inputBuffer.getChannelData(0)[2047]);

Maybe this is a leftover dump that should be removed?

::: content/media/webaudio/test/test_audioBufferSourceNodeLoopStartEnd.html
@@ +44,5 @@
> +  source.connect(sp);
> +  sp.connect(context.destination);
> +  sp.onaudioprocess = function(e) {
> +    is(e.inputBuffer.numberOfChannels, 1, "input buffer must have only one channel");
> +    dump(e.inputBuffer.getChannelData(0)[2047]);

Same here.
Attachment #740351 - Flags: review?(paul) → review+
Yeah, both of these were debugging dumps, sorry, forgot to take them out!
Comment on attachment 740351 [details] [diff] [review]
Part 1: Send the AudioBufferSourceNode loop parameter changes to the stream

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac63545817c
Attachment #740351 - Flags: checkin+
Whiteboard: [leave open]
I think this is all that I needed to do here.
Assignee

Comment 7

6 years ago
Comment on attachment 740460 [details] [diff] [review]
Part 2: Send the AudioBufferSourceNode buffer parameter changes to the stream

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

::: content/media/webaudio/AudioBufferSourceNode.cpp
@@ +492,5 @@
> +AudioBufferSourceNode::SendOffsetAndDurationParametersToStream(AudioNodeStream* aStream,
> +                                                               double aOffset,
> +                                                               double aDuration)
> +{
> +  MOZ_ASSERT(mBuffer, "How come we don't have a buffer here?");

Consider this javascript code:

  var ctx = new AudioContext();
  var source = ctx.createBufferSource();
  source.start(100, 100, 100);
  source.buffer = null;

This assert blows up.

::: content/media/webaudio/AudioBufferSourceNode.h
@@ +82,3 @@
>    {
>      mBuffer = aBuffer;
> +    SendBufferParameterToStream(aCx);

Maybe:

  if (mBuffer) {
    SendBufferParameterToStream(aCx);
  }
Attachment #740460 - Flags: review?(paul) → review-
You're completely right, good catch!  Patch + test for this case coming up.
(In reply to comment #7)
> ::: content/media/webaudio/AudioBufferSourceNode.h
> @@ +82,3 @@
> >    {
> >      mBuffer = aBuffer;
> > +    SendBufferParameterToStream(aCx);
> 
> Maybe:
> 
>   if (mBuffer) {
>     SendBufferParameterToStream(aCx);
>   }

No, that would make it a no-op to set the buffer to null, wouldn't it?
Assignee

Comment 12

6 years ago
There is a problem I did not catch during the review in the first patch, if we
set the params and then the buffer, and then calling start, because
|SendLoopParametersToStream| is no-op when |mBuffer| is null. This patch fixes
the issue by evaluating the parameters before sending them really lazy.
Attachment #740779 - Flags: review?(ehsan)
Assignee

Updated

6 years ago
Assignee: ehsan → paul
Assignee

Comment 13

6 years ago
Comment on attachment 740779 [details] [diff] [review]
followup - Also send the params to the stream when setting the buffer.

This is addressed in you last patch, nevermind :-)
Attachment #740779 - Flags: review?(ehsan)
Assignee

Updated

6 years ago
Attachment #740779 - Attachment is obsolete: true
Assignee

Comment 14

6 years ago
Comment on attachment 740779 [details] [diff] [review]
followup - Also send the params to the stream when setting the buffer.

Man, I can't read. This is still needed.
Attachment #740779 - Flags: review?(ehsan)
Assignee

Updated

6 years ago
Attachment #740779 - Attachment is obsolete: false
Assignee

Comment 15

6 years ago
Now rebased on top of your patches, and with a test.
Attachment #740847 - Flags: review?(ehsan)
Assignee

Updated

6 years ago
Attachment #740779 - Flags: review?(ehsan)
Comment on attachment 740847 [details] [diff] [review]
followup - Also send the params to the stream when setting the buffer.

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

Very good catch!

::: content/media/webaudio/test/webaudio.js
@@ +41,5 @@
>    var difference = 0;
>    var maxDifference = 0;
>    var firstBadIndex = -1;
>    for (var i = offset || 0; i < Math.min(buf1.length, (offset || 0) + length); ++i) {
> +    dump("("+i+")[" + buf1[i+sourceOffset] + " " + buf2[i+destOffset] + "]\n");

Gah, I meant to take this console.log out!  Can you please take the dump out too?  Doing this will make things very sad on our test machine since you'll get one line of test spew per frame examined here!
Attachment #740847 - Flags: review?(ehsan) → review+
Comment on attachment 740779 [details] [diff] [review]
followup - Also send the params to the stream when setting the buffer.

This is obsolete, AFAICT.
Attachment #740779 - Attachment is obsolete: true
Comment on attachment 740777 [details] [diff] [review]
Part 2: Send the AudioBufferSourceNode buffer parameter changes to the stream

Feel free to land this if you r+ it without comments with your own patch here.  Thanks!
Assignee

Updated

6 years ago
Attachment #740777 - Flags: review?(paul) → review+
https://hg.mozilla.org/mozilla-central/rev/05cec4050393
https://hg.mozilla.org/mozilla-central/rev/979da3f33fa3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
Depends on: 867104
Depends on: 952756
You need to log in before you can comment on or make changes to this bug.