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.
Created attachment 412018 [details] Patch
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?
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().
Committed r268820: <https://trac.webkit.org/changeset/268820> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412018 [details].
<rdar://problem/70542842>
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());
(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);
(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.
(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>.
(In reply to Sam Weinig from comment #8) > will like them more when we have c++20 ranges ^^^