Bug 216425

Summary: Add proper support for AudioContextOptions.sampleRate
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web AudioAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, ews-watchlist, glenn, hta, jer.noble, peng.liu6, philipj, sergio, tommyw, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 212611, 216371    
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch none

Chris Dumez
Reported 2020-09-11 16:52:43 PDT
Add proper support for AudioContextOptions.sampleRate. Our audio context currently always runs at the hardware sample rate, no matter what value is set for AudioContextOptions.sampleRate.
Attachments
Patch (65.08 KB, patch)
2020-09-11 17:19 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (64.18 KB, patch)
2020-09-11 17:53 PDT, Chris Dumez 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (64.76 KB, patch)
2020-09-11 18:27 PDT, Chris Dumez 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (64.84 KB, patch)
2020-09-11 18:32 PDT, Chris Dumez 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (65.55 KB, patch)
2020-09-11 18:38 PDT, Chris Dumez 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (65.53 KB, patch)
2020-09-11 18:50 PDT, Chris Dumez 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (65.67 KB, patch)
2020-09-11 19:37 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (65.80 KB, patch)
2020-09-14 08:19 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2020-09-11 17:19:28 PDT
Created attachment 408574 [details] Patch
Chris Dumez
Comment 2 2020-09-11 17:53:25 PDT
Created attachment 408576 [details] Patch
Chris Dumez
Comment 3 2020-09-11 18:27:34 PDT
Created attachment 408577 [details] Patch
Chris Dumez
Comment 4 2020-09-11 18:32:03 PDT
Created attachment 408578 [details] Patch
Chris Dumez
Comment 5 2020-09-11 18:38:41 PDT
Created attachment 408579 [details] Patch
Chris Dumez
Comment 6 2020-09-11 18:50:17 PDT
Created attachment 408580 [details] Patch
Chris Dumez
Comment 7 2020-09-11 19:37:34 PDT
Created attachment 408581 [details] Patch
Peng Liu
Comment 8 2020-09-11 20:29:41 PDT
Comment on attachment 408581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408581&action=review > Source/WebCore/ChangeLog:9 > + at the hardware's sampleRate, no matter what value for set for AudioContextOptions.sampleRate. Nit. s/for set/set/ > Source/WebCore/ChangeLog:18 > + When creating a AudioDestination, pass the requested AudioContext sample rate Nit. s/a/an/ > Source/WebCore/ChangeLog:58 > + MultiChannelResampler and use in in render() to do the resampling. Nit. s/in in/it in/ > Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:59 > +constexpr CMVideoCodecType kCMVideoCodecType_VP9 { 'vp09' }; There is some update about this in bug 216205. :-)
Eric Carlson
Comment 9 2020-09-11 22:45:34 PDT
Comment on attachment 408581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408581&action=review Can we enable or add tests for AudioContextOptions.sampleRate? > Source/WebCore/ChangeLog:8 > + Add proper support for AudioContextOptions.sampleRate. Previously, our AudioContext always run s/always run/always ran/ > Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:-86 > - float hardwareSampleRate = AudioDestination::hardwareSampleRate(); > - LOG(WebAudio, ">>>> hardwareSampleRate = %f\n", hardwareSampleRate); Is this needed now?
Chris Dumez
Comment 10 2020-09-14 08:14:23 PDT
(In reply to Eric Carlson from comment #9) > Comment on attachment 408581 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=408581&action=review > > Can we enable or add tests for AudioContextOptions.sampleRate? The tests for AudioContextOptions.sampleRate were already passing since we were lying to the JS and returning the sample rate it expected (even though it was not the sample rate we used internally. Do you have an idea on how to better test this? > > > Source/WebCore/ChangeLog:8 > > + Add proper support for AudioContextOptions.sampleRate. Previously, our AudioContext always run > > s/always run/always ran/ > > > Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:-86 > > - float hardwareSampleRate = AudioDestination::hardwareSampleRate(); > > - LOG(WebAudio, ">>>> hardwareSampleRate = %f\n", hardwareSampleRate); > > Is this needed now?
Chris Dumez
Comment 11 2020-09-14 08:19:28 PDT
Created attachment 408706 [details] Patch
Chris Dumez
Comment 12 2020-09-14 08:47:58 PDT
Comment on attachment 408706 [details] Patch Clearing flags on attachment: 408706 Committed r267014: <https://trac.webkit.org/changeset/267014>
Chris Dumez
Comment 13 2020-09-14 08:48:00 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 14 2020-09-14 08:48:17 PDT
<rdar://problem/68856997>
Eric Carlson
Comment 15 2020-09-14 09:15:12 PDT
(In reply to Chris Dumez from comment #10) > (In reply to Eric Carlson from comment #9) > > Comment on attachment 408581 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=408581&action=review > > > > Can we enable or add tests for AudioContextOptions.sampleRate? > > The tests for AudioContextOptions.sampleRate were already passing since we > were lying to the JS and returning the sample rate it expected (even though > it was not the sample rate we used internally. > LOL, why not! > Do you have an idea on how to better test this? > It seems fine as long as we aren't just returning the expected rate.
Note You need to log in before you can comment on or make changes to this bug.