If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Send all AudioBufferSourceNode param changes to the engine

RESOLVED FIXED in mozilla23

Status

()

Core
Web Audio
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: padenot)

Tracking

Trunk
mozilla23
x86
Mac OS X
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
Created attachment 740351 [details] [diff] [review]
Part 1: Send the AudioBufferSourceNode loop parameter changes to the stream
Attachment #740351 - Flags: review?(paul)
(Assignee)

Comment 2

5 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!
Created attachment 740460 [details] [diff] [review]
Part 2: Send the AudioBufferSourceNode buffer parameter changes to the stream
Attachment #740460 - Flags: review?(paul)
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

5 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-

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6ac63545817c
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?
Created attachment 740777 [details] [diff] [review]
Part 2: Send the AudioBufferSourceNode buffer parameter changes to the stream
Attachment #740777 - Flags: review?(paul)
(Assignee)

Comment 12

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

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

5 years ago
Assignee: ehsan → paul
(Assignee)

Comment 13

5 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

5 years ago
Attachment #740779 - Attachment is obsolete: true
(Assignee)

Comment 14

5 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

5 years ago
Attachment #740779 - Attachment is obsolete: false
(Assignee)

Comment 15

5 years ago
Created attachment 740847 [details] [diff] [review]
followup - Also send the params to the stream when setting the buffer.

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

Updated

5 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

5 years ago
Attachment #740777 - Flags: review?(paul) → review+
(Assignee)

Comment 19

5 years ago
Landed both patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/05cec4050393
https://hg.mozilla.org/integration/mozilla-inbound/rev/979da3f33fa3
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/05cec4050393
https://hg.mozilla.org/mozilla-central/rev/979da3f33fa3
Status: NEW → RESOLVED
Last Resolved: 5 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.