Bug 211720 – Allow WebAudioBufferList to dynamically change its number of frames

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Allow WebAudioBufferList to dynamically change its number of frames

[youenn fablet]

Created attachment 399014 [details]
Patch

[Eric Carlson]

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?

[youenn fablet]

Created attachment 399027 [details]
Patch

[youenn fablet]

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

[youenn fablet]

Created attachment 399033 [details]
Patch

[EWS]

Committed r261557: <https://trac.webkit.org/changeset/261557>

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

[Radar WebKit Bug Importer]

<rdar://problem/63140841>

[Jacob Uphoff]

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>

[youenn fablet]

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.

[youenn fablet]

Created attachment 399277 [details]
Patch for landing

[youenn fablet]

Created attachment 399278 [details]
Patch for landing

[youenn fablet]

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

[EWS]

Committed r261626: <https://trac.webkit.org/changeset/261626>

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