Bug 132671

Summary: [GStreamer] A video element isn't drawn onto the canvas.
Product: WebKit Reporter: Seongjun Kim <isair>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: agomez, calvaris, cgarcia, commit-queue, eric.carlson, glenn, gns, isair, jer.noble, menard, mrobinson, noam, philipj, pnormand, sergio, vjaquez, yoon
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
example
none
Patch
none
patch
none
patch
none
patch
none
patch
none
patch mcatanzaro: review+

Description Seongjun Kim 2014-05-07 22:16:31 PDT
void drawImage(in HTMLVideoElement image, in double dx, in double dy, in optional double dw, in double dh);
  void drawImage(in HTMLVideoElement image, in double sx, in double sy, in double sw, in double sh, in double dx, in double dy, in double dw, in double dh);

drawImage for a video elemeht didn't work.


See attached example.
Comment 1 Seongjun Kim 2014-05-07 22:17:10 PDT
Created attachment 231047 [details]
example
Comment 2 Seongjun Kim 2014-05-07 22:59:58 PDT
This is a regression of below.
https://bugs.webkit.org/show_bug.cgi?id=86410


(WebCore::MediaPlayerPrivateGStreamerBase::paint) is used for 'drawImage'.
But when accelerated compositing is used, this function didn't work.


This exception for AC is still needed?
Comment 3 Seongjun Kim 2014-05-07 23:28:57 PDT
Created attachment 231048 [details]
Patch
Comment 4 Víctor M. Jáquez L. 2014-05-08 02:11:07 PDT
(In reply to comment #2)
> This is a regression of below.
> https://bugs.webkit.org/show_bug.cgi?id=86410
> 
> 
> (WebCore::MediaPlayerPrivateGStreamerBase::paint) is used for 'drawImage'.
> But when accelerated compositing is used, this function didn't work.
> 
> 
> This exception for AC is still needed?

In AC the video is painted using GL textures. Look at 

void MediaPlayerPrivateGStreamerBase::paintToTextureMapper(TextureMapper* textureMapper, const FloatRect& targetRect, const TransformationMatrix& matrix, float opacity)

If AC is not used, the painting is done through Cairo, and it's when this method is used.

In AC we wait for the TextureMapper update to paint the video buffer in the texture.

Removing this guard will mean to mix approaches, and that's not sane.
Comment 5 Seongjun Kim 2014-05-08 02:33:47 PDT
(In reply to comment #4)
> (In reply to comment #2)
> > This is a regression of below.
> > https://bugs.webkit.org/show_bug.cgi?id=86410
> > 
> > 
> > (WebCore::MediaPlayerPrivateGStreamerBase::paint) is used for 'drawImage'.
> > But when accelerated compositing is used, this function didn't work.
> > 
> > 
> > This exception for AC is still needed?
> 
> In AC the video is painted using GL textures. Look at 
> 
> void MediaPlayerPrivateGStreamerBase::paintToTextureMapper(TextureMapper* textureMapper, const FloatRect& targetRect, const TransformationMatrix& matrix, float opacity)
> 
> If AC is not used, the painting is done through Cairo, and it's when this method is used.
> 
> In AC we wait for the TextureMapper update to paint the video buffer in the texture.
> 
> Removing this guard will mean to mix approaches, and that's not sane.



Did you mean that 'drawImage' should use 'paintToTextureMapper' instead of 'paint' in AC?
Comment 6 Víctor M. Jáquez L. 2014-05-08 02:33:58 PDT
Comment on attachment 231048 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-350
> -        return;

What about, instead of removing this code, and instead or a simple return,  we do

    if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player) && client()) {
        client()->setPlatformLayerNeedsDisplay();
        return;
    }

just as in void MediaPlayerPrivateGStreamerBase::triggerRepaint(GstBuffer* buffer)
Comment 7 Víctor M. Jáquez L. 2014-05-08 02:34:01 PDT
Comment on attachment 231048 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-350
> -        return;

What about, instead of removing this code, and instead or a simple return,  we do

    if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player) && client()) {
        client()->setPlatformLayerNeedsDisplay();
        return;
    }

just as in void MediaPlayerPrivateGStreamerBase::triggerRepaint(GstBuffer* buffer)
Comment 8 Víctor M. Jáquez L. 2014-05-08 02:34:02 PDT
Comment on attachment 231048 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-350
> -        return;

What about, instead of removing this code, and instead or a simple return,  we do

    if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player) && client()) {
        client()->setPlatformLayerNeedsDisplay();
        return;
    }

just as in void MediaPlayerPrivateGStreamerBase::triggerRepaint(GstBuffer* buffer)
Comment 9 Víctor M. Jáquez L. 2014-05-08 02:34:06 PDT
Comment on attachment 231048 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-350
> -        return;

