1069825 - Setting a value curve twice with the same startTime and duration should not throw

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Paul Adenot (:padenot)]

Attachment: test case

From: http://stackoverflow.com/questions/25899432/web-audio-api-firefox-setvaluecurveattime

(See attached test case)

The rules, from the spec:
(1) If one of these events is added at a time where there is already an event of the exact same type, then the new event will replace the old one.
(2) If one of these events is added at a time where there is already one or more events of a different type, then it will be placed in the list after them, but before events whose times are after the event.
(3) If setValueCurveAtTime() is called for time T and duration D and there are any events having a time greater than T, but less than T + D, then a NOT_SUPPORTED_ERR exception MUST be thrown. In other words, it's not ok to schedule a value curve during a time period containing other events.
(4) Similarly a NOT_SUPPORTED_ERR exception MUST be thrown if any automation method is called at a time which is inside of the time interval of a SetValueCurve event at time T and duration D.


(1) this is the case, the second call should replace the first one
(2) this is not the case
(3) This is not the case, T and T + D are equal for both calls, so we are not in the [T; T+D) range
(4) This is not the case

The second call should not throw, and it should be a no-op.

Comment 1

[Baptiste Emmanuel]

Attachment: curve_twice.patch.v1

Hi, I would like to work on this bug. I also added a compiled test.

Comment 2

[Paul Adenot (:padenot)]

Comment on attachment 8613624 [details] [diff] [review]
curve_twice.patch.v1

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

Looks good. I'd rather have an HTML test (mochitest, like in dom/media/webaudio/test/), it's easier to share between browsers. We are currently trying to share some tests with Chrome, for example, to make sure everything is compatible, and C++ tests are not shareable, since they rely on non-public interfaces.

Comment 3

[Baptiste Emmanuel]

Attachment: curve_twice.patch.v2

Add the mochitest to the previous patch

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 8615183 [details] [diff] [review]
curve_twice.patch.v2

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

::: dom/media/webaudio/test/test_audioParamSetCurveAtTimeTwice.html
@@ +25,5 @@
> +    source.buffer = sourceBuffer;
> +
> +    var gain = context.createGain();
> +    gain.gain.setValueCurveAtTime(this.curve, T0, this.duration);
> +    gain.gain.setValueCurveAtTime(this.curve, T0, this.duration);

We need to make sure the second curve is set, here, can you make the curves different and compare?

There is also the case where the second curve has a different duration, but since it's the same event type, we are in case one, and it should be replaced. Can you add a test for this?

Comment 5

[Baptiste Emmanuel]

Attachment: curve_twice.patch.v3

Change the first curve's parameters in the mochitest, to show that this is the second curve that is selected.

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 8615295 [details] [diff] [review]
curve_twice.patch.v3

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

That should be alright, thanks!

Comment 7

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c52c76329c3

Comment 8

[Maire Reavy [:mreavy]]

Paul - can we push a new Try?  Is anything blocking this?  Thanks

Comment 9

[Paul Adenot (:padenot)]

Yeah I just tested again and pushed, after some light rebasing.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22f39dd335cc

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/22f39dd335cc