[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 #12 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #10)
> (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.

So, maybe that should be handled in paint? I'm not sure I understand how this is supposed to work. Because then paint() is not supposed to be called not either, if the canvas always uses nativeImageForCurrentTime() in case of accelerated 2d canvas, no?

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

This sounds to me like a cairo bug, as I understand it, mark dirty is to tell cairo that you have modified the data of an existing surface. The image backend, marks the surface as dirty when cairo_image_surface_create_for_data is called and the given data is not NULL. I guess the gl backend should do something similar when a surface is created with a texture. If this is the case, we should file a bug report in cairo, add a comment here saying this is a workaround for a cairo bug and adding bug url in the comment, and if we also fix cairo that would be awesome :-)

> > > 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/1f5c31ab/attachment-0001.html>