Bug 215301

Summary: Align existing AudioParam API with the specification
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web AudioAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: clark_wang, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, jer.noble, kondapallykalyan, philipj, sam, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://www.w3.org/TR/webaudio/#AudioParam
Bug Depends on: 215289    
Bug Blocks: 212611    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
WIP Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2020-08-07 16:38:02 PDT 
Align existing AudioParam API with the specification:
- https://www.w3.org/TR/webaudio/#AudioParam
Comment 1 Chris Dumez 2020-08-07 17:24:08 PDT 
Created attachment 406234 [details]
WIP Patch
Comment 2 Chris Dumez 2020-08-07 18:23:31 PDT 
Created attachment 406238 [details]
WIP Patch
Comment 3 Chris Dumez 2020-08-07 19:34:08 PDT 
Created attachment 406241 [details]
WIP Patch
Comment 4 Chris Dumez 2020-08-07 19:42:41 PDT 
Created attachment 406242 [details]
Patch
Comment 5 Sam Weinig 2020-08-10 10:15:04 PDT 
Comment on attachment 406242 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406242&action=review

> Source/WebCore/ChangeLog:19
> +        - Throw exceptions when events overlap. This part is based on Chromium's implementation here:
> +          - https://github.com/chromium/chromium/blob/master/third_party/blink/renderer/modules/webaudio/audio_param_timeline.cc#L513

Is this not spec'd? If not, is there bug filed on the spec to add it?

> Source/WebCore/Modules/webaudio/AudioParamTimeline.h:90
> +        double time() const { return m_time; }

It may not make sense or make bindings more troublesome, but can we use something like WallTime/MonotonicTime for these time values?
Comment 6 Chris Dumez 2020-08-10 11:01:45 PDT 
Comment on attachment 406242 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406242&action=review

>> Source/WebCore/ChangeLog:19
>> +          - https://github.com/chromium/chromium/blob/master/third_party/blink/renderer/modules/webaudio/audio_param_timeline.cc#L513
> 
> Is this not spec'd? If not, is there bug filed on the spec to add it?

Yes, it is specified here:
https://webaudio.github.io/web-audio-api/#dfn-automation-event

"""
If setValueCurveAtTime() is called for time 𝑇
T
 and duration 𝐷
D
 and there are any events having a time strictly greater than 𝑇
T
, but strictly 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, but it’s ok to schedule a value curve exactly at the time of another event.
Similarly a NotSupportedError exception MUST be thrown if any automation method is called at a time which is contained in [𝑇,𝑇+𝐷)
[
T
,
T
+
D
)
, 𝑇
T
 being the time of the curve and 𝐷
D
 its duration.
"""
Comment 7 Chris Dumez 2020-08-10 11:06:31 PDT 
Created attachment 406312 [details]
Patch
Comment 8 Chris Dumez 2020-08-10 11:58:30 PDT 
Created attachment 406314 [details]
Patch
Comment 9 EWS 2020-08-10 12:29:07 PDT 
Committed r265440: <https://trac.webkit.org/changeset/265440>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406314 [details].
Comment 10 Radar WebKit Bug Importer 2020-08-10 12:31:36 PDT 
<rdar://problem/66793862>