Allow WebAudioBufferList to dynamically change its number of frames
Created attachment 399014 [details] Patch
Comment on attachment 399014 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399014&action=review > Source/WebCore/Modules/webaudio/MediaStreamAudioSource.cpp:36 > +#if USE(AVFOUNDATION) > +#include "WebAudioBufferList.h" > +#include <CoreAudio/CoreAudioTypes.h> > +#endif Do we really need to include these here in the cross platform file? > Source/WebCore/Modules/webaudio/MediaStreamAudioSource.h:65 > #if USE(AVFOUNDATION) > + std::unique_ptr<WebAudioBufferList> m_audioBuffer; > size_t m_numberOfFrames { 0 }; > #endif This seems like a mistake. It's probably time to actually subclass for Cocoa rather than just implementing parts of the class in MediaStreamAudioSourceCocoa.cpp > Source/WebCore/platform/audio/cocoa/WebAudioBufferList.cpp:71 > + uint32_t bufferCount = m_canonicalList->mNumberBuffers; > + if (!bufferCount) > + return; > > - size_t bytesPerBuffer = sampleCount * channelCount * format.bytesPerFrame(); > - m_flatBuffer.reserveInitialCapacity(bufferCount * bytesPerBuffer); > + size_t bytesPerBuffer = sampleCount * m_channelCount * m_bytesPerFrame; > + m_flatBuffer.reserveCapacity(bufferCount * bytesPerBuffer); > auto data = m_flatBuffer.data(); Can this return early if nothing has changed so we don't call reset() needlessly?
Created attachment 399027 [details] Patch
(In reply to Eric Carlson from comment #2) > Comment on attachment 399014 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399014&action=review > > > Source/WebCore/Modules/webaudio/MediaStreamAudioSource.cpp:36 > > +#if USE(AVFOUNDATION) > > +#include "WebAudioBufferList.h" > > +#include <CoreAudio/CoreAudioTypes.h> > > +#endif > > Do we really need to include these here in the cross platform file? Fixed it by using PlatformAudioData. > > Source/WebCore/Modules/webaudio/MediaStreamAudioSource.h:65 > > #if USE(AVFOUNDATION) > > + std::unique_ptr<WebAudioBufferList> m_audioBuffer; > > size_t m_numberOfFrames { 0 }; > > #endif > > This seems like a mistake. It's probably time to actually subclass for Cocoa > rather than just implementing parts of the class in > MediaStreamAudioSourceCocoa.cpp I made a try at keeping it like it is but, yes maybe we should have a cocoa specialisation. > > Source/WebCore/platform/audio/cocoa/WebAudioBufferList.cpp:71 > > + uint32_t bufferCount = m_canonicalList->mNumberBuffers; > > + if (!bufferCount) > > + return; > > > > - size_t bytesPerBuffer = sampleCount * channelCount * format.bytesPerFrame(); > > - m_flatBuffer.reserveInitialCapacity(bufferCount * bytesPerBuffer); > > + size_t bytesPerBuffer = sampleCount * m_channelCount * m_bytesPerFrame; > > + m_flatBuffer.reserveCapacity(bufferCount * bytesPerBuffer); > > auto data = m_flatBuffer.data(); > > Can this return early if nothing has changed so we don't call reset() > needlessly? Added an if check.
Created attachment 399033 [details] Patch
Committed r261557: <https://trac.webkit.org/changeset/261557> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399033 [details].
<rdar://problem/63140841>
Reverted r261557 for reason: This commit caused testing to exit early due to too many crashes on macOS Catalina Asan Committed r261566: <https://trac.webkit.org/changeset/261566>
ASAN issue is a false positive. We preallocate a Vector with reserveCapacity and write within the bounds of the allocated buffer, but past the end of the Vector (which is equal to begin) since the vector size is zero. We cannot use reserveCapacity here since it is calling asanSetInitialBufferSizeTo.
Created attachment 399277 [details] Patch for landing
Created attachment 399278 [details] Patch for landing
> We cannot use reserveCapacity here since it is calling > asanSetInitialBufferSizeTo. I switched to resize() which is not that much expensive and will make sure we can write on the whole allocated buffer.
Committed r261626: <https://trac.webkit.org/changeset/261626> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399278 [details].