What about, instead of removing this code, and instead or a simple return,  we do

    if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player) && client()) {
        client()->setPlatformLayerNeedsDisplay();
        return;
    }

just as in void MediaPlayerPrivateGStreamerBase::triggerRepaint(GstBuffer* buffer)
Comment 10 Víctor M. Jáquez L. 2014-05-08 02:34:13 PDT
Comment on attachment 231048 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:-350
> -        return;

What about, instead of removing this code, and instead or a simple return,  we do

    if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player) && client()) {
        client()->setPlatformLayerNeedsDisplay();
        return;
    }

just as in void MediaPlayerPrivateGStreamerBase::triggerRepaint(GstBuffer* buffer)
Comment 11 Philippe Normand 2014-05-13 02:45:37 PDT
(In reply to comment #1)
> Created an attachment (id=231047) [details]
> example

Thanks for the patch, it indeed works :) Can a layout test be unskipped thanks to the fix? 

(In reply to comment #4)
> (In reply to comment #2)
> > This is a regression of below.
> > https://bugs.webkit.org/show_bug.cgi?id=86410
> > 
> > 
> > (WebCore::MediaPlayerPrivateGStreamerBase::paint) is used for 'drawImage'.
> > But when accelerated compositing is used, this function didn't work.
> > 
> > 
> > This exception for AC is still needed?
> 
> In AC the video is painted using GL textures. Look at 
> 
> void MediaPlayerPrivateGStreamerBase::paintToTextureMapper(TextureMapper* textureMapper, const FloatRect& targetRect, const TransformationMatrix& matrix, float opacity)
> 
> If AC is not used, the painting is done through Cairo, and it's when this method is used.
> 
> In AC we wait for the TextureMapper update to paint the video buffer in the texture.
> 
> Removing this guard will mean to mix approaches, and that's not sane.

::paint is no longer used indeed excepted in this case, whatever if AC is disabled or not. I think this patch is ok.
Comment 12 Víctor M. Jáquez L. 2014-05-14 01:09:10 PDT
(In reply to comment #11)
> ::paint is no longer used indeed excepted in this case, whatever if AC is disabled or not. I think this patch is ok.

But it makes me wonder, what if the negotiated buffer cannot download the raw image, relying in gst_video_gl_texture_upload_meta_upload(). Or perhaps in the future, when we will be able to negotiate buffers as EGLImages, how could Cairo handle it.

Perhaps I'm not understanding the issue correctly, but I think that ::paint shall be handled by the texturemapper too.
Comment 13 Seongjun Kim 2014-05-14 01:25:21 PDT
(In reply to comment #11) 
> Thanks for the patch, it indeed works :) Can a layout test be unskipped thanks to the fix? 


I tried layouttest, but there are many crashed tests with following message.
 Xlib:  extension "NV-GLX" missing on display ":1".

Would you mind helping for me?
Comment 14 Philippe Normand 2014-05-14 01:32:53 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > ::paint is no longer used indeed excepted in this case, whatever if AC is disabled or not. I think this patch is ok.
> 
> But it makes me wonder, what if the negotiated buffer cannot download the raw image, relying in gst_video_gl_texture_upload_meta_upload(). Or perhaps in the future, when we will be able to negotiate buffers as EGLImages, how could Cairo handle it.
> 
> Perhaps I'm not understanding the issue correctly, but I think that ::paint shall be handled by the texturemapper too.

I think we're going a bit off-topic here but if we add EGLImage support it will be a new memory in the buffer and it should be possible to do a zero-copy of the texture into a Cairo-GL surface (I think/hope :))
Comment 15 Philippe Normand 2014-05-14 01:35:11 PDT
(In reply to comment #13)
> (In reply to comment #11) 
> > Thanks for the patch, it indeed works :) Can a layout test be unskipped thanks to the fix? 
> 
> 
> I tried layouttest, but there are many crashed tests with following message.
>  Xlib:  extension "NV-GLX" missing on display ":1".
> 
> Would you mind helping for me?

Ping me on IRC
Comment 16 Philippe Normand 2014-06-06 00:04:18 PDT
Ping? :)
Comment 17 Seongjun Kim 2014-06-09 02:28:15 PDT
I reinstall ubuntu 14.04 however, I still fail to run layout-test with Xvfb errors... I don't know what is wrong.


How about applying this patch first or someone continue this work?
Comment 18 Philippe Normand 2014-11-25 00:09:53 PST
To work around Xvbf errors you can set the USE_NATIVE_XDISPLAY=1 env var when running the layout tests.
Comment 19 Seongjun Kim 2014-11-26 01:58:40 PST
Fortunately, Xvfb problem is sovled after rebasing latest revision!

I am runing layout test now :)
Comment 20 Philippe Normand 2015-03-30 09:40:14 PDT
This is going to need more work when we switch to glimagesink for rendering. ::paint will need to handle the texture stored in the sample and somehow render it with Cairo, if that's possible.

Worst case we'll need to download the bitmap data from the GPU to CPU :/
Comment 21 Martin Robinson 2015-03-30 10:11:28 PDT
(In reply to comment #20)
> This is going to need more work when we switch to glimagesink for rendering.
> ::paint will need to handle the texture stored in the sample and somehow
> render it with Cairo, if that's possible.
> 
> Worst case we'll need to download the bitmap data from the GPU to CPU :/

I'm not sure it makes sense to use the GL path of GStreamer if accelerated compositing isn't enabled. One thing we can start considering though is to deprecate the non-accelerated path entirely.
Comment 22 Philippe Normand 2015-03-31 00:10:05 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > This is going to need more work when we switch to glimagesink for rendering.
> > ::paint will need to handle the texture stored in the sample and somehow
> > render it with Cairo, if that's possible.
> > 
> > Worst case we'll need to download the bitmap data from the GPU to CPU :/
> 
> I'm not sure it makes sense to use the GL path of GStreamer if accelerated
> compositing isn't enabled. One thing we can start considering though is to
> deprecate the non-accelerated path entirely.

I agree, perhaps we should wait the gst 1.6 release though.

Anyway for this specific issue it seems we could directly render the gst texture to a cairo-gl surface.
Comment 23 Philippe Normand 2015-04-01 03:56:06 PDT
Created attachment 249912 [details]
patch

Likely more iterations are needed, I'm not very familiar with the
GraphicsContext stuff :)
Comment 24 Gwang Yoon Hwang 2015-04-01 04:30:42 PDT
Comment on attachment 249912 [details]
patch

I think it is much safe to test ENABLE(ACCELERATED_2D_CANVAS) with a USE(CAIRO) to ensure we are using cairo-gl.
Comment 25 Philippe Normand 2015-04-01 06:28:21 PDT
Created attachment 249918 [details]
patch
Comment 26 Carlos Garcia Campos 2015-04-01 07:03:27 PDT
Comment on attachment 249918 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:83
> +#if USE(CAIRO)
> +#include <cairo-gl.h>
> +#endif

We can compile with cairo, but without cairo gl. We should expose those to the build somehow, or use ENABLE(ACCELERATED_2D_CANVAS) && USE(CAIRO)

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:579
> +PassNativeImagePtr MediaPlayerPrivateGStreamerBase::nativeImageForCurrentTime()
> +{
> +#if !USE(GSTREAMER_GL)
> +    return nullptr;
> +#endif

This function is called inside a #if USE(GSTREAMER_GL) block, right? I think we could just defined it only in such case

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:595
> +    if (!GST_IS_SAMPLE(m_sample.get()))
> +        return nullptr;
> +
> +    GstCaps* caps = gst_sample_get_caps(m_sample.get());
> +    if (!caps)
> +        return nullptr;
> +
> +    GstVideoInfo videoInfo;
> +    gst_video_info_init(&videoInfo);
> +    if (!gst_video_info_from_caps(&videoInfo, caps))
> +        return nullptr;

It seems to me this code is duplicated in several places already, maybe we could move it to a helper function, something like bool getSampleVideoInfo(GstSmaple* sample, GstVideoInfo& videoInfo);

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:602
> +#if USE(CAIRO)

Do we need previous stuff (gst_video_frame_map) when not using cairo?

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

Why getting the sharing context twice? Can we get the context first and then calling context->makeContextCurrent() ?

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

Why FloatSize? cairo expects integers
Comment 27 Philippe Normand 2015-04-01 09:17:33 PDT
Created attachment 249924 [details]
patch
Comment 28 Philippe Normand 2015-04-01 09:45:10 PDT
Created attachment 249926 [details]
patch
Comment 29 Philippe Normand 2015-04-01 10:06:39 PDT
Created attachment 249927 [details]
patch
Comment 30 Michael Catanzaro 2016-01-02 11:00:44 PST
Comment on attachment 249927 [details]
patch

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

I only spot-checked this for obvious C++ errors, since not familiar with this code, but most of these changes only execute when two experimental build options are enabled (ACCELERATED_2D_CANVAS and USE_GSTREAMER_GL), neither of which we are likely to turn on for end users anytime soon, so it seems silly for this to be stalled on request queue any longer.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:275
> +            // The destination rectangle will have it's width and height already reversed for the orientation of

it's -> its
Comment 31 Philippe Normand 2016-01-04 01:33:07 PST
This one no longer works... I'll have to dive in again.
Comment 32 Philippe Normand 2016-01-04 03:14:01 PST
(In reply to comment #31)
> This one no longer works... I'll have to dive in again.

I had the threaded compositor disabled, it works if I enable it. Will land the patch.
Comment 33 Philippe Normand 2016-01-04 03:15:49 PST
Committed r194528: <http://trac.webkit.org/changeset/194528>