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