Bug 236502 – WebGL GPUP: Crash when running fast/mediastream/getUserMedia-to-canvas-1.html

• Component: WebGL
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

WebGL GPUP: Crash when running fast/mediastream/getUserMedia-to-canvas-1.html 


Needs implementation of user media capture'd media player paint to canvas

[Radar WebKit Bug Importer]

<rdar://problem/88862921>

[Kimmo Kinnunen]

Created attachment 452365 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 452365 [details]
Patch

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

> Source/WebCore/platform/VideoFrame.h:56
>      virtual bool isCV() const { return false; }

The CV abbreviation is a bit obscure here.

> Source/WebCore/platform/VideoFrame.h:57
> +    virtual RefPtr<WebCore::VideoFrameCV> asVideoFrameCV() = 0;

This seems like a layering violation. Normally we'd do this by adding SPECIALIZE_TYPE_TRAITS macros to allow for is<> and downcast<>

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLCocoa.cpp:62
> +        videoFrameCV = &downcast<WebCore::VideoFrameCV>(*videoFrame);

This doesn't use asVideoFrameCV()?

[Kimmo Kinnunen]

Comment on attachment 452365 [details]
Patch

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

>> Source/WebCore/platform/VideoFrame.h:57
>> +    virtual RefPtr<WebCore::VideoFrameCV> asVideoFrameCV() = 0;
> 
> This seems like a layering violation. Normally we'd do this by adding SPECIALIZE_TYPE_TRAITS macros to allow for is<> and downcast<>

It has those. This method is different. It converts to the type.
This is the polymorphic conversion function.

>> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLCocoa.cpp:62
>> +        videoFrameCV = &downcast<WebCore::VideoFrameCV>(*videoFrame);
> 
> This doesn't use asVideoFrameCV()?

No, that's a different function.

[Eric Carlson]

Comment on attachment 452365 [details]
Patch

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

>> Source/WebCore/platform/VideoFrame.h:56
>>      virtual bool isCV() const { return false; }
> 
> The CV abbreviation is a bit obscure here.

It seems fine to me, and it won't be the first time we've used it, e.g. GraphicsContextGLCV, PixelBufferConformerCV, GraphicsContextGLCVCocoa

[Eric Carlson]

r=me once the bots are happy

[Kimmo Kinnunen]

Created attachment 452506 [details]
Patch

[Kimmo Kinnunen]

Created attachment 452509 [details]
Patch

[Kimmo Kinnunen]

Created attachment 452515 [details]
Patch for landing

[Kimmo Kinnunen]

Created attachment 452524 [details]
Patch for landing

[Kimmo Kinnunen]

Created attachment 452538 [details]
Patch for landing

[EWS]

Committed r290175 (247506@main): <https://commits.webkit.org/247506@main>

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

[Kimmo Kinnunen]

nongpup webgl started to assert, will fix in bug 236878

[youenn fablet]

Comment on attachment 452538 [details]
Patch for landing

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

> Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxy.cpp:190
> +    if (!videoFrame || !is<RemoteVideoFrameProxy>(*videoFrame))

This should probably be a FIXME, painting VP8 WebRTC streamed content on WebGL may exercise that code path.
I filed https://bugs.webkit.org/show_bug.cgi?id=236972 to track this.