1308436 - Change behavior of setValueCurveAtTime to add an event at the end of the curve

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P3)

Description

[Paul Adenot (:padenot)]

`setValueCurveAtTime` is modified so that an implicit call to `setValueAtTime` is made at the end of the curve duration.  This makes any following automation events start from the end of the `setValueCurveAtTime` event with the last value of the curve.

Spec changes at https://github.com/webaudio/web-audio-api/commit/ee1090becbcc71ca21b08abeb5ad4b8061d65627.

Comment 1

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 2

[theo.rabut]

Attachment: Bug 1308436 - Implicit call to SetValueAtTime in SetValueCurve

Review commit: https://reviewboard.mozilla.org/r/193910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193910/

Comment 3

[theo.rabut]

Comment on attachment 8922764 [details]
Bug 1308436 - Implicit call to SetValueAtTime in SetValueCurve

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193910/diff/1-2/

Comment 4

[Paul Adenot (:padenot)]

I've opened a spec bug for this: https://github.com/WebAudio/web-audio-api/issues/1434

Comment 5

[Paul Adenot (:padenot)]

Comment on attachment 8922764 [details]
Bug 1308436 - Implicit call to SetValueAtTime in SetValueCurve

https://reviewboard.mozilla.org/r/193910/#review198992

::: dom/media/webaudio/AudioEventTimeline.h:183
(Diff revision 2)
> -          TimeOf(mEvents[i]) + mEvents[i].mDuration >= TimeOf(aEvent)) {
> +          TimeOf(mEvents[i]) + mEvents[i].mDuration > TimeOf(aEvent)) {
>          aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
>          return false;
>        }
>      }
> -
> +      

Trailing spaces.

::: dom/media/webaudio/AudioParam.h
(Diff revision 2)
>      if (!WebAudioUtils::IsTimeValid(aStartTime)) {
>        aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>        return this;
>      }
>      aValues.ComputeLengthAndData();
> -

Leave this blank line, please.

::: dom/media/webaudio/AudioParam.h:62
(Diff revision 2)
> -
>      aStartTime = std::max(aStartTime, GetParentObject()->CurrentTime());
>      EventInsertionHelper(aRv, AudioTimelineEvent::SetValueCurve,
>                           aStartTime, 0.0f, 0.0f, aDuration, aValues.Data(),
>                           aValues.Length());
> +    

Trailing spaces.

