Bug 159928 – [GStreamer] Support a direct GPU-to-GPU copy of video textures to WebGL

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Olivier Blin]

Copy of GStreamer video frames to WebGL textures is not accelerated, since copyVideoTextureToPlatformTexture() is not implemented.

Currently, WebGLRenderingContextBase::texImage2D(HTMLVideoElement) goes through a slow software paint() that wraps the video frame into a cairo surface (ImageGStreamer) and downloads it to draw to the image cache context, which gets copied again, and uploaded again to an OpenGL texture.

Instead, GPU-to-GPU copy should be implemented in copyVideoTextureToPlatformTexture(), like Apple port are doing with VideoTextureCopierCV.

[Olivier Blin]

Created attachment 284029 [details]
Patch

[Olivier Blin]

(In reply to comment #1)
> Created attachment 284029 [details]
> Patch

Posting for comments.

This patch implements copyVideoTextureToPlatformTexture() for the GStreamer media player backend, to do GPU-to-GPU copy.

This copies the VideoTextureCopierCV code from Apple backends, removing Apple specifics.

If this approach is ok, the patch should be reworked to properly extract VideoTextureCopier, by inlining the few remaining VideoTextureCopierCV specifics in MediaPlayerPrivateAVFoundationObjC.

Though, Miguel already has concerns about this, since it does not support the video rotation flags.
Miguel suggests to reuse cairo_gl_surface_create_for_texture() instead, like in nativeImageForCurrentTime(), and let cairo do the texture copy and rotating if needed.

[Philippe Normand]

Adding some Apple Media experts as this patch intends to refactor Apple code.

[Miguel Gomez]

> Though, Miguel already has concerns about this, since it does not support
> the video rotation flags.
> Miguel suggests to reuse cairo_gl_surface_create_for_texture() instead, like
> in nativeImageForCurrentTime(), and let cairo do the texture copy and
> rotating if needed.

As I explained to you, we need to take into consideration the video orientation flag when we are rendering video frames with accelerated composition enabled. This is because the frames we get from the video are not following the video orientation in the acc composition case, and we apply the appropriate rotation when painting them. This is done in the video element, in the canvas, and needs to be done in webgl as well.

Also, on a fast look to the patch I realized that you are not using the appropriate mutex when accessing the video sample.

[Olivier Blin]

(In reply to comment #4)
> > Though, Miguel already has concerns about this, since it does not support
> > the video rotation flags.
> > Miguel suggests to reuse cairo_gl_surface_create_for_texture() instead, like
> > in nativeImageForCurrentTime(), and let cairo do the texture copy and
> > rotating if needed.
> 
> As I explained to you, we need to take into consideration the video
> orientation flag when we are rendering video frames with accelerated
> composition enabled. This is because the frames we get from the video are
> not following the video orientation in the acc composition case, and we
> apply the appropriate rotation when painting them. This is done in the video
> element, in the canvas, and needs to be done in webgl as well.

Yep, I posted my current state of work for reference, I will rework as you advised.

> Also, on a fast look to the patch I realized that you are not using the
> appropriate mutex when accessing the video sample.

Ok, isn't one needed in the GSTREAMER_GL case of paintToTextureMapper() as well?

Thanks for your input

[Olivier Blin]

Created attachment 286293 [details]
Patch

new version using cairo, sharing code with nativeImageForCurrentTime()

[Olivier Blin]

(In reply to comment #6)
> Created attachment 286293 [details]
> Patch
> 
> new version using cairo, sharing code with nativeImageForCurrentTime()

Not sure if the following calls should be repeated, we can probably just do this once in the caller:
    context->makeContextCurrent();
    cairo_gl_device_set_thread_aware(device, FALSE);

[Michael Catanzaro]

Miguel, Philippe, can you review it again?

[Olivier Blin]

Created attachment 288829 [details]
Patch

[Olivier Blin]

(In reply to comment #9)
> Created attachment 288829 [details]
> Patch

This is rebased on trunk, with improvements by Loïc Yhuel:
- save and restore TEXTURE_MIN/MAG_FILTER after cairo_gl_surface_create_for_texture
textures filters, otherwise cairo overwrites GL_LINEAR with GL_NEAREST, and the video gets pixelated
- remove useless texture parameters

[Miguel Gomez]

Comment on attachment 288829 [details]
Patch

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

Sorry for taking so long, I've been quite busy.
The approach is fine, just a couple of suggested improvements.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:683
>      if (m_usingFallbackVideoSink)

This check should be at the beginning of nativeImageForCurrentTime() and copyVideoTextureToPlatformTexture(). We can save some useless operations if we are not using opengl.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:755
> +    GLContext* glContext = GLContext::sharingContext();

You'll need to change this with GLContext* context = PlatformDisplay::sharedDisplayForCompositing().sharingGLContext();

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:756
> +    glContext->makeContextCurrent();

No need to make the sharing context current here, as context->bindTexture() below will change it again. Cairo will make it current when needed.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:775
> +    if (!paintToCairoSurface(outputSurface))

Pass the cairo device and the videoInfo to paintToCairoSurface()

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:793
> +    GLContext* context = GLContext::sharingContext();

You'll need to change this with GLContext* context = PlatformDisplay::sharedDisplayForCompositing().sharingGLContext();

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:804
> +    if (!paintToCairoSurface(rotatedSurface))

Pass the cairo device and the videoInfo to paintToCairoSurface()

[Olivier Blin]

Created attachment 289443 [details]
Patch

This addresses issues listed in comment 11.

[Miguel Gomez]

Comment on attachment 289443 [details]
Patch

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

Besides one small comment the rest looks good to me.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:710
>      GLContext* context = PlatformDisplay::sharedDisplayForCompositing().sharingGLContext();
>      context->makeContextCurrent();

Now that the cairo device is a parameter, these 2 lines are not needed. The appropriate context will be set by cairo as well.

[Olivier Blin]

Created attachment 289463 [details]
Patch

This addresses the last issue listed in comment 13.

[Carlos Garcia Campos]

Comment on attachment 289463 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:700
> +bool MediaPlayerPrivateGStreamerBase::paintToCairoSurface(RefPtr<cairo_surface_t> outputSurface, cairo_device_t* device, GstVideoInfo* videoInfo)

I don't think you need to pass the smart pointer to this function, and in that case I would pass a reference instead of a copy, but I think you can simply pass the pointer.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:702
> +    // m_sampleMutex should be locked by the caller

I would move this comment before the function, saying something like: // This should be called with the sample mutex locked.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:710
> -    IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo));
> +    IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(videoInfo), GST_VIDEO_INFO_HEIGHT(videoInfo));

Both callers do this, maybe you could pass this as a const reference.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:713
>      IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size;

Ditto.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:773
> +    GLContext* glContext = PlatformDisplay::sharedDisplayForCompositing().sharingGLContext();
> +    cairo_device_t* device = glContext->cairoDevice();
> +
> +    // Thread-awareness is a huge performance hit on non-Intel drivers.
> +    cairo_gl_device_set_thread_aware(device, FALSE);
> +
> +    IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo));
> +    IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size;
> +

