Bug 215518

Created attachment 406809 [details] Patch

Comment on attachment 406809 [details] 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?

Comment on attachment 406809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406809&action=review >> 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? automation rate. > Source/WebCore/Modules/webaudio/StereoPannerNode.h:47 > + // AudioNode Can't all these overrides be private? > Source/WebCore/Modules/webaudio/StereoPannerNode.h:72 > + StereoPannerNode(BaseAudioContext&, float); Please move this up, ideally just after the "private:". We don't put functions after data members. Also, don't omit the name of the second parameter because it is really not obvious what it is otherwise. >> 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. I don't think our constants usually start with a capital letter. >> Source/WebCore/platform/audio/StereoPanner.h:41 >> + virtual ~StereoPanner() { }; > > No need for this semicolon. Why is the constructor visual? >> Source/WebCore/platform/graphics/cocoa/SourceBufferParserWebM.cpp:32 >> +#include "ContentType.h" > > This should not be included in this patch? Well actually, we don't have much choice if we want the patch to build... This is an issue with the bundled builds that WebKit does. As soon as you add a new source file, you get a bunch of unrelated build errors you need to fix :(

Comment on attachment 406809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406809&action=review >>> Source/WebCore/platform/audio/StereoPanner.h:41 >>> + virtual ~StereoPanner() { }; >> >> No need for this semicolon. > > Why is the constructor visual? *VIRTUAL*

Comment on attachment 406809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406809&action=review >>>> Source/WebCore/platform/audio/StereoPanner.h:41 >>>> + virtual ~StereoPanner() { }; >>> >>> No need for this semicolon. >> >> Why is the constructor visual? > > *VIRTUAL* Oh, yes this should be omitted entirely. Just leave this line out.

(In reply to Darin Adler from comment #2) > > 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. > I kept the isSampleAccurate code there and added a commented out aRate check. This is because in the first check, ideally once automation-rate is implemented we should check (isSampleAccurate && aRate). Then, in the second check we are setting panValue a certain way if (isSampleAccurate && kRate). I could remove it for now to simplify the code, but once automationRate is implemented it would have to be changed again.

(In reply to Clark Wang from comment #6) > (In reply to Darin Adler from comment #2) > > > 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. > > > > I kept the isSampleAccurate code there and added a commented out aRate > check. This is because in the first check, ideally once automation-rate is > implemented we should check (isSampleAccurate && aRate). Then, in the second > check we are setting panValue a certain way if (isSampleAccurate && kRate). > I could remove it for now to simplify the code, but once automationRate is > implemented it would have to be changed again. I think we should simplify the code. We are not in the habit of keeping dead code around even if it may be useful in the future.

(In reply to Chris Dumez from comment #7) > (In reply to Clark Wang from comment #6) > > (In reply to Darin Adler from comment #2) > > > > 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. > > > > > > > I kept the isSampleAccurate code there and added a commented out aRate > > check. This is because in the first check, ideally once automation-rate is > > implemented we should check (isSampleAccurate && aRate). Then, in the second > > check we are setting panValue a certain way if (isSampleAccurate && kRate). > > I could remove it for now to simplify the code, but once automationRate is > > implemented it would have to be changed again. > > I think we should simplify the code. We are not in the habit of keeping dead > code around even if it may be useful in the future. Sounds good, I will fix this.

(In reply to Darin Adler from comment #2) > > 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. This seems to be how the AudioNode subclasses handle their std::unique_ptr members, so I thought to keep this in order to be consistent with our other webaudio code. > > 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. > Removed all of this actually, since stereoPanner doesn't even seem to need m_smoothingConstant, so I'm just using the default constructor. > > 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? Like Chris said for the preceding include statement, this is needed for the code to compile.

kCMVideoCodecType_VP9 is needed for the code to compile?

(In reply to Clark Wang from comment #9) > This seems to be how the AudioNode subclasses handle their std::unique_ptr > members, so I thought to keep this in order to be consistent with our other > webaudio code. I’m disappointed to hear this. It’s not a good pattern.

(In reply to Darin Adler from comment #10) > kCMVideoCodecType_VP9 is needed for the code to compile? Yes. Clark started getting this error when he added his new cpp file to the project. This is one bad side effect of our bundled builds in WebKit.

(In reply to Chris Dumez from comment #12) > (In reply to Darin Adler from comment #10) > > kCMVideoCodecType_VP9 is needed for the code to compile? > > Yes. Clark started getting this error when he added his new cpp file to the > project. This is one bad side effect of our bundled builds in WebKit. But how does a different unified build grouping require adding a *constant*? That doesn’t make logical sense. Adding an include makes logical sense. I did some research and found the answer: We either need to *move* kCMVideoCodecType_VP9 from MediaEngineConfigurationFactory.cpp and VP9UtilitiesCocoa.mm to a header (not duplicate it), or *duplicate* it in the SourceBufferParserWebM.cpp file. But please don’t duplicate it in a header without removing it.

(In reply to Darin Adler from comment #13) > (In reply to Chris Dumez from comment #12) > > (In reply to Darin Adler from comment #10) > > > kCMVideoCodecType_VP9 is needed for the code to compile? > > > > Yes. Clark started getting this error when he added his new cpp file to the > > project. This is one bad side effect of our bundled builds in WebKit. > > But how does a different unified build grouping require adding a *constant*? > That doesn’t make logical sense. Adding an include makes logical sense. I > did some research and found the answer: > > We either need to *move* kCMVideoCodecType_VP9 from > MediaEngineConfigurationFactory.cpp and VP9UtilitiesCocoa.mm to a header > (not duplicate it), or *duplicate* it in the SourceBufferParserWebM.cpp file. "or duplicate it in SourceBufferParserWebM.cpp file" is what Clark did, no? He applied the same build fix that was done in MediaEngineConfigurationFactory.cpp to VP9UtilitiesCocoa.mm.

(In reply to Chris Dumez from comment #14) > (In reply to Darin Adler from comment #13) > > (In reply to Chris Dumez from comment #12) > > > (In reply to Darin Adler from comment #10) > > > > kCMVideoCodecType_VP9 is needed for the code to compile? > > > > > > Yes. Clark started getting this error when he added his new cpp file to the > > > project. This is one bad side effect of our bundled builds in WebKit. > > > > But how does a different unified build grouping require adding a *constant*? > > That doesn’t make logical sense. Adding an include makes logical sense. I > > did some research and found the answer: > > > > We either need to *move* kCMVideoCodecType_VP9 from > > MediaEngineConfigurationFactory.cpp and VP9UtilitiesCocoa.mm to a header > > (not duplicate it), or *duplicate* it in the SourceBufferParserWebM.cpp file. > > "or duplicate it in SourceBufferParserWebM.cpp file" is what Clark did, no? > He applied the same build fix that was done in > MediaEngineConfigurationFactory.cpp to VP9UtilitiesCocoa.mm. Got it. I thought I was looking at a header. My mistake!

(In reply to Darin Adler from comment #2) > Comment on attachment 406809 [details] > 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. Fixed this. > > 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. > Fixed this. > > Source/WebCore/Modules/webaudio/StereoPannerNode.h:53 > > + // Listener > > What’s the value of this comment? > Removed 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. > Removed this method. > > 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. > Fixed this. > > 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. > Removed this comment. > > 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. > I converted all doubles to float and it doesn't affect any test cases, so seems ok to change. Values may be a little less precise now though. > > 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. > Removed this and moved makeUnique to the call site. > > 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. > Removed this, as discussed previously. > > Source/WebCore/platform/audio/StereoPanner.h:41 > > + virtual ~StereoPanner() { }; > > No need for this semicolon. > Removed this method. > > Source/WebCore/platform/audio/StereoPanner.h:48 > > + double m_smoothingConstant; > > Why double instead of float? > Removed this, as it wasn't used in StereoPanner anyway.

(In reply to Clark Wang from comment #16) > (In reply to Darin Adler from comment #2) > > Comment on attachment 406809 [details] > > > 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. > > > > I converted all doubles to float and it doesn't affect any test cases, so > seems ok to change. Values may be a little less precise now though. > Actually, I changed something that fixed the assertion error, and it appears that some tests fail now when changing all to float. I changed it back to using both double and float.

Created attachment 406870 [details] Patch

Bubbles are red. Looks like there are a couple of tests that are either flaky or print out some platform-specific values.

Created attachment 406886 [details] Patch

(In reply to Chris Dumez from comment #19) > Bubbles are red. Looks like there are a couple of tests that are either > flaky or print out some platform-specific values. Got it, just added those two tests to TestExpectations.

Comment on attachment 406886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406886&action=review > Source/WebCore/Modules/webaudio/StereoPannerNode.h:45 > + virtual ~StereoPannerNode(); No need for virtual here. > Source/WebCore/Modules/webaudio/StereoPannerNode.h:65 > + Why the blank line? > Source/WebCore/Modules/webaudio/StereoPannerNode.h:67 > + Why the blank line? > Source/WebCore/platform/audio/StereoPanner.h:35 > +class StereoPanner { No data members. Looks like this could simply be free functions in a StereoPanner namespace. It does not look like we need to construct a StereoPanner object at all?

Created attachment 406934 [details] Patch

(In reply to Chris Dumez from comment #22) > Comment on attachment 406886 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406886&action=review > > > Source/WebCore/Modules/webaudio/StereoPannerNode.h:45 > > + virtual ~StereoPannerNode(); > > No need for virtual here. > > > Source/WebCore/Modules/webaudio/StereoPannerNode.h:65 > > + > > Why the blank line? > > > Source/WebCore/Modules/webaudio/StereoPannerNode.h:67 > > + > > Why the blank line? > > > Source/WebCore/platform/audio/StereoPanner.h:35 > > +class StereoPanner { > > No data members. Looks like this could simply be free functions in a > StereoPanner namespace. It does not look like we need to construct a > StereoPanner object at all? Fixed these, thanks.

Comment on attachment 406934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406934&action=review > Source/WebCore/Modules/webaudio/StereoPannerNode.cpp:99 > +void StereoPannerNode::initialize() I don't think we need to override at all. > Source/WebCore/Modules/webaudio/StereoPannerNode.cpp:107 > +void StereoPannerNode::uninitialize() I don't think we need to override at all.

Created attachment 406959 [details] Patch

(In reply to Chris Dumez from comment #25) > Comment on attachment 406934 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406934&action=review > > > Source/WebCore/Modules/webaudio/StereoPannerNode.cpp:99 > > +void StereoPannerNode::initialize() > > I don't think we need to override at all. > > > Source/WebCore/Modules/webaudio/StereoPannerNode.cpp:107 > > +void StereoPannerNode::uninitialize() > > I don't think we need to override at all. Fixed this and submitted new patch.

Comment on attachment 406959 [details] Patch Clearing flags on attachment: 406959 Committed r265962: <https://trac.webkit.org/changeset/265962>

All reviewed patches have been landed. Closing bug.

<rdar://problem/67503340>