Bug 211720

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].