[Webkit-unassigned] [Bug 159405] [GTK] Painting a video into a canvas doesn't work when accelerated compositing is enabled

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=159405

--- Comment #10 from Miguel Gomez <magomez at igalia.com> ---
(In reply to comment #9)
> Comment on attachment 282789 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282789&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-606
> > -#if USE(COORDINATED_GRAPHICS_THREADED)
> > -    return;
> > -#elif USE(TEXTURE_MAPPER_GL) && !USE(COORDINATED_GRAPHICS)
> > -    if (client())
> > -        return;
> > -#endif
> > -
> 
> So, paint was not supposed to be called before when using AC, right? Can
> this still be called when using AC but not rendering in accelerated 2d
> canvas context?

Yes, paint was disabled before when using AC. This was probably because VideoSinkGStreamer was delivering the frames as EGL images, so when using AC there should be a fast copy implemented and it was not there yet. But currently VideoSinkGStreamer returns the frame as normal buffers, so the code to create the image and paint is valid as long as we are not using gstreamer-gl.
Then, when painting, it doesn't matter whether we are using an accelerated canvas or not, as cairo will perform the appropriate operations for each case.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:692
> > -        return nullptr;
> > +        return adoptRef(cairo_gl_surface_create(device, CAIRO_CONTENT_COLOR_ALPHA, 0, 0));
> 
> The caller null checks the return value, when is this supposed to return
> null? So, we are now returning an uninitialized surface?

Yes, the caller checks for null value. But hen trying to create a pattern from the video content, if null is returned CanvasRenderingContext2D will fall back to use paintCurrentFrameInContext, which may crash. I found this problem when trying to get a frame from a video that is not playing yet using gstreamer-gl. In that situation neither the frame nor the caps are available. That's why I return an empty surface instead, so the rendering context doesn't try to paint again.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:697
> > -        return nullptr;
> > +        return adoptRef(cairo_gl_surface_create(device, CAIRO_CONTENT_COLOR_ALPHA, 0, 0));
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:702
> >      cairo_surface_t* surface = cairo_gl_surface_create_for_texture(device, CAIRO_CONTENT_COLOR_ALPHA, textureID, size.width(), size.height());
> 
> Instead of using raw pointers and then create the RefPtr when returning it,
> we could change this to use RefPtr, so that if an early return is added
> after this we won't leak the surface.

Sure.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:703
> > +    cairo_surface_mark_dirty(surface);
> 
> I'm surprised we need to mark the surface as dirty right after creating it,
> and before render into it.

That's because we are creating the surface for a texture that already has content, but cairo doesn't know. So When using cairo to paint it, as there were no cairo painting operations, it assumes that there's nothing to paint. Marking it as dirty tells cairo that there's already content to use.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:706
> > +    cairo_surface_t* rotatedSurface = cairo_gl_surface_create(device, CAIRO_CONTENT_COLOR_ALPHA, rotatedSize.width(), rotatedSize.height());
> 
> Use RefPtr here.

Roger that.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:707
> > +    cairo_t* cr = cairo_create(rotatedSurface);
> 
> And here.

Ok.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:717
> > +    case OriginRightTop: {
> > +        cairo_translate(cr, rotatedSize.width() * 0.5, rotatedSize.height() * 0.5);
> > +        cairo_rotate(cr, M_PI / 2.0);
> > +        cairo_translate(cr, -rotatedSize.height() * 0.5, -rotatedSize.width() * 0.5);
> > +        break;
> > +    }
> 
> Why do you need {} here and not in the other cases?

Good question, I missed that :)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160706/f999c2ee/attachment-0001.html>