::: dom/media/webaudio/AudioParam.h:64
(Diff revision 2)
>      EventInsertionHelper(aRv, AudioTimelineEvent::SetValueCurve,
>                           aStartTime, 0.0f, 0.0f, aDuration, aValues.Data(),
>                           aValues.Length());
> +    
> +    if(aDuration > 0){
> +      aStartTime = aStartTime + aDuration;

Don't overwrite the parameter, use another variable with a meaningful name.

::: dom/media/webaudio/AudioParam.h:66
(Diff revision 2)
>                           aValues.Length());
> +    
> +    if(aDuration > 0){
> +      aStartTime = aStartTime + aDuration;
> +      EventInsertionHelper(aRv, AudioTimelineEvent::SetValueAtTime, aStartTime,
> +                           aValues.Data()[aValues.Length()-1]);

Spaces around '-'.

Comment 6

[theo.rabut]

Comment on attachment 8922764 [details]
Bug 1308436 - Implicit call to SetValueAtTime in SetValueCurve

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193910/diff/2-3/

Comment 7

[Paul Adenot (:padenot)]

When running the test:

[task 2017-11-03T13:45:52.888Z] 13:45:52     INFO - TEST-UNEXPECTED-FAIL | dom/media/webaudio/test/test_audioParamSetCurveAtTimeTwice.html | uncaught exception - SyntaxError: An invalid or illegal string was specified at createGraph@http://mochi.test:8888/tests/dom/media/webaudio/test/test_audioParamSetCurveAtTimeTwice.html:35:5
[task 2017-11-03T13:45:52.888Z] 13:45:52     INFO - runTestOnContext@http://mochi.test:8888/tests/dom/media/webaudio/test/webaudio.js:202:20
[task 2017-11-03T13:45:52.888Z] 13:45:52     INFO - testOnNormalContext@http://mochi.test:8888/tests/dom/media/webaudio/test/webaudio.js:222:7
[task 2017-11-03T13:45:52.889Z] 13:45:52     INFO - runTestFunction@http://mochi.test:8888/tests/dom/media/webaudio/test/webaudio.js:253:5
[task 2017-11-03T13:45:52.890Z] 13:45:52     INFO - rval@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:146:17
[task 2017-11-03T13:45:52.891Z] 13:45:52     INFO - EventHandlerNonNull*this.addLoadEvent@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:171:13
[task 2017-11-03T13:45:52.892Z] 13:45:52     INFO - @http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:1441:5

Comment 8

[Paul Adenot (:padenot)]

Comment on attachment 8922764 [details]
Bug 1308436 - Implicit call to SetValueAtTime in SetValueCurve

https://reviewboard.mozilla.org/r/193910/#review201492

This fails tests.

Comment 9

[theo.rabut]

Comment on attachment 8922764 [details]
Bug 1308436 - Implicit call to SetValueAtTime in SetValueCurve

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193910/diff/3-4/

Comment 10

[theo.rabut]

Comment on attachment 8922764 [details]
Bug 1308436 - Implicit call to SetValueAtTime in SetValueCurve

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193910/diff/4-5/

Comment 11

[theo.rabut]

This test is supposed to fail. I asked for a new review with a good one.

Comment 12

[Paul Adenot (:padenot)]

I've picked this up again, it's slightly more complicated that it looks like.

Comment 13

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Don't convert the curve duration from double to float in the EventInsertionHelper to avoid losing precision.

Review commit: https://reviewboard.mozilla.org/r/259974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259974/

Comment 14

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Allow inserting an event strictly after the a value curve.

From the spec:

> 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 NotSupportedError
> exception MUST be thrown. In other words, it’s not ok to schedule a value curve
> during a time period containing other events.

i.e., "less" and not "less or equal to". It is valid to place an event exactly
at T+D.

Review commit: https://reviewboard.mozilla.org/r/259976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259976/

Comment 15

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Insert a SetValueAtTime event right after the curve, when doing a SetValueCurveAtTime.

Review commit: https://reviewboard.mozilla.org/r/259978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259978/

Comment 16

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Throw correct exceptions when calling SetValueCurveAtTime with invalid parameters.

Review commit: https://reviewboard.mozilla.org/r/259980/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259980/

Comment 17

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html.

Review commit: https://reviewboard.mozilla.org/r/259982/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259982/

Comment 18

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Update wpt expectations for audioparamn-setValueCurve-exceptions.html.

We're still failing a test: an exponential ramp should be allowed, with a value
of zero, or opposite signs, and the value should be the value of the previous
event, during the duration of the ramp. We'll get that fixed in another bug.

Review commit: https://reviewboard.mozilla.org/r/259984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259984/

Comment 19

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Update other WPT expectations for event-insertion.html.

Review commit: https://reviewboard.mozilla.org/r/259986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259986/

Comment 20

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Implement proper behaviour for SetValueCurveAtTime.

We're using a special event, not exposed to content, that behaves like a
SetValueAtTime, but represents the end of a curve. This allows calling
SetValueCurveAtTime twice with the same start time and different durations
(which is valid, events of the same type at the same time replace each other).

Review commit: https://reviewboard.mozilla.org/r/259988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259988/

Comment 21

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Throw a range error when the curve duration is negative, when calling SetValueCurveAtTime.

Review commit: https://reviewboard.mozilla.org/r/259990/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259990/

Comment 22

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Throw proper exceptions when passing in out of range time values in automation methods.

Review commit: https://reviewboard.mozilla.org/r/259992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259992/

Comment 23

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Remove expectation file for audioparam-exceptional-values.html.ini, it now passes.

Review commit: https://reviewboard.mozilla.org/r/259994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259994/

Comment 24

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Remove test_audioParamSetCurveAtTimeZeroDuration.html.

Per spec, it should throw. This is tested in WPT.

Review commit: https://reviewboard.mozilla.org/r/260284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260284/

Comment 25

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Fix test: webaudioeditor/test/browser_callwatcher-02.js.

The exception type changed, it's necessary to update the test.

Review commit: https://reviewboard.mozilla.org/r/260286/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260286/

Comment 26

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Update WPT for audioparam-method-chaining.html.

Review commit: https://reviewboard.mozilla.org/r/260288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260288/

Comment 27

[Paul Adenot (:padenot)]

Comment on attachment 8995572 [details]
Bug 1308436 - Don't convert the curve duration from double to float in the EventInsertionHelper to avoid losing precision.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259974/diff/1-2/

Comment 28

[Paul Adenot (:padenot)]

Comment on attachment 8995573 [details]
Bug 1308436 - Allow inserting an event strictly after the a value curve.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259976/diff/1-2/

Comment 29

[Paul Adenot (:padenot)]

Comment on attachment 8995574 [details]
Bug 1308436 - Insert a SetValueAtTime event right after the curve, when doing a SetValueCurveAtTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259978/diff/1-2/

Comment 30

[Paul Adenot (:padenot)]

Comment on attachment 8995575 [details]
Bug 1308436 - Throw correct exceptions when calling SetValueCurveAtTime with invalid parameters.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259980/diff/1-2/

Comment 31

[Paul Adenot (:padenot)]

Comment on attachment 8995576 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259982/diff/1-2/

Comment 32

[Paul Adenot (:padenot)]

Comment on attachment 8995577 [details]
Bug 1308436 - Update wpt expectations for audioparamn-setValueCurve-exceptions.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259984/diff/1-2/

Comment 33

[Paul Adenot (:padenot)]

Comment on attachment 8995578 [details]
Bug 1308436 - Update other WPT expectations for event-insertion.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259986/diff/1-2/

Comment 34

[Paul Adenot (:padenot)]

Comment on attachment 8995579 [details]
Bug 1308436 - Implement proper behaviour for SetValueCurveAtTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259988/diff/1-2/

Comment 35

[Paul Adenot (:padenot)]

Comment on attachment 8995580 [details]
Bug 1308436 - Throw a range error when the curve duration is negative, when calling SetValueCurveAtTime.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259990/diff/1-2/

Comment 36

[Paul Adenot (:padenot)]

Comment on attachment 8995581 [details]
Bug 1308436 - Throw proper exceptions when passing in out of range time values in automation methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259992/diff/1-2/

Comment 37

[Paul Adenot (:padenot)]

Comment on attachment 8995582 [details]
Bug 1308436 - Remove expectation file for audioparam-exceptional-values.html.ini, it now passes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259994/diff/1-2/

Comment 38

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=728473580461486734e879ce90a49410f7f1284d

This went a bit out of the initial scope, but it's mostly test fixes to align with what the spec says, related to AudioParams.

Comment 39

[Karl Tomlinson (:karlt)]

Comment on attachment 8995572 [details]
Bug 1308436 - Don't convert the curve duration from double to float in the EventInsertionHelper to avoid losing precision.

https://reviewboard.mozilla.org/r/259974/#review267422

Comment 40

[Karl Tomlinson (:karlt)]

Comment on attachment 8995573 [details]
Bug 1308436 - Allow inserting an event strictly after the a value curve.

https://reviewboard.mozilla.org/r/259976/#review267434

::: dom/media/webaudio/AudioEventTimeline.h:173
(Diff revision 2)
>        if (mEvents[i].mType == AudioTimelineEvent::SetValueCurve &&
>            !(aEvent.mType == AudioTimelineEvent::SetValueCurve &&
>              TimeOf(aEvent) == TimeOf(mEvents[i])) &&
>            TimeOf(mEvents[i]) <= TimeOf(aEvent) &&
> -          TimeOf(mEvents[i]) + mEvents[i].mDuration >= TimeOf(aEvent)) {
> -        aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +          TimeOf(mEvents[i]) + mEvents[i].mDuration > TimeOf(aEvent)) {
> +          aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);

2 space indent inside block.

Comment 41

[Karl Tomlinson (:karlt)]

Comment on attachment 8995575 [details]
Bug 1308436 - Throw correct exceptions when calling SetValueCurveAtTime with invalid parameters.

https://reviewboard.mozilla.org/r/259980/#review267448

::: dom/media/webaudio/AudioEventTimeline.h:166
(Diff revision 2)
>        aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
>        return false;
>      }
>  
> +    if (aEvent.mType == AudioTimelineEvent::SetValueCurve) {
> +      if (aEvent.mCurveLength < 2) {

Why not use the existing SetValueCurve block above and just tighten the mCurveLength check?

It seems odd to throw for length of zero and one point and throw for length of one elsewhere.

Comment 42

[Karl Tomlinson (:karlt)]

Comment on attachment 8995576 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html.

https://reviewboard.mozilla.org/r/259982/#review267450

Comment 43

[Karl Tomlinson (:karlt)]

Comment on attachment 8995578 [details]
Bug 1308436 - Update other WPT expectations for event-insertion.html.

https://reviewboard.mozilla.org/r/259986/#review267452

Comment 44

[Karl Tomlinson (:karlt)]

Comment on attachment 8995580 [details]
Bug 1308436 - Throw a range error when the curve duration is negative, when calling SetValueCurveAtTime.

https://reviewboard.mozilla.org/r/259990/#review267456

::: dom/media/webaudio/AudioEventTimeline.h:158
(Diff revision 2)
>        if (!aEvent.mCurve || !aEvent.mCurveLength) {
>          aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>          return false;
>        }
> +      if (aEvent.mDuration <= 0) {
> +        aRv.Throw(NS_ERROR_RANGE_ERR);

The spec says a "RangeError exception" and links to the "simple exception" corresponding to the ECMAScript error object.  I assume this should be ThrowRangeError().

Comment 45

[Karl Tomlinson (:karlt)]

Comment on attachment 8995581 [details]
Bug 1308436 - Throw proper exceptions when passing in out of range time values in automation methods.

https://reviewboard.mozilla.org/r/259992/#review267458

::: dom/media/webaudio/AudioEventTimeline.h:148
(Diff revision 2)
>      };
>  
>      // Validate the event itself
>      if (!WebAudioUtils::IsTimeValid(TimeOf(aEvent)) ||
>          !WebAudioUtils::IsTimeValid(aEvent.mTimeConstant)) {
> -      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +      aRv.Throw(NS_ERROR_RANGE_ERR);

I assume this and elsewhere should be the ECMAScript object.

::: dom/media/webaudio/AudioEventTimeline.h:163
(Diff revision 2)
>      bool timeAndValueValid = IsValid(aEvent.mValue) &&
>                               IsValid(aEvent.mDuration);
>      if (!timeAndValueValid) {
> -      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +      aRv.Throw(NS_ERROR_RANGE_ERR);
>        return false;
>      }

Is this actually reachable or can this just be an assertion?

I expect webidl provides that these are finite.

Comment 46

[Karl Tomlinson (:karlt)]

Comment on attachment 8995582 [details]
Bug 1308436 - Remove expectation file for audioparam-exceptional-values.html.ini, it now passes.

https://reviewboard.mozilla.org/r/259994/#review267460

Comment 47

[Karl Tomlinson (:karlt)]

Comment on attachment 8995580 [details]
Bug 1308436 - Throw a range error when the curve duration is negative, when calling SetValueCurveAtTime.

https://reviewboard.mozilla.org/r/259990/#review267462

::: dom/media/webaudio/AudioEventTimeline.h:157
(Diff revision 2)
>      if (aEvent.mType == AudioTimelineEvent::SetValueCurve) {
>        if (!aEvent.mCurve || !aEvent.mCurveLength) {
>          aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>          return false;
>        }
> +      if (aEvent.mDuration <= 0) {

I don't like the spec throwing for zero duration because that case could easily be handled, but interoperability wins here.

Comment 48

[Karl Tomlinson (:karlt)]

Comment on attachment 8996011 [details]
Bug 1308436 - Remove test_audioParamSetCurveAtTimeZeroDuration.html.

https://reviewboard.mozilla.org/r/260284/#review267464

Comment 49

[Karl Tomlinson (:karlt)]

Comment on attachment 8996012 [details]
Bug 1308436 - Fix test: webaudioeditor/test/browser_callwatcher-02.js.

https://reviewboard.mozilla.org/r/260286/#review267466

::: devtools/client/webaudioeditor/test/browser_callwatcher-02.js:32
(Diff revision 1)
>    error = await evalInDebuggee("throwDOMException()");
>    is(error.lineNumber, 37, "exception has correct lineNumber");
>    is(error.columnNumber, 0, "exception has correct columnNumber");
> -  is(error.code, 9, "exception has correct code");
> +  is(error.code, 0, "exception has correct code");

This isn't going make much sense when this is an error object.

I guess throwDOMException should be updated to something that still throws a DOMException.

Comment 50

[Karl Tomlinson (:karlt)]

Comment on attachment 8996013 [details]
Bug 1308436 - Update WPT for audioparam-method-chaining.html.

https://reviewboard.mozilla.org/r/260288/#review267468

Comment 51

[Karl Tomlinson (:karlt)]

Comment on attachment 8995574 [details]
Bug 1308436 - Insert a SetValueAtTime event right after the curve, when doing a SetValueCurveAtTime.

https://reviewboard.mozilla.org/r/259978/#review267438

::: commit-message-c417f:1
(Diff revision 2)
> +Bug 1308436 - Insert a SetValueAtTime event right after the curve, when doing a SetValueCurveAtTime. r?karlt

Can you update the comment, please, in the code here that depends on this?
https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/dom/media/webaudio/AudioEventTimeline.cpp#236-237

There will now always be a next node.
And the reason why aNext doesn't need to be considered in determining
values after the curve is that it is always SetValueAtTime.
A MOZ_ASSERT() to confirm that would also be useful.

::: dom/media/webaudio/AudioParam.h:66
(Diff revision 2)
> +    EventInsertionHelper(aRv, AudioTimelineEvent::SetValueAtTime,
> +                         aStartTime + aDuration, aValues[aValues.Length() - 1]);

The intention is that "following automations will start from the end of the
setValueCurveAtTime() event."

Calling EventInsertionHelper() will place the SetValueAtTime after any
following automations already at T + D, essentially erasing existing following
automations.

I expect it will be easier to add the implicit SetValueAtTime immediately
after SetValueCurve in AudioEventTimeline::InsertEvent().

I expect there are alternative approaches also that do not require having what
is really a redundant AudioTimelineEvent.  e.g. aValues[aValues.Length() - 1]
could be passed as the aValue parameter to EventInsertionHelper and
TimeOf(aPrevious) in AudioEventTimeline::GetValuesAtTimeHelperInternal()
could be changed to include mDuration.

Comment 52

[Karl Tomlinson (:karlt)]

Comment on attachment 8995579 [details]
Bug 1308436 - Implement proper behaviour for SetValueCurveAtTime.

https://reviewboard.mozilla.org/r/259988/#review267444

::: commit-message-9a871:6
(Diff revision 2)
> +Bug 1308436 - Implement proper behaviour for SetValueCurveAtTime. r?karlt
> +
> +We're using a special event, not exposed to content, that behaves like a
> +SetValueAtTime, but represents the end of a curve. This allows calling
> +SetValueCurveAtTime twice with the same start time and different durations
> +(which is valid, events of the same type at the same time replace each other).

Can you point me at the part of the spec, please, that implies that "events of
the same type at the same time replace each other".

"If one of these events is added at a time where there is already one or more
events, then it will be placed in the list after them".

But the curves would overlap in that situation, and so I would apply
"a NotSupportedError 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 and duration" to that situation.

"inside" is not particularly specific about whether it includes events at T,
but the same overlap problem would exist for any type of event placed at T.

If this is really the desired behavior, then I suspect this would be
simpler without the redundant event.

Comment 53

[Karl Tomlinson (:karlt)]

Comment on attachment 8995577 [details]
Bug 1308436 - Update wpt expectations for audioparamn-setValueCurve-exceptions.html.

https://reviewboard.mozilla.org/r/259984/#review267470

::: testing/web-platform/meta/webaudio/the-audio-api/the-audioparam-interface/audioparam-setValueCurve-exceptions.html.ini:2
(Diff revision 2)
> -  [X setValueAtTime(1, 0.018750000000000003) threw "SyntaxError" instead of NotSupportedError.]
> +  [X exponentialRampToValueAtTime(1, 0.0075) incorrectly threw SyntaxError: "An invalid or illegal string was specified".]
>      expected: FAIL
>  
> -  [X linearRampToValueAtTime(1, 0.018750000000000003) threw "SyntaxError" instead of NotSupportedError.]
> +  [< [start-end\] 1 out of 9 assertions were failed.]
>      expected: FAIL
>  
> -  [X exponentialRampToValueAtTime(1, 0.018750000000000003) threw "SyntaxError" instead of NotSupportedError.]
> +  [# AUDIT TASK RUNNER FINISHED: 1 out of 5 tasks were failed.]

I wonder why

Comment 54

[Karl Tomlinson (:karlt)]

Comment on attachment 8995577 [details]
Bug 1308436 - Update wpt expectations for audioparamn-setValueCurve-exceptions.html.

https://reviewboard.mozilla.org/r/259984/#review267470

> I wonder why

I was wondering why the exponentialRampToValueAtTime subtest name had changed, but it hasn't.
I guess that linearRampToValueAtTime now passing means that
exponentialRampToValueAtTime(1, 0.0075) has started to run.

Anyway, I meant to delete this comment.

Comment 55

[Paul Adenot (:padenot)]

Comment on attachment 8995579 [details]
Bug 1308436 - Implement proper behaviour for SetValueCurveAtTime.

https://reviewboard.mozilla.org/r/259988/#review267518

::: commit-message-9a871:6
(Diff revision 2)
> +Bug 1308436 - Implement proper behaviour for SetValueCurveAtTime. r?karlt
> +
> +We're using a special event, not exposed to content, that behaves like a
> +SetValueAtTime, but represents the end of a curve. This allows calling
> +SetValueCurveAtTime twice with the same start time and different durations
> +(which is valid, events of the same type at the same time replace each other).

huh, this got removed from the spec in:

https://github.com/WebAudio/web-audio-api/issues/573#issuecomment-151032325

Comment 56

[Paul Adenot (:padenot)]

Looks like I implemented badly the wrong behaviour, trying to preserve some of what gecko does today and that we encode in tests, but it's wrong.

Comment 57

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Update WPT expectations for audioparam-method-chaining.html. r?karlt

MozReview-Commit-ID: KmfDil9LKGS

Comment 58

[Karl Tomlinson (:karlt)]

Comment on attachment 9002466 [details]
Bug 1308436 - Update WPT expectations for audioparam-method-chaining.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 59

[Paul Adenot (:padenot)]

I filed [0] so that we make the rules for inserting events more precise. I've addressed all the comments, but we're failing the wpt case where two curves are trying to be inserted at the same time [1].

[0]: https://github.com/WebAudio/web-audio-api/issues/1721
[1]: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webaudio/the-audio-api/the-audioparam-interface/event-insertion.html#60

Comment 60

[Karl Tomlinson (:karlt)]

Looks like that test is out of date.  Attempting to insert the second (overlapping) curve should throw.

Comment 61

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Don't convert the curve duration from double to float in the EventInsertionHelper to avoid losing precision. r?karlt

MozReview-Commit-ID: 9vazrYi3J8C

Comment 62

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Add a test case for a negative curve duration in audioparam-exceptional-values.html. r?karlt

Comment 63

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Rework the AudioParam event insertion algorithm to be spec compliant, and fix the exception types and values. r?karlt

Comment 64

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Adjust the AudioParam processing algorithm to match the spec. r?karlt

Ramps now take into account the end of the curve as a start value for their
automation.

Comment 65

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html. r?karlt

MozReview-Commit-ID: DDAUkkwdACv

Comment 66

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Remove test_audioParamSetCurveAtTimeZeroDuration.html. r?karlt

Per spec, it should throw. This is tested in WPT.

MozReview-Commit-ID: InzdbPQM6HD

Comment 67

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Update other WPT expectations for event-insertion.html. r?karlt

MozReview-Commit-ID: BRcethELDJB

Comment 68

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Update devtools tests with another Web Audio API call the throws the right exception. r?karlt

Comment 69

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Remove audioparam-setValueCurve-exceptions.html.ini, it's now passing. r?karlt

Comment 70

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Remove TestAudioEventTimeline.cpp. r?karlt

It's too difficult to maintain, obsolete and also it duplicates the
functionnality of wpt.

Comment 71

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Remove expectation file for audioparam-exceptional-values.html.ini, it now passes. r?karlt

MozReview-Commit-ID: 4iXmUDgr81x

Comment 72

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Fix a test case and add more test cases for overlapping SetValueCurveAtTime. r?karlt

Comment 73

[Paul Adenot (:padenot)]

Attachment: Bug 1308436 - Remove test_audioParamSetCurveAtTimeTwice.html because it's now wrong per spec. r?karlt

Comment 74

[Karl Tomlinson (:karlt)]

Comment on attachment 9003508 [details]
Bug 1308436 - Don't convert the curve duration from double to float in the EventInsertionHelper to avoid losing precision. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 75

[Karl Tomlinson (:karlt)]

Comment on attachment 9003513 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 76

[Karl Tomlinson (:karlt)]

Comment on attachment 9003518 [details]
Bug 1308436 - Remove audioparam-setValueCurve-exceptions.html.ini, it's now passing. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 77

[Karl Tomlinson (:karlt)]

Comment on attachment 9003516 [details]
Bug 1308436 - Update other WPT expectations for event-insertion.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 78

[Karl Tomlinson (:karlt)]

Comment on attachment 9003520 [details]
Bug 1308436 - Remove expectation file for audioparam-exceptional-values.html.ini, it now passes. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 79

[Karl Tomlinson (:karlt)]

Comment on attachment 9003514 [details]
Bug 1308436 - Remove test_audioParamSetCurveAtTimeZeroDuration.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 80

[Karl Tomlinson (:karlt)]

I wonder whether it is the phabsend command from
https://bitbucket.org/kmaglione/hgext/src/default/phabricator.py
that caused the funny dependency graph in the "Stack" at
https://phabricator.services.mozilla.com/D4108
which is not connected to the Stack at
https://phabricator.services.mozilla.com/D3791

"arc patch --nobranch D4103" doesn't apply due to conflicts, but using "arc
patch --nobranch D4099" followed by "arc patch --skip-dependencies --nobranch
<rev>" for each revision in the order of bugzilla attachments applies fine.

I don't know whether that is related to the disconnected stacks, attempts to
apply to https://hg.mozilla.org/mozilla-central/rev/3f99a091261f (the resting
place of D3507) and silently rebase (but leave behind pruned changesets), or
something else.

Anyway I can find my way around now.

Comment 81

[Karl Tomlinson (:karlt)]

Comment on attachment 9003509 [details]
Bug 1308436 - Add a test case for a negative curve duration in audioparam-exceptional-values.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 82

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #80)
> I don't know whether that is related to the disconnected stacks,

https://phabricator.services.mozilla.com/D4099 was missing from the
dependencies of the other stack, which is why it did not apply.

> attempts to and silently rebase (but leave behind pruned changesets)

This was due to the --nobranch argument.

Comment 83

[Karl Tomlinson (:karlt)]

Comment on attachment 9003517 [details]
Bug 1308436 - Update devtools tests with another Web Audio API call the throws the right exception. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 84

[Karl Tomlinson (:karlt)]

Comment on attachment 9003511 [details]
Bug 1308436 - Adjust the AudioParam processing algorithm to match the spec.  r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 85

[Karl Tomlinson (:karlt)]

Comment on attachment 9003519 [details]
Bug 1308436 - Remove TestAudioEventTimeline.cpp. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 86

[Karl Tomlinson (:karlt)]

Comment on attachment 9003521 [details]
Bug 1308436 - Fix a test case and add more test cases for overlapping SetValueCurveAtTime. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 87

[Karl Tomlinson (:karlt)]

Comment on attachment 9003522 [details]
Bug 1308436 - Remove test_audioParamSetCurveAtTimeTwice.html because it's now wrong per spec. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 88

[Karl Tomlinson (:karlt)]

Comment on attachment 9002466 [details]
Bug 1308436 - Update WPT expectations for audioparam-method-chaining.html. r?karlt

Karl Tomlinson (:karlt) has requested changes to the revision.

Comment 89

[Karl Tomlinson (:karlt)]

Comment on attachment 9003513 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html. r?karlt

Karl Tomlinson (:karlt) has requested changes to the revision.

Comment 90

[Karl Tomlinson (:karlt)]

Comment on attachment 9003509 [details]
Bug 1308436 - Add a test case for a negative curve duration in audioparam-exceptional-values.html. r?karlt

Karl Tomlinson (:karlt) has requested changes to the revision.

Comment 91

[Karl Tomlinson (:karlt)]

Comment on attachment 9003521 [details]
Bug 1308436 - Fix a test case and add more test cases for overlapping SetValueCurveAtTime. r?karlt

Karl Tomlinson (:karlt) has requested changes to the revision.

Comment 92

[Karl Tomlinson (:karlt)]

If bug 1473915 is not fixed (and it doesn't look like it will be soon), then
the changesets that modify web platform tests (not expectations) need changes
to MANIFEST.json.

Bug 1480644 may mean that sending MANIFEST.json changes to Phabricator is not
a good idea.  (Sometimes people have had more success than other times.)  If
not, then just apply the changes locally before pushing.
I've marked the revisions that need such changes with "Request changes" to
help identify, even though Phabricator may not be the best place to perform
those changes.

Comment 93

[Karl Tomlinson (:karlt)]

Comment on attachment 9002466 [details]
Bug 1308436 - Update WPT expectations for audioparam-method-chaining.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 94

[Karl Tomlinson (:karlt)]

Comment on attachment 9003509 [details]
Bug 1308436 - Add a test case for a negative curve duration in audioparam-exceptional-values.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 95

[Karl Tomlinson (:karlt)]

Comment on attachment 9003513 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 96

[Karl Tomlinson (:karlt)]

Comment on attachment 9003521 [details]
Bug 1308436 - Fix a test case and add more test cases for overlapping SetValueCurveAtTime. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 97

[Karl Tomlinson (:karlt)]

Comment on attachment 9003510 [details]
Bug 1308436 - Rework the AudioParam event insertion algorithm to be spec compliant, and fix the exception types and values. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Comment 98

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c781bf4c4970
Don't convert the curve duration from double to float in the EventInsertionHelper to avoid losing precision. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/08897a32ca05
Add a test case for a negative curve duration in audioparam-exceptional-values.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a729b75c155
Rework the AudioParam event insertion algorithm to be spec compliant, and fix the exception types and values. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2140bc66e3
Adjust the AudioParam processing algorithm to match the spec.  r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ad67bac8643
Fix typo in audioparam-setValueCurve-exceptions.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dfd506e16f4
Remove test_audioParamSetCurveAtTimeZeroDuration.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/09a3186f939e
Update other WPT expectations for event-insertion.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b69df966f74
Update WPT for audioparam-method-chaining.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/33972bbac0d6
Update devtools tests with another Web Audio API call the throws the right exception. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd578b3bcf54
Remove audioparam-setValueCurve-exceptions.html.ini, it's now passing. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/a61e3c9c4ba7
Remove expectation file for audioparam-exceptional-values.html.ini, it now passes. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3233377aec2
Remove TestAudioEventTimeline.cpp. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8691a86020
Fix a test case and add more test cases for overlapping SetValueCurveAtTime. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d905753c20
Remove test_audioParamSetCurveAtTimeTwice.html because it's now wrong per spec. r=karlt

Comment 99

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12773 for changes under testing/web-platform/tests

Comment 100

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 101

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/c781bf4c4970
https://hg.mozilla.org/mozilla-central/rev/08897a32ca05
https://hg.mozilla.org/mozilla-central/rev/0a729b75c155
https://hg.mozilla.org/mozilla-central/rev/9f2140bc66e3
https://hg.mozilla.org/mozilla-central/rev/9ad67bac8643
https://hg.mozilla.org/mozilla-central/rev/7dfd506e16f4
https://hg.mozilla.org/mozilla-central/rev/09a3186f939e
https://hg.mozilla.org/mozilla-central/rev/6b69df966f74
https://hg.mozilla.org/mozilla-central/rev/33972bbac0d6
https://hg.mozilla.org/mozilla-central/rev/cd578b3bcf54
https://hg.mozilla.org/mozilla-central/rev/a61e3c9c4ba7
https://hg.mozilla.org/mozilla-central/rev/f3233377aec2
https://hg.mozilla.org/mozilla-central/rev/1c8691a86020
https://hg.mozilla.org/mozilla-central/rev/a7d905753c20

Comment 102

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 103

[Karl Tomlinson (:karlt)]

https://phabricator.services.mozilla.com/D4099?vs=10240&id=12155&whitespace=ignore-most#toc
indicates that the commits that landed were the first ones submitted to phabricator, not the ones that were reviewed.
Phabricator says "This revision was automatically updated to reflect the committed changes" for every revision, even the ones that did not change during the review process, which makes the message unhelpful at indicating which revisions pushed differ from the ones reviewed.  The order of commits is also the order prior to review.

Comment 104

[Paul Adenot (:padenot)]

I don't know what happened. I'm going to back everything out and reland the right ones.

Comment 105

[Paul Adenot (:padenot)]

I've manually gone through all the patches and grafted the revisions on top of central, on top of a backout patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e13834bf32e6adb986ffd8405f38a74d7ee379d

Comment 106

[Paul Adenot (:padenot)]

It's green. I'm going to request uplift on the backout so we return to the previous code in 63 (it's too late), and then re-land everything on inbound (backout + new patches).

Comment 107

[Paul Adenot (:padenot)]

Attachment: Backout patch for 63

The wrong patch have landed. I need those patches to be backed out from beta 63, to restore the previous state of the code.

This is the output of a `hg diff --reverse` command and is green on try.

Comment 108

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1452a7d4442
Backout all patches of bug 1308436, wrong patches have landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d40e82bb57
Don't convert the curve duration from double to float in the EventInsertionHelper to avoid losing precision. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba41d875df9b
Add a test case for a negative curve duration in audioparam-exceptional-values.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d0486eaac8
Remove TestAudioEventTimeline.cpp. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/65901b920626
Rework the AudioParam event insertion algorithm to be spec compliant, and fix the exception types and values. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/6321efdf65d5
Adjust the AudioParam processing algorithm to match the spec. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5a98b2f4fdc
Fix typo in audioparam-setValueCurve-exceptions.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/8834636b7209
Remove test_audioParamSetCurveAtTimeZeroDuration.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf9bdaa1bfb
Update other WPT expectations for event-insertion.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/41fbd4fd0622
Update devtools tests with another Web Audio API call the throws the right exception. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/57720a1c67dc
Remove audioparam-setValueCurve-exceptions.html.ini, it's now passing. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/290b7ee630e3
Remove expectation file for audioparam-exceptional-values.html.ini, it now passes. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6a1baa45d1c
Fix a test case and add more test cases for overlapping SetValueCurveAtTime. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fcef05c05bb
Remove test_audioParamSetCurveAtTimeTwice.html because it's now wrong per spec. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/f58ddafd8a65
Update WPT expectations for audioparam-method-chaining.html. r=karlt

Comment 109

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12831 for changes under testing/web-platform/tests

Comment 110

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 111

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 112

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/d5d40e82bb57
https://hg.mozilla.org/mozilla-central/rev/ba41d875df9b
https://hg.mozilla.org/mozilla-central/rev/d7d0486eaac8
https://hg.mozilla.org/mozilla-central/rev/65901b920626
https://hg.mozilla.org/mozilla-central/rev/6321efdf65d5
https://hg.mozilla.org/mozilla-central/rev/b5a98b2f4fdc
https://hg.mozilla.org/mozilla-central/rev/8834636b7209
https://hg.mozilla.org/mozilla-central/rev/eaf9bdaa1bfb
https://hg.mozilla.org/mozilla-central/rev/41fbd4fd0622
https://hg.mozilla.org/mozilla-central/rev/57720a1c67dc
https://hg.mozilla.org/mozilla-central/rev/290b7ee630e3
https://hg.mozilla.org/mozilla-central/rev/b6a1baa45d1c
https://hg.mozilla.org/mozilla-central/rev/7fcef05c05bb
https://hg.mozilla.org/mozilla-central/rev/f58ddafd8a65

Comment 113

[Paul Adenot (:padenot)]

(In reply to Paul Adenot (:padenot) from comment #107)
> Created attachment 9006274 [details] [diff] [review]
> Backout patch for 63
> 
> The wrong patch have landed. I need those patches to be backed out from beta
> 63, to restore the previous state of the code.
> 
> This is the output of a `hg diff --reverse` command and is green on try.

Forgot to put the try for this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=282a241af571236919c4790725e9d978c0708358

Comment 114

[Pascal Chevrel:pascalc]

Comment on attachment 9006274 [details] [diff] [review]
Backout patch for 63

Approved for 63 beta 4, thanks for the quick action.

Comment 115

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/AudioParam/setValueCurveAtTime

And listed on Firefox 63 for developers.