Bug 215377 – Introduce ConstantSourceNode Interface

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Clark Wang]

Introduce missing interface according to spec: https://www.w3.org/TR/webaudio/#ConstantSourceNode

[Clark Wang]

Created attachment 406524 [details]
Patch

[Chris Dumez]

Comment on attachment 406524 [details]
Patch

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

> Source/WebCore/Modules/webaudio/ConstantSourceNode.cpp:2
> + * Copyright (C) 2020 Apple Inc. All rights reserved.

Given that you claim to have used the Chromium implementation as reference. You should also import the copyright line from constant_source_node.cc (assuming it is the same license).

> Source/WebCore/Modules/webaudio/ConstantSourceNode.h:2
> + * Copyright (C) 2020 Apple Inc. All rights reserved.

Given that you claim to have used the Chromium implementation as reference. You should also import the copyright line from constant_source_node.cc (assuming it is the same license).

> Source/WebCore/Modules/webaudio/ConstantSourceNode.h:34
> +class ConstantSourceNode : public AudioScheduledSourceNode {

class can be marked final.

> Source/WebCore/Modules/webaudio/ConstantSourceNode.h:41
> +    AudioParam* offset() { return m_offset.get(); }

Should return a AudioParam&

> Source/WebCore/Modules/webaudio/ConstantSourceNode.h:43
> +    const char* activeDOMObjectName() const override { return "ConstantSourceNode"; }

Can likely be private.

> Source/WebCore/Modules/webaudio/ConstantSourceNode.h:51
> +protected:

This can be private. We don't have any subclasses at the moment.

> Source/WebCore/Modules/webaudio/ConstantSourceNode.h:58
> +    RefPtr<AudioParam> m_offset;

Ref<>

> Source/WebCore/Modules/webaudio/ConstantSourceNode.h:61
> +    mutable Lock m_processMutex;

m_processLock

[Darin Adler]

Comment on attachment 406524 [details]
Patch

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

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:617
> +    ALWAYS_LOG(LOGIDENTIFIER);

Do we really want to land this?

[Chris Dumez]

Comment on attachment 406524 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:617
>> +    ALWAYS_LOG(LOGIDENTIFIER);
> 
> Do we really want to land this?

I believe so, this is the pre-existing pattern for most functions in this class. It merely does os_log of "BaseAudioContext::createConstantSource()" to indicate it was called. I don't think it is super important logging but it is the pre-existing pattern.

[Chris Dumez]

Comment on attachment 406524 [details]
Patch

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

> Source/WebCore/Modules/webaudio/ConstantSourceNode.cpp:54
> +    addOutput(makeUnique<AudioNodeOutput>(this, 1));

Looks like you're missing an include for AudioNodeOutput.

[Clark Wang]

Created attachment 406530 [details]
Patch

[Clark Wang]

Added in suggested changes above, and I also added in new flaky tests to LayoutTests/TestExpectations.

[Clark Wang]

Created attachment 406537 [details]
Patch

[Clark Wang]

(In reply to Clark Wang from comment #8)
> Created attachment 406537 [details]
> Patch

Re-uploaded patch, previous one was faulty.

[Chris Dumez]

Comment on attachment 406537 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioNode.h:92
> +        NodeTypeEnd,

NodeTypeEnd should always be LAST in the enum, per its name :)

[Clark Wang]

Created attachment 406540 [details]
Patch

[Clark Wang]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 406537 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406537&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioNode.h:92
> > +        NodeTypeEnd,
> 
> NodeTypeEnd should always be LAST in the enum, per its name :)

Darn alright, fixed this.

[Chris Dumez]

Comment on attachment 406540 [details]
Patch

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

> Source/WebCore/Modules/webaudio/ConstantSourceNode.cpp:75
> +    std::unique_lock<Lock> lock(m_processLock, std::try_to_lock);

I don't understand the purpose of this Lock since it is not locked anywhere else. I don't believe process() can be called in parallel so locking here does not seem to make sense.

> Source/WebCore/Modules/webaudio/ConstantSourceNode.h:44
> +    void process(size_t framesToProcess) final;

Does this need to be public?

> Source/WebCore/Modules/webaudio/ConstantSourceNode.h:45
> +    void reset() final { };

ditto.

> Source/WebCore/Modules/webaudio/ConstantSourceNode.h:48
> +    bool propagatesSilence() const final;

ditto.

> Source/WebCore/Modules/webaudio/ConstantSourceNode.h:51
> +    explicit ConstantSourceNode(BaseAudioContext&, float);

No need for explicit since there is more than 1 mandatory parameter.

[Clark Wang]

Created attachment 406590 [details]
Patch

[Clark Wang]

(In reply to Chris Dumez from comment #13)
> Comment on attachment 406540 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406540&action=review
> 
> > Source/WebCore/Modules/webaudio/ConstantSourceNode.cpp:75
> > +    std::unique_lock<Lock> lock(m_processLock, std::try_to_lock);
> 
> I don't understand the purpose of this Lock since it is not locked anywhere
> else. I don't believe process() can be called in parallel so locking here
> does not seem to make sense.
> 
> > Source/WebCore/Modules/webaudio/ConstantSourceNode.h:44
> > +    void process(size_t framesToProcess) final;
> 
> Does this need to be public?
> 
> > Source/WebCore/Modules/webaudio/ConstantSourceNode.h:45
> > +    void reset() final { };
> 
> ditto.
> 
> > Source/WebCore/Modules/webaudio/ConstantSourceNode.h:48
> > +    bool propagatesSilence() const final;
> 
> ditto.
> 
> > Source/WebCore/Modules/webaudio/ConstantSourceNode.h:51
> > +    explicit ConstantSourceNode(BaseAudioContext&, float);
> 
> No need for explicit since there is more than 1 mandatory parameter.

Fixed these suggestions. We can add the lock back in, if we need to add a method that synchronizes with process().

[Chris Dumez]

Comment on attachment 406590 [details]
Patch

Debug bots are unhappy and the crashes look related.

[Chris Dumez]

Comment on attachment 406590 [details]
Patch

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

> Source/WebCore/Modules/webaudio/ConstantSourceNode.cpp:48
> +    return adoptRef(*new ConstantSourceNode(context, options.offset));

If I had to guess, I'd say you're likely missing a call to context.refNode(node) here.

[Clark Wang]

Created attachment 406600 [details]
Patch

[Clark Wang]

(In reply to Chris Dumez from comment #17)
> Comment on attachment 406590 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406590&action=review
> 
> > Source/WebCore/Modules/webaudio/ConstantSourceNode.cpp:48
> > +    return adoptRef(*new ConstantSourceNode(context, options.offset));
> 
> If I had to guess, I'd say you're likely missing a call to
> context.refNode(node) here.

Thanks, yes I think that did it :)

[EWS]

Committed r265689: <https://trac.webkit.org/changeset/265689>

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

[Radar WebKit Bug Importer]

<rdar://problem/67087237>

[Chris Dumez]

*** Bug 204340 has been marked as a duplicate of this bug. ***