Bug 214615 – Added PeriodicWave constructor according to spec

• Component: Web Audio
• Status: RESOLVED FIXED

[Clark Wang]

Added PeriodicWave constructor according to spec. Added PeriodicWaveConstraints, PeriodicWaveOptions files. Added updated createPeriodicWave method in BaseAudioContext.

[Clark Wang]

Created attachment 404865 [details]
Patch

[Chris Dumez]

Comment on attachment 404865 [details]
Patch

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

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:652
> +ExceptionOr<Ref<PeriodicWave>> BaseAudioContext::createPeriodicWave(Vector<float>& real, Vector<float>& imaginary, const PeriodicWaveConstraints& constraints)

Vector<float>&&

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:663
> +    PeriodicWaveOptions* options = new PeriodicWaveOptions();

This is a memory leak. This should be:
PeriodicWaveOptions options;

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:664
> +    options->real = real;

WTFMove(real)

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:665
> +    options->imag = imaginary;

WTFMove(imaginary)

> Source/WebCore/Modules/webaudio/BaseAudioContext.h:157
>      ExceptionOr<Ref<PeriodicWave>> createPeriodicWave(Float32Array& real, Float32Array& imaginary);

What's this one? We should probably drop it in favor of your new one, no?

> Source/WebCore/Modules/webaudio/BaseAudioContext.h:158
> +    ExceptionOr<Ref<PeriodicWave>> createPeriodicWave(Vector<float>& real, Vector<float>& imaginary, const PeriodicWaveConstraints& = { });

Why add it to the C++ implementation but not the IDL? What's the point?

> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:57
> +ExceptionOr<Ref<PeriodicWave>> PeriodicWave::create(BaseAudioContext& context, const PeriodicWaveOptions& options)

PeriodicWaveOptions&&

> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:62
> +    if (options.real.hasValue()) {

Can we format this as in the spec? I also mentioned this in previous comments but you don't need hasValue() and value(), it results in less concise code.
if (options.real && options.imag) {
    real = WTFMove(*options.real);
    imag = WTFMove(*options.imag);
} else if (options.real) {

} else if ...

> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:72
> +        real.resize(2);

I could be wrong but I don't think this code is safe. I do not believe those 2 vector items will be 0 initialized.
Vector::fill() is your friend here I think.

> Source/WebCore/Modules/webaudio/PeriodicWave.h:99
> +    void createBandLimitedTables(const float* real, const float* imag, unsigned numberOfComponents, bool = false);

We don't like boolean parameters in WebKit because this result is not very readable code like this. We prefer using enum classes:
enum class ShouldDisableNormalization : bool { No, Yes };

> Source/WebCore/Modules/webaudio/PeriodicWave.idl:30
> +    [MayThrowException] constructor(BaseAudioContext context, optional PeriodicWaveOptions options);

You're changing the behavior of the shipping API. This should be behind a flag:
[EnabledBySetting=ModernUnprefixedWebAudio]

> Source/WebCore/Modules/webaudio/PeriodicWaveOptions.h:34
> +struct PeriodicWaveOptions : PeriodicWaveConstraints {

public PeriodicWaveConstraints

[Chris Dumez]

Comment on attachment 404865 [details]
Patch

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

> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:77
> +    if (real.size() != imag.size() || real.size() < 2 || imag.size() < 2)

Can you please do the checks exactly as in the specification and thus provide a more accurate exception message when it makes sense? If the client did not provide a 'real' sequence, only an 'imag' sequence, then no point in saying that both have incorrect length.

[Clark Wang]

Created attachment 404952 [details]
Patch

[Chris Dumez]

Comment on attachment 404952 [details]
Patch

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

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:301
> +unsigned BaseAudioContext::getMaxPeriodicWaveLength() const

WebKit getters never start with "get". Also, I don't think we should have a getter for a constant. MaxPeriodicWaveLength does not seem to be used in this file anymore, can't you simply move MaxPeriodicWaveLength definition to WebKitAudioContext.cpp?

> Source/WebCore/Modules/webaudio/PeriodicWave.h:40
> +enum class ShouldDisableNormalization : bool {

I believe we prefer these on one line.

> Source/WebCore/Modules/webaudio/PeriodicWave.h:42
> +    Yes,

No need for comma.

[Clark Wang]

Created attachment 404965 [details]
Patch

[Clark Wang]

Created attachment 404969 [details]
Patch

[Clark Wang]

Added #include "BaseAudioContext.h" to PeriodicWave.h to hopefully get past build errors

[Chris Dumez]

Comment on attachment 404969 [details]
Patch

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

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

This should have been added to the cpp, not the header. It should remain a forward declaration in the header.

[Clark Wang]

Created attachment 404974 [details]
Patch

[Chris Dumez]

Comment on attachment 404974 [details]
Patch

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

> Source/WebCore/Modules/webaudio/BaseAudioContext.h:38
> +#include "PeriodicWaveOptions.h"

Where is this used in this header?

> Source/WebCore/Modules/webaudio/PeriodicWaveOptions.h:30
> +#include "ExceptionOr.h"

This does not make sense, there is not ExceptionOr in this header...

[Clark Wang]

Created attachment 404982 [details]
Patch

[youenn fablet]

Comment on attachment 404982 [details]
Patch

Some nits below.
It also seems some tests might need new baselines.

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

> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:89
> +    imag[0] = 0;

There are cases where these two lines may not be necessary, should we move them where they are useful.

> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:91
> +    auto waveTable = adoptRef(*new PeriodicWave(context.sampleRate()));

Why not passing sampleRate directly, context does not seem used anywhere else.

> Source/WebCore/Modules/webaudio/PeriodicWave.h:56
> +    static ExceptionOr<Ref<PeriodicWave>> create(BaseAudioContext&, PeriodicWaveOptions&& = { });

Do we need { }.
Could we forward declare PeriodicWaveOptions?

[Clark Wang]

(In reply to youenn fablet from comment #13)
> Comment on attachment 404982 [details]
> Patch
> 
> Some nits below.
> It also seems some tests might need new baselines.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404982&action=review
> 
> > Source/WebCore/Modules/webaudio/PeriodicWave.cpp:89
> > +    imag[0] = 0;
> 
> There are cases where these two lines may not be necessary, should we move
> them where they are useful.
 
Got it, I adjusted this code.

> > Source/WebCore/Modules/webaudio/PeriodicWave.cpp:91
> > +    auto waveTable = adoptRef(*new PeriodicWave(context.sampleRate()));
> 
> Why not passing sampleRate directly, context does not seem used anywhere
> else.
 
Yeah, I agree that context is not used anywhere. I thought I needed to match this constructor spec here: https://www.w3.org/TR/webaudio/#periodicwave, which takes in context as a parameter. 

> > Source/WebCore/Modules/webaudio/PeriodicWave.h:56
> > +    static ExceptionOr<Ref<PeriodicWave>> create(BaseAudioContext&, PeriodicWaveOptions&& = { });
> 
> Do we need { }.
> Could we forward declare PeriodicWaveOptions?

I thought I should pass in { } for options, since according to spec: https://www.w3.org/TR/webaudio/#dom-periodicwave-periodicwave, options is an optional parameter. Though, I think I could move = { } to IDL file, if that is preferred.

[Chris Dumez]

Comment on attachment 404982 [details]
Patch

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

>>> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:89
>>> +    imag[0] = 0;
>> 
>> There are cases where these two lines may not be necessary, should we move them where they are useful.
> 
> Got it, I adjusted this code.

This was matching the spec text so I liked it.

[Chris Dumez]

Comment on attachment 404982 [details]
Patch

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

>>> Source/WebCore/Modules/webaudio/PeriodicWave.cpp:91
>>> +    auto waveTable = adoptRef(*new PeriodicWave(context.sampleRate()));
>> 
>> Why not passing sampleRate directly, context does not seem used anywhere else.
> 
> Yeah, I agree that context is not used anywhere. I thought I needed to match this constructor spec here: https://www.w3.org/TR/webaudio/#periodicwave, which takes in context as a parameter.

Yes, Clark is correct. The factor takes a context in because that is what the IDL says.

>>> Source/WebCore/Modules/webaudio/PeriodicWave.h:56
>>> +    static ExceptionOr<Ref<PeriodicWave>> create(BaseAudioContext&, PeriodicWaveOptions&& = { });
>> 
>> Do we need { }.
>> Could we forward declare PeriodicWaveOptions?
> 
> I thought I should pass in { } for options, since according to spec: https://www.w3.org/TR/webaudio/#dom-periodicwave-periodicwave, options is an optional parameter. Though, I think I could move = { } to IDL file, if that is preferred.

I like it with a default value. No reason you have to specify options and they are indeed optional in the IDL as well.

[Clark Wang]

Created attachment 405043 [details]
Patch

[Clark Wang]

Created attachment 405045 [details]
Patch

[Clark Wang]

Created attachment 405048 [details]
Patch

[EWS]

Committed r264782: <https://trac.webkit.org/changeset/264782>

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

[Radar WebKit Bug Importer]

<rdar://problem/66005752>