Don't you need to make the context current in this case?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:813
> +    GstVideoInfo videoInfo;
> +    WTF::GMutexLocker<GMutex> lock(m_sampleMutex);
> +    if (!getSampleVideoInfo(m_sample.get(), videoInfo))
> +        return nullptr;
> +
> +    GLContext* context = PlatformDisplay::sharedDisplayForCompositing().sharingGLContext();
> +    context->makeContextCurrent();
> +    cairo_device_t* device = context->cairoDevice();
> +
> +    // Thread-awareness is a huge performance hit on non-Intel drivers.
> +    cairo_gl_device_set_thread_aware(device, FALSE);
> +
> +    IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo));
> +    IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size;

All this is mostly duplicated code, could we move it to a helper function? maybe a function that prepares the context and returns the GLContext as return value, and video info, and sizes as output parameters

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:119
> +    bool paintToCairoSurface(RefPtr<cairo_surface_t> outputSurface, cairo_device_t*, GstVideoInfo*);

This should be private.

[Miguel Gomez]

Awesome, I was just about to ping a reviewer to give it a look and you were faster Carlos. 

Also a reply to one of your comments:

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:773
> > +    GLContext* glContext = PlatformDisplay::sharedDisplayForCompositing().sharingGLContext();
> > +    cairo_device_t* device = glContext->cairoDevice();
> > +
> > +    // Thread-awareness is a huge performance hit on non-Intel drivers.
> > +    cairo_gl_device_set_thread_aware(device, FALSE);
> > +
> > +    IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo));
> > +    IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size;
> > +
> 
> Don't you need to make the context current in this case?
> 

Making the context current in that case is not needed. Setting thread awareness is just a flag, doesn't need the context to be current, and also the context->bindTexture() call three lines below will make current the weblg context again.

[Olivier Blin]

(In reply to comment #15)
> Comment on attachment 289463 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289463&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:700
> > +bool MediaPlayerPrivateGStreamerBase::paintToCairoSurface(RefPtr<cairo_surface_t> outputSurface, cairo_device_t* device, GstVideoInfo* videoInfo)
> 
> I don't think you need to pass the smart pointer to this function, and in
> that case I would pass a reference instead of a copy, but I think you can
> simply pass the pointer.

Right, I have also simplified the callers, the WebGL case does not need a smart pointer at all.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:702
> > +    // m_sampleMutex should be locked by the caller
> 
> I would move this comment before the function, saying something like: //
> This should be called with the sample mutex locked.
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:710
> > -    IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo));
> > +    IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(videoInfo), GST_VIDEO_INFO_HEIGHT(videoInfo));
> 
> Both callers do this, maybe you could pass this as a const reference.

Ok

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:713
> >      IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size;
> 
> Ditto.

Ok

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:773
> > +    GLContext* glContext = PlatformDisplay::sharedDisplayForCompositing().sharingGLContext();
> > +    cairo_device_t* device = glContext->cairoDevice();
> > +
> > +    // Thread-awareness is a huge performance hit on non-Intel drivers.
> > +    cairo_gl_device_set_thread_aware(device, FALSE);
> > +
> > +    IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo));
> > +    IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size;
> > +
> 
> Don't you need to make the context current in this case?

See Miguel's answer.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:813
> > +    GstVideoInfo videoInfo;
> > +    WTF::GMutexLocker<GMutex> lock(m_sampleMutex);
> > +    if (!getSampleVideoInfo(m_sample.get(), videoInfo))
> > +        return nullptr;
> > +
> > +    GLContext* context = PlatformDisplay::sharedDisplayForCompositing().sharingGLContext();
> > +    context->makeContextCurrent();
> > +    cairo_device_t* device = context->cairoDevice();
> > +
> > +    // Thread-awareness is a huge performance hit on non-Intel drivers.
> > +    cairo_gl_device_set_thread_aware(device, FALSE);
> > +
> > +    IntSize size = IntSize(GST_VIDEO_INFO_WIDTH(&videoInfo), GST_VIDEO_INFO_HEIGHT(&videoInfo));
> > +    IntSize rotatedSize = m_videoSourceOrientation.usesWidthAsHeight() ? size.transposedSize() : size;
> 
> All this is mostly duplicated code, could we move it to a helper function?
> maybe a function that prepares the context and returns the GLContext as
> return value, and video info, and sizes as output parameters

I am not sure it is worth it, it is not much more readable after being extracted, but I will do so in the next patch proposal.
Also, the cairo_device needs to be kept around as well.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:119
> > +    bool paintToCairoSurface(RefPtr<cairo_surface_t> outputSurface, cairo_device_t*, GstVideoInfo*);
> 
> This should be private.

Yes, moved it to together with other protected methods.

Thanks for your feedback!

[Olivier Blin]

Created attachment 289542 [details]
Patch

This addresses the last issue listed in comment 13.

[Carlos Garcia Campos]

Comment on attachment 289542 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:701
> +GLContext* MediaPlayerPrivateGStreamerBase::prepareContextForCairoPaint(cairo_device_t*& device, GstVideoInfo& videoInfo, IntSize& size, IntSize& rotatedSize)

We don't need to return the device, you can get it from the context.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:710
> +    cairo_gl_device_set_thread_aware(device, FALSE);

You can do cairo_gl_device_set_thread_aware(glContext->cairoDevice(), FALSE); directly.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:773
> +    cairo_device_t* device;

