Bug 195436 – WebRTC: don't use stack buffer

• Status: NEW
• Component: New Bugs

[JF Bastien]

WebRTC: don't use stack buffer

[JF Bastien]

Created attachment 363933 [details]
Patch

[EWS Watchlist]

Attachment 363933 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 363941 [details]
Patch

[EWS Watchlist]

Attachment 363941 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 363947 [details]
Patch

[EWS Watchlist]

Attachment 363947 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 363957 [details]
Patch

[EWS Watchlist]

Attachment 363957 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 363957 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        WebRTC: don't create huge stack buffer in a loop

Probably missing https://bugs.webkit.org/show_bug.cgi?id=195436 here.

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:99
> +        m_data.reset(new char[bytesPerSample * channels * samplesPerFrame]);

Would better read with make_unique?

[JF Bastien]

Created attachment 363958 [details]
Patch

Address comments.

[youenn fablet]

Comment on attachment 363958 [details]
Patch

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

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:99
> +        m_data.reset(std::make_unique<char[]>(bytesPerSample * channels * samplesPerFrame));

Would use m_data = std::make_unique.

If we go down that road, we could also allocate m_data in StartPlayoutOnAudioThread and delete it when exiting of StartPlayoutOnAudioThread.
That would remove the if check and deallocate it when no longer needed.

[JF Bastien]

Created attachment 363960 [details]
Patch

(In reply to youenn fablet from comment #11)
> Comment on attachment 363958 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363958&action=review
> 
> > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:99
> > +        m_data.reset(std::make_unique<char[]>(bytesPerSample * channels * samplesPerFrame));
> 
> Would use m_data = std::make_unique.

Oops yes. Fixed.


> If we go down that road, we could also allocate m_data in
> StartPlayoutOnAudioThread and delete it when exiting of
> StartPlayoutOnAudioThread.
> That would remove the if check and deallocate it when no longer needed.

I can do that if you want, I didn't dig through the code to convince myself that it was correct :-)

[Darin Adler]

Comment on attachment 363960 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        The 1920 byte buffer is pretty big, and the loop is unrolled 5
> +        times in a regular build.

If the unrolling is the issue, just putting the buffer outside the loop would solve the problem. Not sure allocating on the heap is the right thing.

[JF Bastien]

Comment on attachment 363960 [details]
Patch

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

>> Source/WebCore/ChangeLog:7
>> +        times in a regular build.
> 
> If the unrolling is the issue, just putting the buffer outside the loop would solve the problem. Not sure allocating on the heap is the right thing.

My ulterior motive is that, once we automatically initialize this stack buffer, it'll be expensive. Without LTO the compiler won't be able to prove anything about the buffer being initialized somewhere else, so it'll call memset. There's a few other places in WebCore where this happens for somewhat large buffers.

[Darin Adler]

(In reply to JF Bastien from comment #14)
> My ulterior motive is that, once we automatically initialize this stack
> buffer, it'll be expensive.

OK.

[youenn fablet]

(In reply to JF Bastien from comment #12)
> Created attachment 363960 [details]
> Patch
> 
> (In reply to youenn fablet from comment #11)
> > Comment on attachment 363958 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=363958&action=review
> > 
> > > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:99
> > > +        m_data.reset(std::make_unique<char[]>(bytesPerSample * channels * samplesPerFrame));
> > 
> > Would use m_data = std::make_unique.
> 
> Oops yes. Fixed.
> 
> 
> > If we go down that road, we could also allocate m_data in
> > StartPlayoutOnAudioThread and delete it when exiting of
> > StartPlayoutOnAudioThread.
> > That would remove the if check and deallocate it when no longer needed.
> 
> I can do that if you want, I didn't dig through the code to convince myself
> that it was correct :-)

I think it is correct and would improve the code a bit.
m_data could be replaced by a local var as well.

[Alex Christensen]

Your earlier version used operator new[] which I believe does not initialize the values to 0, and based on youenn's feedback you now use std::make_unique<char[]>, which I believe does initialize the values to 0.  Is this true?  Is this desired?

[Alex Christensen]

Comment on attachment 363960 [details]
Patch

This also doesn't use fastMalloc/fastFree, but makeUniqueArray does.