Added Constructor method to OscillatorNode

• Component: Web Audio
• Status: RESOLVED FIXED

[Clark Wang]

Added ctor to OscillatorNode. Added OscillatorOptions, OscillatorType files. Changed createOscillator() methods in BaseAudioContext, WebKitAudioContext.

[Clark Wang]

Created attachment 405163 [details]
Patch

[Darin Adler]

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

[Darin Adler]

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().

[Clark Wang]

Created attachment 405174 [details]
Patch

[Darin Adler]

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.

[Clark Wang]

Created attachment 405185 [details]
Patch

[Darin Adler]

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?

[Clark Wang]

(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.

[Chris Dumez]

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.

[Chris Dumez]

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().

[Chris Dumez]

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.

[Clark Wang]

(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.

[Clark Wang]

(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.

[Chris Dumez]

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.

[Darin Adler]

(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.

[Chris Dumez]

(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.

[Clark Wang]

Created attachment 405288 [details]
Patch

[Chris Dumez]

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?

[Darin Adler]

(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.

[Chris Dumez]

(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.

[Clark Wang]

Created attachment 405298 [details]
Patch

[EWS]

Committed r264941: <https://trac.webkit.org/changeset/264941>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405298 [details].

[Radar WebKit Bug Importer]

<rdar://problem/66179060>