Bug 214746 - Added Constructor method to OscillatorNode
Summary: Added Constructor method to OscillatorNode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Clark Wang
URL:
Keywords: InRadar
Depends on:
Blocks: 212611
  Show dependency treegraph
 
Reported: 2020-07-24 10:18 PDT by Clark Wang
Modified: 2020-07-27 13:26 PDT (History)
14 users (show)

See Also:


Attachments
Patch (67.00 KB, patch)
2020-07-24 10:32 PDT, Clark Wang
no flags Details | Formatted Diff | Diff
Patch (68.17 KB, patch)
2020-07-24 14:29 PDT, Clark Wang
no flags Details | Formatted Diff | Diff
Patch (68.17 KB, patch)
2020-07-24 14:46 PDT, Clark Wang
no flags Details | Formatted Diff | Diff
Patch (66.01 KB, patch)
2020-07-27 10:31 PDT, Clark Wang
no flags Details | Formatted Diff | Diff
Patch (66.02 KB, patch)
2020-07-27 11:25 PDT, Clark Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Clark Wang 2020-07-24 10:18:23 PDT
Added ctor to OscillatorNode. Added OscillatorOptions, OscillatorType files. Changed createOscillator() methods in BaseAudioContext, WebKitAudioContext.
Comment 1 Clark Wang 2020-07-24 10:32:18 PDT
Created attachment 405163 [details]
Patch
Comment 2 Darin Adler 2020-07-24 11:10:06 PDT
Comment on attachment 405163 [details]
Patch

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

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:634
> -    refNode(node);
> +    refNode(node.releaseReturnValue());

This change is incorrect. We can’t return node *after* calling releaseReturnValue on it. We need to put the return value into a separate variable and return that, not "node", because the value has been moved out of "node". That’s what the word "release" means in "releaseReturnValue".

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:48
> +// FIXME: Remove once old constructor is phased out.

I’m not sure these comments are helpful. Yes, we should remove anything unused, but we don’t need to add comments next to each thing that we plan to remove. Unless there is some scenario where we think this is particularly helpful? It’s not even obvious what "old constructor" refers to, unless you are already expert.

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:76
> +// FIXME: Remove once old constructor is phased out.

Ditto.

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:103
> +    , m_detune(AudioParam::create(context, "detune", options.detune, -1200*log2(FLT_MAX), 1200*log2(FLT_MAX)))

WebKit project formatting adds spaces around "*".

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:107
> +    , m_firstRender(true)
> +    , m_virtualReadIndex(0)
> +    , m_phaseIncrements(AudioNode::ProcessingSizeInFrames)
> +    , m_detuneValues(AudioNode::ProcessingSizeInFrames)

These should be done in the class definition instead of in multiple constructors.

> Source/WebCore/Modules/webaudio/OscillatorNode.h:41
> +    // FIXME: Remove once old constructor is phased out.

Ditto.

> Source/WebCore/Modules/webaudio/OscillatorNode.idl:25
>   */
> -
> -enum OscillatorType {
> -    "sine",
> -    "square",
> -    "sawtooth",
> -    "triangle",
> -    "custom"
> -};
> -
>  [

Please leave a blank line.

> Source/WebCore/Modules/webaudio/OscillatorOptions.h:37
> +struct OscillatorOptions : public AudioNodeOptions {

No need for "public" with a struct.

> Source/WebCore/Modules/webaudio/OscillatorOptions.h:41
> +    RefPtr<PeriodicWave> periodicWave;

Is it correct that this is silently ignored for types other than custom?

> Source/WebCore/Modules/webaudio/OscillatorType.h:30
> +enum class OscillatorType {

enum class OscillatorType : uint8_t

> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:48
> +// FIXME: Remove once old constructor is phased out.

I suggest removing these comments. Of course we should remove things once they are unused.

> Source/WebCore/Modules/webaudio/WebKitAudioContext.cpp:184
> +    Ref<OscillatorNode> node = OscillatorNode::create(*this, sampleRate());

auto
Comment 3 Darin Adler 2020-07-24 11:12:52 PDT
Comment on attachment 405163 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:103
>> +    , m_detune(AudioParam::create(context, "detune", options.detune, -1200*log2(FLT_MAX), 1200*log2(FLT_MAX)))
> 
> WebKit project formatting adds spaces around "*".

Why is FLT_MAX being used when the type here is a double? Is that the correct value? Not DBL_MAX? Or since this is C++ code, std::numeric_limits<double>::max().
Comment 4 Clark Wang 2020-07-24 14:29:34 PDT
Created attachment 405174 [details]
Patch
Comment 5 Darin Adler 2020-07-24 14:38:36 PDT
Comment on attachment 405174 [details]
Patch

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

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:629
> +    auto nodeValue = node.releaseReturnValue();

This has to be done after the hasException check. It will fail if there’s an exception.
Comment 6 Clark Wang 2020-07-24 14:46:59 PDT
Created attachment 405185 [details]
Patch
Comment 7 Darin Adler 2020-07-24 14:51:24 PDT
Comment on attachment 405174 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:629
>> +    auto nodeValue = node.releaseReturnValue();
> 
> This has to be done after the hasException check. It will fail if there’s an exception.

We need to add a test case where this is an exception. That test would fail on a debug build at least.

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:97
> +    , m_detune(AudioParam::create(context, "detune", options.detune, -1200 * log2f(FLT_MAX), 1200 * log2f(FLT_MAX)))

Still not sure I understand the use of float here. The AudioParam arguments are double, so the float will be converted to a double. Why are these the correct values? Is there a test that verifies these are correct and match other browsers?

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:100
> +    setType(options.type, options.periodicWave.get());

I think this won’t work correctly for OscillatorType::Custom because the setType function is going to check m_type and see that it’s wrong and return early. Do we have a test case that covers this? Is it passing? If so, how?
Comment 8 Clark Wang 2020-07-24 15:02:27 PDT
(In reply to Darin Adler from comment #7)
> Comment on attachment 405174 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405174&action=review
> 
> >> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:629
> >> +    auto nodeValue = node.releaseReturnValue();
> > 
> > This has to be done after the hasException check. It will fail if there’s an exception.
> 
> We need to add a test case where this is an exception. That test would fail
> on a debug build at least.

Got it, I'll work on a test case for this.

> 
> > Source/WebCore/Modules/webaudio/OscillatorNode.cpp:97
> > +    , m_detune(AudioParam::create(context, "detune", options.detune, -1200 * log2f(FLT_MAX), 1200 * log2f(FLT_MAX)))
> 
> Still not sure I understand the use of float here. The AudioParam arguments
> are double, so the float will be converted to a double. Why are these the
> correct values? Is there a test that verifies these are correct and match
> other browsers?
> 

According to this spec, https://www.w3.org/TR/webaudio/#dom-biquadfilternode-detune, that's what the value should be. Though I suppose I could use the approximate value, if that's better. I also think that AudioParam::create may need to be updated to be float for minValue, maxValue according to https://www.w3.org/TR/webaudio/#AudioParam. I will also incorporate tests, and look into how other browsers are doing this.

> > Source/WebCore/Modules/webaudio/OscillatorNode.cpp:100
> > +    setType(options.type, options.periodicWave.get());
> 
> I think this won’t work correctly for OscillatorType::Custom because the
> setType function is going to check m_type and see that it’s wrong and return
> early. Do we have a test case that covers this? Is it passing? If so, how?

I will see if there are any test cases for this one as well, thanks.
Comment 9 Chris Dumez 2020-07-24 15:24:57 PDT
Comment on attachment 405185 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by Darin Adler.

Unless I missed something, Darin did not r+ (quite the contrary) so this is not right.
Comment 10 Chris Dumez 2020-07-24 15:45:45 PDT
Comment on attachment 405185 [details]
Patch

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

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:94
> +OscillatorNode::OscillatorNode(BaseAudioContext& context, const OscillatorOptions& options)

Is there a reason we cannot have a single constructor?

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:96
> +    , m_frequency(AudioParam::create(context, "frequency", options.frequency, -context.sampleRate() / 2, context.sampleRate() / 2))

"frequency"_s

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:97
> +    , m_detune(AudioParam::create(context, "detune", options.detune, -1200 * log2f(FLT_MAX), 1200 * log2f(FLT_MAX)))

"detune"_s

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:110
> +ExceptionOr<void> OscillatorNode::setType(OscillatorType type, PeriodicWave* periodicWave)

From the spec:
"If periodicWave is specified, then any valid value for type is ignored; it is treated as if it were set to "custom".

Where is this implemented? Probably, the constructor should just call setPeriodicWave() if the dictionary contains a periodicWave, instead of calling setType().
Comment 11 Chris Dumez 2020-07-24 15:56:13 PDT
Comment on attachment 405185 [details]
Patch

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

> Source/WebCore/Modules/webaudio/OscillatorNode.idl:-47
> -    readonly attribute unsigned short playbackState;

Why did you drop this? This is not a backward compatible change. If you need to drop things because they are not standard, then you'll need to introduce a WebKitOscillatorNode interface to maintain backward compatibility.

Note also that this bug is about adding a constructor so this change seems unexpected.

> Source/WebCore/Modules/webaudio/PannerOptions.h:36
> +struct PannerOptions : public AudioNodeOptions {

I think Darin asked to drop the public.
Comment 12 Clark Wang 2020-07-27 08:59:22 PDT
(In reply to Chris Dumez from comment #10)
> Comment on attachment 405185 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405185&action=review
> 
> > Source/WebCore/Modules/webaudio/OscillatorNode.cpp:94
> > +OscillatorNode::OscillatorNode(BaseAudioContext& context, const OscillatorOptions& options)
> 
> Is there a reason we cannot have a single constructor?

The only dependency on the old constructor was the old createOscillator() I moved into WebKitAudioContext, but the sampleRate passed in is from context() anyway. I removed the old constructor and reverted createOscillator() to depend on BaseAudioContext's implementation. Hopefully that is backward compatible.
 
> > Source/WebCore/Modules/webaudio/OscillatorNode.cpp:96
> > +    , m_frequency(AudioParam::create(context, "frequency", options.frequency, -context.sampleRate() / 2, context.sampleRate() / 2))
> 
> "frequency"_s
> 
> > Source/WebCore/Modules/webaudio/OscillatorNode.cpp:97
> > +    , m_detune(AudioParam::create(context, "detune", options.detune, -1200 * log2f(FLT_MAX), 1200 * log2f(FLT_MAX)))
> 
> "detune"_s
> 
> > Source/WebCore/Modules/webaudio/OscillatorNode.cpp:110
> > +ExceptionOr<void> OscillatorNode::setType(OscillatorType type, PeriodicWave* periodicWave)
> 
> From the spec:
> "If periodicWave is specified, then any valid value for type is ignored; it
> is treated as if it were set to "custom".
> 
> Where is this implemented? Probably, the constructor should just call
> setPeriodicWave() if the dictionary contains a periodicWave, instead of
> calling setType().

That makes sense, I implemented this code now.
Comment 13 Clark Wang 2020-07-27 09:00:31 PDT
(In reply to Clark Wang from comment #8)
> (In reply to Darin Adler from comment #7)
> > Comment on attachment 405174 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=405174&action=review
> > 
> > >> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:629
> > >> +    auto nodeValue = node.releaseReturnValue();
> > > 
> > > This has to be done after the hasException check. It will fail if there’s an exception.
> > 
> > We need to add a test case where this is an exception. That test would fail
> > on a debug build at least.
> 
> Got it, I'll work on a test case for this.
> 
> > 
> > > Source/WebCore/Modules/webaudio/OscillatorNode.cpp:97
> > > +    , m_detune(AudioParam::create(context, "detune", options.detune, -1200 * log2f(FLT_MAX), 1200 * log2f(FLT_MAX)))
> > 
> > Still not sure I understand the use of float here. The AudioParam arguments
> > are double, so the float will be converted to a double. Why are these the
> > correct values? Is there a test that verifies these are correct and match
> > other browsers?
> > 
> 
> According to this spec,
> https://www.w3.org/TR/webaudio/#dom-biquadfilternode-detune, that's what the
> value should be. Though I suppose I could use the approximate value, if
> that's better. I also think that AudioParam::create may need to be updated
> to be float for minValue, maxValue according to
> https://www.w3.org/TR/webaudio/#AudioParam. I will also incorporate tests,
> and look into how other browsers are doing this.
> 
> > > Source/WebCore/Modules/webaudio/OscillatorNode.cpp:100
> > > +    setType(options.type, options.periodicWave.get());
> > 
> > I think this won’t work correctly for OscillatorType::Custom because the
> > setType function is going to check m_type and see that it’s wrong and return
> > early. Do we have a test case that covers this? Is it passing? If so, how?
> 
> I will see if there are any test cases for this one as well, thanks.

Imported tests did have something covering this, but my previous code was ignoring the exception being thrown. With Chris' change, the test is correctly hit now and passes.
Comment 14 Chris Dumez 2020-07-27 09:54:39 PDT
Comment on attachment 405163 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:634
>> +    refNode(node.releaseReturnValue());
> 
> This change is incorrect. We can’t return node *after* calling releaseReturnValue on it. We need to put the return value into a separate variable and return that, not "node", because the value has been moved out of "node". That’s what the word "release" means in "releaseReturnValue".

FYI, we have test coverage for this and the tests were passing. The reason is that releaseReturnValue() does not actually release anything, it merely does a WTFMove(), which is a cast. No move constructor was getting called here so the value was not actually getting moved out ("released"). This was wrong and dangerous code but turns out it was working, which is why the tests were passing.
Comment 15 Darin Adler 2020-07-27 10:20:03 PDT
(In reply to Chris Dumez from comment #14)
> FYI, we have test coverage for this and the tests were passing. The reason
> is that releaseReturnValue() does not actually release anything, it merely
> does a WTFMove(), which is a cast. No move constructor was getting called
> here so the value was not actually getting moved out ("released"). This was
> wrong and dangerous code but turns out it was working, which is why the
> tests were passing.

We could deal with this by adding code to ExceptionOr to assert in debug builds. A benefit of using ExceptionOr instead of using Expected directly.
Comment 16 Chris Dumez 2020-07-27 10:26:19 PDT
(In reply to Darin Adler from comment #15)
> (In reply to Chris Dumez from comment #14)
> > FYI, we have test coverage for this and the tests were passing. The reason
> > is that releaseReturnValue() does not actually release anything, it merely
> > does a WTFMove(), which is a cast. No move constructor was getting called
> > here so the value was not actually getting moved out ("released"). This was
> > wrong and dangerous code but turns out it was working, which is why the
> > tests were passing.
> 
> We could deal with this by adding code to ExceptionOr to assert in debug
> builds. A benefit of using ExceptionOr instead of using Expected directly.

In my opinion, we should rewrite:
template<typename ReturnType> inline ReturnType&& ExceptionOr<ReturnType>::releaseReturnValue()
{
    return WTFMove(m_value.value());
}

to be:
template<typename ReturnType> inline ReturnType ExceptionOr<ReturnType>::releaseReturnValue()
{
    return WTFMove(m_value.value());
}

I believe this is how we usually write our "release*" functions so that they actually release no matter what the caller does.
Comment 17 Clark Wang 2020-07-27 10:31:00 PDT
Created attachment 405288 [details]
Patch
Comment 18 Chris Dumez 2020-07-27 10:37:12 PDT
Comment on attachment 405288 [details]
Patch

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

> Source/WebCore/Modules/webaudio/OscillatorNode.cpp:-70
> -    // An oscillator is always mono.

I would keep this comment.

> Source/WebCore/Modules/webaudio/OscillatorNode.h:56
> +    OscillatorNode(BaseAudioContext&, const OscillatorOptions& = { });

explicit

> Source/WebCore/Modules/webaudio/OscillatorOptions.h:32
> +#include "PeriodicWave.h"

Can this be forward-declared?
Comment 19 Darin Adler 2020-07-27 10:40:06 PDT
(In reply to Chris Dumez from comment #16)
> In my opinion, we should rewrite:
> template<typename ReturnType> inline ReturnType&&
> ExceptionOr<ReturnType>::releaseReturnValue()
> {
>     return WTFMove(m_value.value());
> }
> 
> to be:
> template<typename ReturnType> inline ReturnType
> ExceptionOr<ReturnType>::releaseReturnValue()
> {
>     return WTFMove(m_value.value());
> }
> 
> I believe this is how we usually write our "release*" functions so that they
> actually release no matter what the caller does.

Agreed.
Comment 20 Chris Dumez 2020-07-27 10:42:02 PDT
(In reply to Darin Adler from comment #19)
> (In reply to Chris Dumez from comment #16)
> > In my opinion, we should rewrite:
> > template<typename ReturnType> inline ReturnType&&
> > ExceptionOr<ReturnType>::releaseReturnValue()
> > {
> >     return WTFMove(m_value.value());
> > }
> > 
> > to be:
> > template<typename ReturnType> inline ReturnType
> > ExceptionOr<ReturnType>::releaseReturnValue()
> > {
> >     return WTFMove(m_value.value());
> > }
> > 
> > I believe this is how we usually write our "release*" functions so that they
> > actually release no matter what the caller does.
> 
> Agreed.

Ok. I will prepare a separate patch for this then.
Comment 21 Clark Wang 2020-07-27 11:25:09 PDT
Created attachment 405298 [details]
Patch
Comment 22 EWS 2020-07-27 13:25:09 PDT
Committed r264941: <https://trac.webkit.org/changeset/264941>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405298 [details].
Comment 23 Radar WebKit Bug Importer 2020-07-27 13:26:18 PDT
<rdar://problem/66179060>