Bug 217339

Created attachment 410690 [details]
Patch

<rdar://problem/69409029>

Comment on attachment 410690 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        details.

I guess we can enable some tests in this patch?

(In reply to Peng Liu from comment #6)
> Comment on attachment 410690 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410690&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        details.
> 
> I guess we can enable some tests in this patch?

The entirety of fast/canvas, canvas, imported/w3c/canvas and imported/w3c/web-platform-tests/html/canvas are skipped at the moment. This code should be covered by the following tests once we enable those directories:

• imported/w3c/web-platform-tests/html/canvas/element/imagebitmap/canvas-createImageBitmap-video-resize.html
• fast/canvas/canvas-createPattern-video-loading.html
• fast/canvas/canvas-createPattern-video-modify.html
• imported/w3c/web-platform-tests/html/canvas/element/imagebitmap/createImageBitmap-drawImage.html
• imported/w3c/web-platform-tests/html/canvas/element/imagebitmap/createImageBitmap-flipY.html

Comment on attachment 410690 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:1263
> +    player.setVisible(true);

Surprised that this:

1) is needed.
2) is needed every time.
3) is harmless from a side effect point of view

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1937
> +void GraphicsContext::paintFrameForMedia(MediaPlayer& player, const FloatRect& destination)

Annoying that this repeats the code from the non-CG version. Is there a better way to factor this?

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1947
> +    player.setVisible(true);

Ditto.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1242
> +    // Should be handled by the delegate.

Ugly but OK.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2589
> +    static Ref<PaintFrameForMedia> create(MediaPlayer& player, const FloatRect& destination)

Should not inline this kind of function. The constructor can get inclined instead. Better for efficiency and for class definition brevity.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2603
> +    static Ref<PaintFrameForMedia> create(MediaPlayerIdentifier identifier, const FloatRect& destination)

Ditto.

Comment on attachment 410690 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:1263
>> +    player.setVisible(true);
> 
> Surprised that this:
> 
> 1) is needed.
> 2) is needed every time.
> 3) is harmless from a side effect point of view

Good catch! After taking a closer look, it shouldn’t be necessary at all — I removed this line.

My intent had been to preserve the logic in `HTMLVideoElement::paintCurrentFrameInContext` when playing the drawing item back in the GPUP. However, since `MediaPlayerPrivateRemote::setVisible` will make the remote-side MediaPlayer visible anyways, doing this in GraphicsContext is redundant.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1937
>> +void GraphicsContext::paintFrameForMedia(MediaPlayer& player, const FloatRect& destination)
> 
> Annoying that this repeats the code from the non-CG version. Is there a better way to factor this?

I had done it this way to avoid changing behavior when using the Nicosia-specific `CairoOperationRecorder` as the `GraphicsContextImpl` — I’ve changed this to avoid duplicating the logic in CoreGraphics platform code by instead doing it like this:
• Add a `GraphicsContextImpl::canPaintFrameForMedia` method that returns `true` for the display list recorder, and false in other GraphicsContextImpl subclasses where it’s not implemented.
• Move `GraphicsContext::paintFrameForMedia` into the common `GraphicsContext.cpp` file, and consult `canPaintFrameForMedia` before delegating the call to `m_impl`.

(This way, we can avoid code duplication, and also keep Nicosia working).

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1947
>> +    player.setVisible(true);
> 
> Ditto.

Removed!

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1242
>> +    // Should be handled by the delegate.
> 
> Ugly but OK.

Indeed. I hatched a plan (with @thorton) to fix this:

We’re thinking of a design where we push logic for applying the display list item out of the delegate (in this case, RemoteImageBufferProxy) and into `PaintFrameForMedia::apply` and `PutImageData::apply`. Instead of a `DisplayList::Replayer::Delegate::apply`, we would then have a `DisplayList::Replayer::Delegate::willApply` method that will prepare the Display List item for playback. In the case of PaintFrameForMedia, we could set a pointer to the `MediaPlayer`, and in the case of `PutImageData`, it could be something like telling the display list item about the `ImageBuffer`.

I’m planning to explore this in a followup.

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2589
>> +    static Ref<PaintFrameForMedia> create(MediaPlayer& player, const FloatRect& destination)
> 
> Should not inline this kind of function. The constructor can get inclined instead. Better for efficiency and for class definition brevity.

Sounds good — made this out of line. That said, the other static `create` methods in this file are also currently inline; I’ll put together a followup patch to mass move these too.

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2603
>> +    static Ref<PaintFrameForMedia> create(MediaPlayerIdentifier identifier, const FloatRect& destination)
> 
> Ditto.

👍🏻

Created attachment 410758 [details]
For EWS

Committed r268145: <https://trac.webkit.org/changeset/268145>

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