[webkit-reviews] review granted: [Bug 215518] Introduce StereoPannerNode Interface : [Attachment 406809] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Clark Wang <clark_wang at apple.com>'s
request for review:
Bug 215518: Introduce StereoPannerNode Interface
https://bugs.webkit.org/show_bug.cgi?id=215518

Attachment 406809: Patch

https://bugs.webkit.org/attachment.cgi?id=406809&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 406809
  --> https://bugs.webkit.org/attachment.cgi?id=406809
Patch

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

> Source/WebCore/Modules/webaudio/StereoPannerNode.cpp:75
> +    if (!isInitialized() || !input(0)->isConnected() ||
!m_stereoPanner.get()) {

Should not need to call ".get()" here.

> Source/WebCore/Modules/webaudio/StereoPannerNode.cpp:86
> +    bool isSampleAccurate = m_pan->hasSampleAccurateValues();

This expression should be inside the if statement, no local variable needed.

> Source/WebCore/Modules/webaudio/StereoPannerNode.cpp:88
> +    // FIXME: Fix once automatio-rate is introduced. Some tests are failing
because this

Is this a typo for automatic-rate or is automatio-rate a term of art?

> Source/WebCore/Modules/webaudio/StereoPannerNode.cpp:97
> +    float panValue = isSampleAccurate ? m_pan->finalValue() :
m_pan->value();

Code above already handles the isSampleAccurate case, so we don’t need to check
it again here: we know it’s false. I suggest getting rid of this local
variable.

> Source/WebCore/Modules/webaudio/StereoPannerNode.cpp:115
> +    m_stereoPanner = nullptr;

Seems unnecessary to do this unless we need to guarantee the StereoPanner is
destroyed before calling the base class uninitialize. Destructors already take
care of things like this. I feel like this uninitialized design pattern is
possibly over engineering for a case like this one. Not sure how it’s used
throughout the audio code.

> Source/WebCore/Modules/webaudio/StereoPannerNode.h:51
> +    // AudioNode
> +    void process(size_t framesToProcess) override;
> +    void reset() override { };
> +    void initialize() override;
> +    void uninitialize() override;

These should be "final" rather than "override" in WebKit coding style. Also
consider making them private instead of public.

> Source/WebCore/Modules/webaudio/StereoPannerNode.h:53
> +    // Listener

What’s the value of this comment?

> Source/WebCore/Modules/webaudio/StereoPannerNode.h:54
> +    AudioListener& listener();

What is this function used for? I couldn’t find a caller. Let’s not add it if
it’s not needed.

> Source/WebCore/Modules/webaudio/StereoPannerNode.h:63
> +    double tailTime() const override { return 0; }
> +    double latencyTime() const override { return 0; }

These should be "final" rather than "override" in WebKit coding style.

> Source/WebCore/Modules/webaudio/StereoPannerNode.h:69
> +    // Stores sample-accurate values calculated.

Not sure this comment adds anything beyond the name of the data member.

> Source/WebCore/platform/audio/StereoPanner.cpp:36
> +const float SmoothingTimeConstant = 0.050f;

constexpr

No need to write it as 0.050f; 0.05 would work.

This should be inside the WebCore namespace, not the global namespace.

> Source/WebCore/platform/audio/StereoPanner.cpp:44
> +std::unique_ptr<StereoPanner> StereoPanner::create(float sampleRate)
> +{
> +    std::unique_ptr<StereoPanner> stereoPanner=
makeUnique<StereoPanner>(sampleRate);
> +    return stereoPanner;
> +}

I suggest using makeUnique at the call sites and not defining a create
function.

We define create functions when there is extra work to do beyond the
constructor, like adopting into a Ref<> for a reference counted object, or
functions that need to be called on creation.

> Source/WebCore/platform/audio/StereoPanner.cpp:48
> +    m_smoothingConstant =
AudioUtilities::discreteTimeConstantForSampleRate(SmoothingTimeConstant,
sampleRate);

I suggest using construction syntax instead of assignment syntax.

> Source/WebCore/platform/audio/StereoPanner.cpp:75
> +    double gainL;
> +    double gainR;
> +    double panRadian;

Unclear to me why we mix float and double in this function. Is that needed to
get accurate results? It’s typically more efficient to stay in float the whole
time and also avoids the static_cast<float>.

> Source/WebCore/platform/audio/StereoPanner.cpp:133
> +    float targetPan = clampTo(panValue, -1.0, 1.0);

This converts to double and then back to float, because -1.0 and 1.0 are double
constants.

> Source/WebCore/platform/audio/StereoPanner.cpp:138
> +	   double panRadian = (targetPan * 0.5 + 0.5) * piOverTwoDouble;

Same question about why we convert back and forth between double and float.

> Source/WebCore/platform/audio/StereoPanner.cpp:149
> +	   double panRadian = (targetPan <= 0 ? targetPan + 1 : targetPan) *
piOverTwoDouble;

Same question about why we convert back and forth between double and float.

> Source/WebCore/platform/audio/StereoPanner.h:38
> +    static std::unique_ptr<StereoPanner> create(float);

I suggest omitting this. We can use makeUnique to create an object on the heap
and don’t need a dedicated create function when the constructor is public.

> Source/WebCore/platform/audio/StereoPanner.h:39
> +    explicit StereoPanner(float);

I suggest a name for this argument since it’s not obvious what it is.

> Source/WebCore/platform/audio/StereoPanner.h:41
> +    virtual ~StereoPanner() { };

No need for this semicolon.

> Source/WebCore/platform/audio/StereoPanner.h:48
> +    double m_smoothingConstant;

Why double instead of float?

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:32
> +#include "ContentType.h"

This should not be included in this patch?

> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:55
> +// FIXME: Remove this once kCMVideoCodecType_VP9 is added to
CMFormatDescription.h
> +constexpr CMVideoCodecType kCMVideoCodecType_VP9 { 'vp09' };

This should not be included in this patch?