Bug 218045 – Add addOutput() / removeOutput() utility functions to AudioSummingJunction

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

[Chris Dumez]

Add addOutput() / removeOutput() utility functions to AudioSummingJunction to add or remove outputs from m_outputs and abstract away the call to changedOutputs(). It was awkward that subclasses were modifying m_outputs directly and had to explicitly call changedOutputs() whenever they did.

[Chris Dumez]

Created attachment 412018 [details]
Patch

[Eric Carlson]

Comment on attachment 412018 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioNodeInput.cpp:60
> +    auto addPotentiallyDisabledOutput = [this](AudioNodeOutput& output) {
> +        if (output.isEnabled())
> +            return addOutput(output);
> +        return m_disabledOutputs.add(&output).isNewEntry;
> +    };

Is it not necessary to mark rendering state as dirty when a disabled output is connected?

[Chris Dumez]

Comment on attachment 412018 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/AudioNodeInput.cpp:60
>> +    };
> 
> Is it not necessary to mark rendering state as dirty when a disabled output is connected?

No, it is only necessary when m_output changes. We were calling it unnecessarily in this case previously. You can see in this file that m_disabledOutputs gets changed in several places without calling changedOutputs().

[EWS]

Committed r268820: <https://trac.webkit.org/changeset/268820>

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

[Radar WebKit Bug Importer]

<rdar://problem/70542842>

[Sam Weinig]

Comment on attachment 412018 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioSummingJunction.cpp:97
> +    std::for_each(m_outputs.begin(), m_outputs.end(), [&](auto* output) {

Since this is a little non-standard for WebKit, what made you use std::for_each here rather than a range-for loop? e.g.

for (auto output : m_outputs)
    maxChannels = std::max(maxChannels, output->numberOfChannels());

[Chris Dumez]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 412018 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412018&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioSummingJunction.cpp:97
> > +    std::for_each(m_outputs.begin(), m_outputs.end(), [&](auto* output) {
> 
> Since this is a little non-standard for WebKit, what made you use
> std::for_each here rather than a range-for loop? e.g.
> 
> for (auto output : m_outputs)
>     maxChannels = std::max(maxChannels, output->numberOfChannels());

It actually used to be such a for-loop before I moved the code. This was my attempt at modernizing the code. Do you prefer the for loop?

I believe we are using STL algorithm more and more. Here are some examples:
Source/WebCore/accessibility/AXObjectCache.cpp:    std::for_each(m_deferredFocusedNodeChange.begin(), m_deferredFocusedNodeChange.end(), [&node](auto& entry) {
Source/WebCore/platform/graphics/ImageSource.cpp:    std::for_each(m_frameCommitQueue.begin(), m_frameCommitQueue.end(), [this](const ImageFrameRequest& frameRequest) {
Source/WebCore/Modules/encryptedmedia/MediaKeyStatusMap.cpp:    return std::any_of(statuses.begin(), statuses.end(),
Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:    return std::any_of(m_sessions.begin(), m_sessions.end(),
Source/WebCore/Modules/mediasource/MediaSource.cpp:    if (std::any_of(m_sourceBuffers->begin(), m_sourceBuffers->end(), [](auto& sourceBuffer) { return sourceBuffer->updating(); }))
Source/WebCore/accessibility/AccessibilityRenderObject.cpp:        if (std::any_of(parentTags->begin(), parentTags->end(), [&name] (auto* possibleName) { return *possibleName == name; }))
Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:        if (std::any_of(exponent.begin(), exponent.end() - 4, [](uint8_t element) { return !!element; }))
Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:    return std::any_of(types.begin(), types.end(),
Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.mm:    return std::any_of(m_activeSourceBuffers.begin(), m_activeSourceBuffers.end(), MediaSourcePrivateAVFObjCHasAudio);
Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.mm:    return std::any_of(m_activeSourceBuffers.begin(), m_activeSourceBuffers.end(), [] (SourceBufferPrivateAVFObjC* sourceBuffer) {
Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.mm:    return std::any_of(m_activeSourceBuffers.begin(), m_activeSourceBuffers.end(), [] (SourceBufferPrivateAVFObjC* sourceBuffer) {
Source/WebCore/platform/graphics/nicosia/NicosiaAnimation.cpp:    return std::any_of(m_animations.begin(), m_animations.end(),
Source/WebCore/platform/graphics/nicosia/NicosiaAnimation.cpp:    return std::any_of(m_animations.begin(), m_animations.end(),
Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:            ASSERT(std::any_of(contextDataMap().begin(), contextDataMap().end(),
Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:    return std::any_of(m_children.begin(), m_children.end(),
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:    bool alreadyRecorded = std::any_of(rects.begin(), rects.end(),
Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:    return std::any_of(devices.begin(), devices.end(), [&deviceID, deviceType](auto& device) {
Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp:    return std::any_of(m_activeSourceBuffers.begin(), m_activeSourceBuffers.end(), MockSourceBufferPrivateHasAudio);
Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp:    return std::any_of(m_activeSourceBuffers.begin(), m_activeSourceBuffers.end(), MockSourceBufferPrivateHasVideo);

[Sam Weinig]

(In reply to Chris Dumez from comment #7)
> (In reply to Sam Weinig from comment #6)
> > Comment on attachment 412018 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=412018&action=review
> > 
> > > Source/WebCore/Modules/webaudio/AudioSummingJunction.cpp:97
> > > +    std::for_each(m_outputs.begin(), m_outputs.end(), [&](auto* output) {
> > 
> > Since this is a little non-standard for WebKit, what made you use
> > std::for_each here rather than a range-for loop? e.g.
> > 
> > for (auto output : m_outputs)
> >     maxChannels = std::max(maxChannels, output->numberOfChannels());
> 
> It actually used to be such a for-loop before I moved the code. This was my
> attempt at modernizing the code. Do you prefer the for loop?
> 
> I believe we are using STL algorithm more and more. Here are some examples:
> Source/WebCore/accessibility/AXObjectCache.cpp:   
> std::for_each(m_deferredFocusedNodeChange.begin(),
> m_deferredFocusedNodeChange.end(), [&node](auto& entry) {
> Source/WebCore/platform/graphics/ImageSource.cpp:   
> std::for_each(m_frameCommitQueue.begin(), m_frameCommitQueue.end(),
> [this](const ImageFrameRequest& frameRequest) {
> Source/WebCore/Modules/encryptedmedia/MediaKeyStatusMap.cpp:    return
> std::any_of(statuses.begin(), statuses.end(),
> Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:    return
> std::any_of(m_sessions.begin(), m_sessions.end(),
> Source/WebCore/Modules/mediasource/MediaSource.cpp:    if
> (std::any_of(m_sourceBuffers->begin(), m_sourceBuffers->end(), [](auto&
> sourceBuffer) { return sourceBuffer->updating(); }))
> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:        if
> (std::any_of(parentTags->begin(), parentTags->end(), [&name] (auto*
> possibleName) { return *possibleName == name; }))
> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:        if
> (std::any_of(exponent.begin(), exponent.end() - 4, [](uint8_t element) {
> return !!element; }))
> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:    return
> std::any_of(types.begin(), types.end(),
> Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.
> mm:    return std::any_of(m_activeSourceBuffers.begin(),
> m_activeSourceBuffers.end(), MediaSourcePrivateAVFObjCHasAudio);
> Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.
> mm:    return std::any_of(m_activeSourceBuffers.begin(),
> m_activeSourceBuffers.end(), [] (SourceBufferPrivateAVFObjC* sourceBuffer) {
> Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.
> mm:    return std::any_of(m_activeSourceBuffers.begin(),
> m_activeSourceBuffers.end(), [] (SourceBufferPrivateAVFObjC* sourceBuffer) {
> Source/WebCore/platform/graphics/nicosia/NicosiaAnimation.cpp:    return
> std::any_of(m_animations.begin(), m_animations.end(),
> Source/WebCore/platform/graphics/nicosia/NicosiaAnimation.cpp:    return
> std::any_of(m_animations.begin(), m_animations.end(),
> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:           
> ASSERT(std::any_of(contextDataMap().begin(), contextDataMap().end(),
> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:    return
> std::any_of(m_children.begin(), m_children.end(),
> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.
> cpp:    bool alreadyRecorded = std::any_of(rects.begin(), rects.end(),
> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:  
> return std::any_of(devices.begin(), devices.end(), [&deviceID,
> deviceType](auto& device) {
> Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp:   
> return std::any_of(m_activeSourceBuffers.begin(),
> m_activeSourceBuffers.end(), MockSourceBufferPrivateHasAudio);
> Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp:   
> return std::any_of(m_activeSourceBuffers.begin(),
> m_activeSourceBuffers.end(), MockSourceBufferPrivateHasVideo);

Personally, I like the use of the c++ algorithm functions in cases where they make it more clear what is happening than a range-for loop. That includes things like std::any_of and std::fill_n (and will like them more when we have c++20 ranges, and don't have to pass in explicit begin/end iterators). But in the case of that a range-for loop is identical or shorter, I think we should stick with it. Perhaps something to re-evaluate when we adopt c++20 ranges and can pipeline things.

[Chris Dumez]

(In reply to Sam Weinig from comment #8)
> (In reply to Chris Dumez from comment #7)
> > (In reply to Sam Weinig from comment #6)
> > > Comment on attachment 412018 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=412018&action=review
> > > 
> > > > Source/WebCore/Modules/webaudio/AudioSummingJunction.cpp:97
> > > > +    std::for_each(m_outputs.begin(), m_outputs.end(), [&](auto* output) {
> > > 
> > > Since this is a little non-standard for WebKit, what made you use
> > > std::for_each here rather than a range-for loop? e.g.
> > > 
> > > for (auto output : m_outputs)
> > >     maxChannels = std::max(maxChannels, output->numberOfChannels());
> > 
> > It actually used to be such a for-loop before I moved the code. This was my
> > attempt at modernizing the code. Do you prefer the for loop?
> > 
> > I believe we are using STL algorithm more and more. Here are some examples:
> > Source/WebCore/accessibility/AXObjectCache.cpp:   
> > std::for_each(m_deferredFocusedNodeChange.begin(),
> > m_deferredFocusedNodeChange.end(), [&node](auto& entry) {
> > Source/WebCore/platform/graphics/ImageSource.cpp:   
> > std::for_each(m_frameCommitQueue.begin(), m_frameCommitQueue.end(),
> > [this](const ImageFrameRequest& frameRequest) {
> > Source/WebCore/Modules/encryptedmedia/MediaKeyStatusMap.cpp:    return
> > std::any_of(statuses.begin(), statuses.end(),
> > Source/WebCore/Modules/encryptedmedia/MediaKeys.cpp:    return
> > std::any_of(m_sessions.begin(), m_sessions.end(),
> > Source/WebCore/Modules/mediasource/MediaSource.cpp:    if
> > (std::any_of(m_sourceBuffers->begin(), m_sourceBuffers->end(), [](auto&
> > sourceBuffer) { return sourceBuffer->updating(); }))
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:        if
> > (std::any_of(parentTags->begin(), parentTags->end(), [&name] (auto*
> > possibleName) { return *possibleName == name; }))
> > Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:        if
> > (std::any_of(exponent.begin(), exponent.end() - 4, [](uint8_t element) {
> > return !!element; }))
> > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:    return
> > std::any_of(types.begin(), types.end(),
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.
> > mm:    return std::any_of(m_activeSourceBuffers.begin(),
> > m_activeSourceBuffers.end(), MediaSourcePrivateAVFObjCHasAudio);
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.
> > mm:    return std::any_of(m_activeSourceBuffers.begin(),
> > m_activeSourceBuffers.end(), [] (SourceBufferPrivateAVFObjC* sourceBuffer) {
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaSourcePrivateAVFObjC.
> > mm:    return std::any_of(m_activeSourceBuffers.begin(),
> > m_activeSourceBuffers.end(), [] (SourceBufferPrivateAVFObjC* sourceBuffer) {
> > Source/WebCore/platform/graphics/nicosia/NicosiaAnimation.cpp:    return
> > std::any_of(m_animations.begin(), m_animations.end(),
> > Source/WebCore/platform/graphics/nicosia/NicosiaAnimation.cpp:    return
> > std::any_of(m_animations.begin(), m_animations.end(),
> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:           
> > ASSERT(std::any_of(contextDataMap().begin(), contextDataMap().end(),
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:    return
> > std::any_of(m_children.begin(), m_children.end(),
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.
> > cpp:    bool alreadyRecorded = std::any_of(rects.begin(), rects.end(),
> > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureDeviceManager.cpp:  
> > return std::any_of(devices.begin(), devices.end(), [&deviceID,
> > deviceType](auto& device) {
> > Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp:   
> > return std::any_of(m_activeSourceBuffers.begin(),
> > m_activeSourceBuffers.end(), MockSourceBufferPrivateHasAudio);
> > Source/WebCore/platform/mock/mediasource/MockMediaSourcePrivate.cpp:   
> > return std::any_of(m_activeSourceBuffers.begin(),
> > m_activeSourceBuffers.end(), MockSourceBufferPrivateHasVideo);
> 
> Personally, I like the use of the c++ algorithm functions in cases where
> they make it more clear what is happening than a range-for loop. That
> includes things like std::any_of and std::fill_n (and will like them more
> when we have c++20 ranges, and don't have to pass in explicit begin/end
> iterators). But in the case of that a range-for loop is identical or
> shorter, I think we should stick with it. Perhaps something to re-evaluate
> when we adopt c++20 ranges and can pipeline things.

Ok. Fixed in <https://trac.webkit.org/changeset/268841>.

[Darin Adler]

(In reply to Sam Weinig from comment #8)
> will like them more when we have c++20 ranges

^^^