Closed Bug 1069825 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: Web Audio, defect, P2)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: padenot, Unassigned)

Details

Attachments

(2 files, 2 obsolete files)

Attached file 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.
Attached patch curve_twice.patch.v1 (obsolete) — Splinter Review
Hi, I would like to work on this bug. I also added a compiled test.
Attachment #8613624 - Flags: feedback?(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.
Attachment #8613624 - Flags: feedback?(padenot)
Attached patch curve_twice.patch.v2 (obsolete) — Splinter Review
Add the mochitest to the previous patch
Attachment #8613624 - Attachment is obsolete: true
Attachment #8615183 - Flags: review?(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?
Attachment #8615183 - Flags: review?(padenot)
Change the first curve's parameters in the mochitest, to show that this is the second curve that is selected.
Attachment #8615183 - Attachment is obsolete: true
Attachment #8615295 - Flags: feedback?(padenot)
Comment on attachment 8615295 [details] [diff] [review]
curve_twice.patch.v3

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

That should be alright, thanks!
Attachment #8615295 - Flags: feedback?(padenot) → feedback+
Attachment #8615295 - Flags: feedback+ → review+
Paul - can we push a new Try?  Is anything blocking this?  Thanks
Rank: 25
Flags: needinfo?(padenot)
Priority: -- → P2
Yeah I just tested again and pushed, after some light rebasing.
Flags: needinfo?(padenot)
https://hg.mozilla.org/mozilla-central/rev/22f39dd335cc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.