WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
215518
Introduce StereoPannerNode Interface
https://bugs.webkit.org/show_bug.cgi?id=215518
Summary
Introduce StereoPannerNode Interface
Clark Wang
Reported
2020-08-14 11:30:28 PDT
Introduce StereoPannerNode interface, according to spec:
https://www.w3.org/TR/webaudio/#stereopannernode
.
Attachments
Patch
(84.58 KB, patch)
2020-08-18 14:31 PDT
,
Clark Wang
no flags
Details
Formatted Diff
Diff
Patch
(87.85 KB, patch)
2020-08-19 13:31 PDT
,
Clark Wang
no flags
Details
Formatted Diff
Diff
Patch
(88.98 KB, patch)
2020-08-19 16:26 PDT
,
Clark Wang
no flags
Details
Formatted Diff
Diff
Patch
(88.77 KB, patch)
2020-08-20 08:53 PDT
,
Clark Wang
no flags
Details
Formatted Diff
Diff
Patch
(88.46 KB, patch)
2020-08-20 12:59 PDT
,
Clark Wang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Clark Wang
Comment 1
2020-08-18 14:31:04 PDT
Created
attachment 406809
[details]
Patch
Darin Adler
Comment 2
2020-08-18 14:57:28 PDT
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?
Chris Dumez
Comment 3
2020-08-18 15:09:22 PDT
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 :(
Chris Dumez
Comment 4
2020-08-18 15:09:54 PDT
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*
Darin Adler
Comment 5
2020-08-18 15:12:24 PDT
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.
Clark Wang
Comment 6
2020-08-19 08:14:51 PDT
(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.
Chris Dumez
Comment 7
2020-08-19 08:56:06 PDT
(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.
Clark Wang
Comment 8
2020-08-19 09:16:11 PDT
(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.
Clark Wang
Comment 9
2020-08-19 09:22:21 PDT
(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.
Darin Adler
Comment 10
2020-08-19 09:26:10 PDT
kCMVideoCodecType_VP9 is needed for the code to compile?
Darin Adler
Comment 11
2020-08-19 09:27:13 PDT
(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.
Chris Dumez
Comment 12
2020-08-19 09:32:33 PDT
(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.
Darin Adler
Comment 13
2020-08-19 09:38:47 PDT
(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.
Chris Dumez
Comment 14
2020-08-19 09:42:52 PDT
(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.
Darin Adler
Comment 15
2020-08-19 10:03:57 PDT
(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!
Clark Wang
Comment 16
2020-08-19 10:05:45 PDT
(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.
Clark Wang
Comment 17
2020-08-19 13:22:33 PDT
(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.
Clark Wang
Comment 18
2020-08-19 13:31:27 PDT
Created
attachment 406870
[details]
Patch
Chris Dumez
Comment 19
2020-08-19 15:19:29 PDT
Bubbles are red. Looks like there are a couple of tests that are either flaky or print out some platform-specific values.
Clark Wang
Comment 20
2020-08-19 16:26:36 PDT
Created
attachment 406886
[details]
Patch
Clark Wang
Comment 21
2020-08-19 16:26:57 PDT
(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.
Chris Dumez
Comment 22
2020-08-19 16:39:43 PDT
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?
Clark Wang
Comment 23
2020-08-20 08:53:07 PDT
Created
attachment 406934
[details]
Patch
Clark Wang
Comment 24
2020-08-20 08:53:40 PDT
(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.
Chris Dumez
Comment 25
2020-08-20 10:12:35 PDT
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.
Clark Wang
Comment 26
2020-08-20 12:59:59 PDT
Created
attachment 406959
[details]
Patch
Clark Wang
Comment 27
2020-08-20 13:00:32 PDT
(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.
Chris Dumez
Comment 28
2020-08-20 13:44:32 PDT
Comment on
attachment 406959
[details]
Patch Clearing flags on attachment: 406959 Committed
r265962
: <
https://trac.webkit.org/changeset/265962
>
Chris Dumez
Comment 29
2020-08-20 13:44:34 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 30
2020-08-20 13:45:17 PDT
<
rdar://problem/67503340
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug