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

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Miguel Gomez]

When not using accelerated compositing, it's possible to paint a video's content into a canvas, but this is not working when accelerated compositing is enabled. This happens with or without the threaded compositor enabled, and also with and without using gstreamer-gl.

[Miguel Gomez]

Created attachment 282712 [details]
Patch

[Miguel Gomez]

Created attachment 282714 [details]
Patch

[Philippe Normand]

Comment on attachment 282714 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        Delegate the painting into the player, who knows the video orientation to apply.

Why?
Can't we expose the video orientation somehow to the rendering context so that everything could be handled there without going to platform layer?

It's a bit weird to let the CG code path as-is and handle the feature differently on our side.

[Miguel Gomez]

Created attachment 282786 [details]
Patch

[Miguel Gomez]

I've changed the approach to follow the same code path inside CanvasRenderingContext2D. Now MediaPlayerPrivateGStreamerBase::nativeImageForCurrentTime returs the image properly rotated according to the video orientation. This way all the orientation stuff gets managed inside that class.

[Philippe Normand]

Comment on attachment 282786 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:708
> +    double pi = 3.14159265358979323846;

Please use the constants defined in wtf/MathExtras.h :)

[Miguel Gomez]

Created attachment 282789 [details]
Patch

[Miguel Gomez]

(In reply to comment #6)
> Comment on attachment 282786 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282786&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:708
> > +    double pi = 3.14159265358979323846;
> 
> Please use the constants defined in wtf/MathExtras.h :)

Fixed in last version of the patch :)

[Carlos Garcia Campos]

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?

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

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

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

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

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

And here.

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

[Miguel Gomez]

(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 :)

[Miguel Gomez]

Created attachment 282858 [details]
Patch

[Carlos Garcia Campos]

(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 :)

[Miguel Gomez]

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

Ok, I'll modify the patch to return null again, and open a new bug to handle the situation where the caps are not negotiated in another bug.
 
> > > > 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 :-)

I've been retesting this and seems that the mark_dirty is not necessary. I added it when using other approaches to fix the issue, but seems that with this approach it's working properly without it.

[Miguel Gomez]

Created attachment 282870 [details]
Patch

[Miguel Gomez]

I've just created the bug to handle the empty caps/frames before playing state when using gstreamer-gl:

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

[Philippe Normand]

Comment on attachment 282870 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:712
> +        cairo_rotate(cr.get(), M_PI / 2.0);

Use piOverTwoDouble or piOverTwoFloat ?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:717
> +        cairo_rotate(cr.get(), M_PI);

Use piFloat or piDouble

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:722
> +        cairo_rotate(cr.get(), 3 * M_PI / 2.0);

Ditto

[Miguel Gomez]

Created attachment 282885 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 282885 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:733
> +    // Ensure that we destroy the surface before unmapping the video frame.
> +    surface.release();

release() leaks the surface, it doesn't destroy it. What you want is

surface = nullptr;

but I'm not sure this is enough, though. I don't know why it's needed to destroy the surface before unmapping the frame, but the cairo context owns a reference to the surface , so even if you clear the smart pointer the surface will still be alive.

[Miguel Gomez]

(In reply to comment #18)
> Comment on attachment 282885 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282885&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:733
> > +    // Ensure that we destroy the surface before unmapping the video frame.
> > +    surface.release();
> 
> release() leaks the surface, it doesn't destroy it. What you want is
> 
> surface = nullptr;

Ooops

> but I'm not sure this is enough, though. I don't know why it's needed to
> destroy the surface before unmapping the frame, but the cairo context owns a
> reference to the surface , so even if you clear the smart pointer the
> surface will still be alive.

I wanna destroy the surface created from the video frame texture before the frame unmapping because once the frame is unmapped that texture can be freed/reused by the gstreamer bin.
Curiously this isn't checked in the current code, just returning the video frame surface. I wonder if I'm being too careful.
Ok, I'll keep it as in the original code, where the surface is not destroyed.

[Miguel Gomez]

Created attachment 282995 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to comment #13)
> > 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?
> 
> Ok, I'll modify the patch to return null again, and open a new bug to handle
> the situation where the caps are not negotiated in another bug.

Please add a FIXME comment with a reference to that bug so that we have a reference in the code too.

[Xabier Rodríguez Calvar]

Comment on attachment 282995 [details]
Patch

Please, add the requested comments and we are good to go

[Miguel Gomez]

Created attachment 283005 [details]
Patch

[Miguel Gomez]

Created attachment 283006 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 283006 [details]
Patch

Rejecting attachment 283006 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 283006, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/1640748

[WebKit Commit Bot]

Comment on attachment 283006 [details]
Patch

Clearing flags on attachment: 283006

Committed r202901: <http://trac.webkit.org/changeset/202901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.