Add proper support for AudioContextOptions.sampleRate

• Component: Web Audio
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 408574 [details]
Patch

[Chris Dumez]

Created attachment 408576 [details]
Patch

[Chris Dumez]

Created attachment 408577 [details]
Patch

[Chris Dumez]

Created attachment 408578 [details]
Patch

[Chris Dumez]

Created attachment 408579 [details]
Patch

[Chris Dumez]

Created attachment 408580 [details]
Patch

[Chris Dumez]

Created attachment 408581 [details]
Patch

[Peng Liu]

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 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]

(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]

Created attachment 408706 [details]
Patch

[Chris Dumez]

Comment on attachment 408706 [details]
Patch

Clearing flags on attachment: 408706

Committed r267014: <https://trac.webkit.org/changeset/267014>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/68856997>

[Eric Carlson]

(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.