Closed Bug 1308433 Opened 7 years ago Closed 7 years ago

In automation methods, when startTime is < AudioContext.currentTime, clamp to AudioContext.currentTime.

Categories

(Core :: Web Audio, defect, P1)

Product:
Core
Component:
Web Audio
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: padenot, Assigned: dminor)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

Bug 1308433 - In automation methods, when startTime is < AudioContext.currentTime, clamp to AudioContext.currentTime;
58 bytes, text/x-review-board-request
padenot
: review+
Details 
This allow to compensate for delays in thread communication, amongst other things.

Spec changes at https://github.com/webaudio/web-audio-api/commit/a37db9a3c2705d5c67812f2722fa03a5b8ac27a7.
Rank: 15
Priority: P2 → P1
Depends on: 1184057
Assignee: nobody → dminor
Status: NEW → ASSIGNED
I think this is all we need here, maybe we also need to adjust the start time when we first look at the event in AudioEventTimeline as well.
Comment on attachment 8802457 [details]
Bug 1308433 - In automation methods, when startTime is < AudioContext.currentTime, clamp to AudioContext.currentTime;

https://reviewboard.mozilla.org/r/86864/#review86592

This could use a test I think.
Comment on attachment 8802457 [details]
Bug 1308433 - In automation methods, when startTime is < AudioContext.currentTime, clamp to AudioContext.currentTime;

https://reviewboard.mozilla.org/r/86864/#review86626
Attachment #8802457 - Flags: review?(padenot)
Hi Paul, I was wondering if you had any suggestions on how to test this. I've tried using the onended events from ConstantSourceNode but those don't seem to fire until after the OfflineAudioContext has stopped rendering. I might be able to use a ScriptProcessorNode to set values part way through rendering, but it is deprecated anyway. Maybe this is a use case for adding suspend/resume in Bug 1265406 before getting back to this.

Another option would be to clamp the times inside AudioEventTimeline which would let me test using the gtest, but something like that should maybe wait until Bug 1184057 is fixed, I think it could end up being an ugly fix if done now.

Please let me know what you think. Thanks!
Flags: needinfo?(padenot)
With a realtime context, don't be afraid to use a ScriptProcessor, as it
works well for reading from the graph and its worklet replacement is not yet
available.

testing/web-platform/tests/webaudio/the-audio-api/the-audioparam-interface/retrospective-setValueAtTime.html should need updating.
If it is not failing with this change, then do you know why?

Can that test be modified for the new behaviour by checking that two different start times in the past produce the same result?
Might also be worth testing this, since it is kind of unexpected:

1. Initially set event 1 at a short time in the future.
2. Use ended to get notification of when that time has passed.
3. Add event 2 with a time prior to event 1.
4. Add a ScriptProcessor to check that the effect of event 2 is as if it was
   scheduled at time after event 1.
(In reply to Karl Tomlinson (:karlt) from comment #9)
> testing/web-platform/tests/webaudio/the-audio-api/the-audioparam-interface/
> retrospective-setValueAtTime.html should need updating.
> If it is not failing with this change, then do you know why?

It turns out it is failing! I didn't notice in my try push because there were a lot of intermittent failures in the same chunk before that test was run.

Thanks for your suggestions.
Flags: needinfo?(padenot)
Comment on attachment 8802457 [details]
Bug 1308433 - In automation methods, when startTime is < AudioContext.currentTime, clamp to AudioContext.currentTime;

https://reviewboard.mozilla.org/r/86864/#review93176
Attachment #8802457 - Flags: review?(padenot) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b63a68fbfd2e
In automation methods, when startTime is < AudioContext.currentTime, clamp to AudioContext.currentTime; r=padenot
https://hg.mozilla.org/mozilla-central/rev/b63a68fbfd2e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1228207
No longer blocks: 1228207
You need to log in before you can comment on or make changes to this bug.