[webkit-reviews] review granted: [Bug 75057] Implement MediaElementAudioSourceNode::setFormat() so numberOfChannels and sampleRate are accounted for : [Attachment 120248] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Chris Rogers
<crogers at google.com>'s request for review:
Bug 75057: Implement MediaElementAudioSourceNode::setFormat() so
numberOfChannels and sampleRate are accounted for
https://bugs.webkit.org/show_bug.cgi?id=75057

Attachment 120248: Patch
https://bugs.webkit.org/attachment.cgi?id=120248&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=120248&action=review


> Source/WebCore/platform/audio/MultiChannelResampler.cpp:90
> +    size_t m_framesToProcess; // used to verify that all channels ask for
the same amount

Nit: Comments should use proper sentences (missing capital and period).

> Source/WebCore/platform/audio/MultiChannelResampler.cpp:102
> +	   OwnPtr<SincResampler> resampler = adoptPtr(new
SincResampler(scaleFactor));
> +	   m_kernels.append(resampler.release());

This local variable isn't necessary, it causes ref count churn.

Why doesn't SincResampler use the preferred WebKit idiom of a private
constructor plus a create() method?

> Source/WebCore/platform/audio/MultiChannelResampler.cpp:108
> +    // provider can provide us with multi-channel audio data. But each of
our single-channel resamplers (kernels)

Nit: Capitalize.

> Source/WebCore/platform/audio/MultiChannelResampler.h:48
> +    // FIXME: the mac port can have a more highly optimized implementation
based on CoreAudio
> +    // instead of SincResampler. For now the default implementation will be
used on all ports.

Please include a bug number with the FIXME.

> Source/WebCore/platform/audio/MultiChannelResampler.h:53
> +    double m_scaleFactor;

This is initialized in the constructor but never used in the code. Is it
necessary?

> Source/WebCore/webaudio/MediaElementAudioSourceNode.cpp:70
> +	   // FIXME: implement multi-channel greater than stereo.

Please include a bug number for the FIXME.

> Source/WebCore/webaudio/MediaElementAudioSourceNode.cpp:75
> +	   if (!numberOfChannels || numberOfChannels > 2 || sourceSampleRate <
minSampleRate || sourceSampleRate > maxSampleRate) {
> +	       // process() will generate silence for these uninitialized
values.
> +	       m_sourceNumberOfChannels = 0;
> +	       m_sourceSampleRate = 0;
> +	       return;

It would be helpful to LOG() exactly which condition(s) trigger this so it is
possible to discover why a file generates only silence.