Bug 237027

Created attachment 452844 [details]
Patch

Created attachment 452971 [details]
Patch

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

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

Created attachment 453097 [details]
Patch for landing

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

<rdar://problem/89463204>

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

(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