Introduce missing interface according to spec: https://www.w3.org/TR/webaudio/#ConstantSourceNode
Created attachment 406524 [details] Patch
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
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?
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.
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.
Created attachment 406530 [details] Patch
Added in suggested changes above, and I also added in new flaky tests to LayoutTests/TestExpectations.
Created attachment 406537 [details] Patch
(In reply to Clark Wang from comment #8) > Created attachment 406537 [details] > Patch Re-uploaded patch, previous one was faulty.
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 :)
Created attachment 406540 [details] Patch
(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.
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.
Created attachment 406590 [details] Patch
(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().
Comment on attachment 406590 [details] Patch Debug bots are unhappy and the crashes look related.
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.
Created attachment 406600 [details] Patch
(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 :)
Committed r265689: <https://trac.webkit.org/changeset/265689> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406600 [details].
<rdar://problem/67087237>
*** Bug 204340 has been marked as a duplicate of this bug. ***