Canvas drawImage is very slow when used with video as source

• Component: Canvas
• Status: RESOLVED FIXED

[Patryk Rogalski]

Created attachment 439181 [details]
comparison between Chrome and Safari

I'm running the latest Safari and Safari Technology Preview and I've run into a performance issue in Safari.

I have a video that is encoded in H264 2176x1088. The video itself plays smoothly however I have to cut the content of it into 8 different canvas elements.

I have a working demo of it working however the performance I'm getting is abysmal. I've ran the same code in Google Chrome 94 and it's perfect.

Here is the comparison:
Chrome takes about 0.5-2ms to draw
Safari takes 150-330ms to draw
Firefox takes 100-150ms

And it has to run at 30 fps... which is completely impossible right now.

Here is a demo to showcase the issue: https://cdn.zu.casa/dev/apple/index.html

If open developer console it'll print out to the console the time it took to render.

[Patryk Rogalski]

I had an older version of Safari on my iMac and it ran super fast (1.5-4ms) - as fast as in Google Chrome. After updating to the latest version it's terrible (200-350ms).

[Sam Sneddon [:gsnedders]]

Quick look at a sample suggests we're blocking on cross-thread/process communication a bunch. Probably a regression caused by moving more things out-of-process?

[Radar WebKit Bug Importer]

<rdar://problem/83576009>

[Cameron McCormack (:heycam)]

Thanks for the bug report.  Sam is right: this is because with GPU process canvas rendering enabled, we end up doing some copies of the HTMLVideoElement frame data (which lives in the GPU process) to share it (synchronously) to the Web process, only to ship it back to the GPU process again for the drawImage() call.  We should be able to avoid any of this copying.

[Cameron McCormack (:heycam)]

Created attachment 442334 [details]
Patch for EWS

[Cameron McCormack (:heycam)]

Created attachment 442649 [details]
Patch

[Cameron McCormack (:heycam)]

Simon pointed out to me there's another call to nativeImageForCurrentTime(), in createPattern().  But patterns currently can't avoid copying the pattern source image data from the GPU process, so I'll leave that be for now.  And filed https://bugs.webkit.org/show_bug.cgi?id=232411 for the more general problem.

[Cameron McCormack (:heycam)]

Created attachment 442787 [details]
Patch

[EWS]

Committed r285055 (243698@main): <https://commits.webkit.org/243698@main>

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

Reverted r285055 for reason:

Reverting because this commit may have caused webgl/1.0.x/conformance/textures/misc/texture-corner-case-videos.html and webgl/2.0.y/conformance/textures/misc/texture-corner-case-videos.html to time out

Committed r285110 (243751@main): <https://commits.webkit.org/243751@main>

[Said Abou-Hallawa]

Comment on attachment 442787 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1680
> +    if (!canvasBase().buffer()->isRemote()) {

We have been trying to avoid adding this condition in WebCore. Can't we do this by checking whether 'c', which is the GraphicsContext of canvasBase().buffer(), is a recording GraphicsContext or not?

I think you can replace this condition by 

if (c->hasPlatformContext()) {
    ...
    return { };
}

This function has been used for this purpose in WebCore in a few places. Just search for it.

> Source/WebCore/platform/graphics/ImageBuffer.h:74
> +    virtual bool isRemote() const { return false; }

Please avoid adding such check in WebCore.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:98
> +    bool isRemote() const final { return true; }

This method can be private.

[Cameron McCormack (:heycam)]

(In reply to Said Abou-Hallawa from comment #11)
> > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1680
> > +    if (!canvasBase().buffer()->isRemote()) {
> 
> We have been trying to avoid adding this condition in WebCore. Can't we do
> this by checking whether 'c', which is the GraphicsContext of
> canvasBase().buffer(), is a recording GraphicsContext or not?
> 
> I think you can replace this condition by 
> 
> if (c->hasPlatformContext()) {
>     ...
>     return { };
> }

I had that in my original patch but Simon was not sure about it, so I changed it to this more direct check on the ImageBuffer.  I can change it back.

[Wenson Hsieh]

(In reply to Cameron McCormack (:heycam) from comment #12)
> (In reply to Said Abou-Hallawa from comment #11)
> > > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1680
> > > +    if (!canvasBase().buffer()->isRemote()) {
> > 
> > We have been trying to avoid adding this condition in WebCore. Can't we do
> > this by checking whether 'c', which is the GraphicsContext of
> > canvasBase().buffer(), is a recording GraphicsContext or not?
> > 
> > I think you can replace this condition by 
> > 
> > if (c->hasPlatformContext()) {
> >     ...
> >     return { };
> > }
> 
> I had that in my original patch but Simon was not sure about it, so I
> changed it to this more direct check on the ImageBuffer.  I can change it
> back.

Generally speaking, methods that explicitly reference the WebKit process model (e.g. the notion of "remote" or "proxy" objects) normally exist in the WebKit2 layer, with client layer hooks (on WebChromeClient, for instance) that WebCore code may use in order to delegate decision-making to things that know about the process model.

That said, there are some exceptions to this. But in general, ImageBuffer and its subclasses/backends have preserved this separation — it's also how we've (previously) avoided adding a method like `isRemote()` to image buffer.

[Cameron McCormack (:heycam)]

Created attachment 443372 [details]
Patch for landing

[EWS]

Committed r285333 (243896@main): <https://commits.webkit.org/243896@main>

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