So, this is not needed.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:778
> +    if (!prepareContextForCairoPaint(device, videoInfo, size, rotatedSize))
> +        return false;

Get the context here and use it below to pass the cairo device to paintToCairoSurface()

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:784
> +    // cairo_gl_surface_create_for_texture sets these parameters to GL_NEAREST, so we need to backup them

Nit: finish comments with '.'

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:806
> +    cairo_device_t* device;

Same here.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:813
> +    GLContext* context = prepareContextForCairoPaint(device, videoInfo, size, rotatedSize);
> +    if (!context)
> +        return nullptr;
> +    context->makeContextCurrent();

I still think that making the context current in prepareContextForCairoPaint wouldn't hurt even if it's not needed in the other case.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:819
> +    cairo_surface_t* rotatedSurface = cairo_gl_surface_create(device, CAIRO_CONTENT_COLOR_ALPHA, rotatedSize.width(), rotatedSize.height());
> +    if (!paintToCairoSurface(rotatedSurface, device, videoInfo, size, rotatedSize))
> +        return nullptr;
> +
> +    return adoptRef(rotatedSurface);

The smart pointer was correct here, now you are leaking the surface if paintToCairoSurface() returns false.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:136
> +    bool paintToCairoSurface(cairo_surface_t* outputSurface, cairo_device_t*, GstVideoInfo&, const IntSize&, const IntSize&);

outputSurface can be omitted in this case

[Olivier Blin]

(In reply to comment #19)
> Comment on attachment 289542 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289542&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:701
> > +GLContext* MediaPlayerPrivateGStreamerBase::prepareContextForCairoPaint(cairo_device_t*& device, GstVideoInfo& videoInfo, IntSize& size, IntSize& rotatedSize)
> 
> We don't need to return the device, you can get it from the context.

Ok

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:710
> > +    cairo_gl_device_set_thread_aware(device, FALSE);
> 
> You can do cairo_gl_device_set_thread_aware(glContext->cairoDevice(),
> FALSE); directly.

Ok
 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:773
> > +    cairo_device_t* device;
> 
> So, this is not needed.

Ok
 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:778
> > +    if (!prepareContextForCairoPaint(device, videoInfo, size, rotatedSize))
> > +        return false;
> 
> Get the context here and use it below to pass the cairo device to
> paintToCairoSurface()

Ok

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:784
> > +    // cairo_gl_surface_create_for_texture sets these parameters to GL_NEAREST, so we need to backup them
> 
> Nit: finish comments with '.'

Ok

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:806
> > +    cairo_device_t* device;
> 
> Same here.

Ok

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:813
> > +    GLContext* context = prepareContextForCairoPaint(device, videoInfo, size, rotatedSize);
> > +    if (!context)
> > +        return nullptr;
> > +    context->makeContextCurrent();
> 
> I still think that making the context current in prepareContextForCairoPaint
> wouldn't hurt even if it's not needed in the other case.

Done as well
 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:819
> > +    cairo_surface_t* rotatedSurface = cairo_gl_surface_create(device, CAIRO_CONTENT_COLOR_ALPHA, rotatedSize.width(), rotatedSize.height());
> > +    if (!paintToCairoSurface(rotatedSurface, device, videoInfo, size, rotatedSize))
> > +        return nullptr;
> > +
> > +    return adoptRef(rotatedSurface);
> 
> The smart pointer was correct here, now you are leaking the surface if
> paintToCairoSurface() returns false.

That's right, I will fix for both cases.
Thanks for spotting this.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:136
> > +    bool paintToCairoSurface(cairo_surface_t* outputSurface, cairo_device_t*, GstVideoInfo&, const IntSize&, const IntSize&);
> 
> outputSurface can be omitted in this case

Ok

[Olivier Blin]

This change also fixes bug #159621, I'll add the following in ChangeLog:

+        Doing this also fixes bug #159621: red and blue colors were
+        swapped in video rendered through WebGL with GSTREAMER_GL enabled.

[Olivier Blin]

Created attachment 289547 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 289547 [details]
Patch

Excellent!

[WebKit Commit Bot]

Comment on attachment 289547 [details]
Patch

Clearing flags on attachment: 289547

Committed r206257: <http://trac.webkit.org/changeset/206257>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.