Bug 237027 - Optimize black frame sending in MediaRecorderPrivate
Summary: Optimize black frame sending in MediaRecorderPrivate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 237025
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-22 01:45 PST by youenn fablet
Modified: 2022-03-01 02:20 PST (History)
11 users (show)

See Also:


Attachments
Patch (14.68 KB, patch)
2022-02-22 02:03 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (13.71 KB, patch)
2022-02-23 06:06 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (24.13 KB, patch)
2022-02-24 06:16 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2022-02-22 01:45:39 PST
Optimise black frame sending in MediaRecorderPrivate
Comment 1 youenn fablet 2022-02-22 02:03:24 PST
Created attachment 452844 [details]
Patch
Comment 2 youenn fablet 2022-02-23 06:06:52 PST
Created attachment 452971 [details]
Patch
Comment 3 Peng Liu 2022-02-23 10:43:14 PST
Comment on attachment 452971 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:101
> +            m_blackFrameSize = WebCore::IntSize { static_cast<int>(size.width()), static_cast<int>(size.height()) };

Just curious, is it possible that we need to change the black frame size?
Comment 4 Kimmo Kinnunen 2022-02-23 10:44:27 PST
Comment on attachment 452971 [details]
Patch

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

with some suggestions

> Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:103
> +        SharedVideoFrame sharedVideoFrame { sample.presentationTime(), sample.videoMirrored(), sample.videoRotation(), *m_blackFrameSize };

From this part it would seem you can just remove m_blackFrameSize and keep sending the real sample size?

> Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.cpp:190
> +    }, [](IntSize size) -> RetainPtr<CVPixelBufferRef> {

I think for this use-case, you could;
- cache the last black frame
- update the last black frame only if the size differs
- set the ownership to the caller (so black frames don't consume GPUP memory)
Comment 5 youenn fablet 2022-02-23 11:33:05 PST
(In reply to Kimmo Kinnunen from comment #4)
> Comment on attachment 452971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452971&action=review
> 
> with some suggestions
> 
> > Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:103
> > +        SharedVideoFrame sharedVideoFrame { sample.presentationTime(), sample.videoMirrored(), sample.videoRotation(), *m_blackFrameSize };
> 
> From this part it would seem you can just remove m_blackFrameSize and keep
> sending the real sample size?

This is done to keep things in sync with webrtc encoding where we stick with the last available frame size.

> > Source/WebKit/WebProcess/GPU/webrtc/SharedVideoFrame.cpp:190
> > +    }, [](IntSize size) -> RetainPtr<CVPixelBufferRef> {
> 
> I think for this use-case, you could;
> - cache the last black frame
> - update the last black frame only if the size differs
> - set the ownership to the caller (so black frames don't consume GPUP memory)

I hesitated to do this (we are storing a buffer that might not be reused nor removed), I'll add it.
Comment 6 youenn fablet 2022-02-24 06:16:24 PST
Created attachment 453097 [details]
Patch for landing
Comment 7 EWS 2022-02-25 00:53:08 PST
Committed r290501 (247788@main): <https://commits.webkit.org/247788@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453097 [details].
Comment 8 Radar WebKit Bug Importer 2022-02-25 00:54:26 PST
<rdar://problem/89463204>
Comment 9 Jon Lee 2022-03-01 01:21:45 PST
This appears to fix fast/mediastream/video-rotation-gpu-process-crash.html on the standard bots, but it is still intermittently crashing on the GPU Process bot

https://results.webkit.org/?suite=layout-tests&test=fast%2Fmediastream%2Fvideo-rotation-gpu-process-crash.html
Comment 10 youenn fablet 2022-03-01 02:20:46 PST
(In reply to Jon Lee from comment #9)
> This appears to fix fast/mediastream/video-rotation-gpu-process-crash.html
> on the standard bots, but it is still intermittently crashing on the GPU
> Process bot
> 
> https://results.webkit.org/?suite=layout-
> tests&test=fast%2Fmediastream%2Fvideo-rotation-gpu-process-crash.html

This is tracked in https://bugs.webkit.org/show_bug.cgi?id=237295