Bug 132671 – [GStreamer] A video element isn't drawn onto the canvas.

• Component: Media
• Status: RESOLVED FIXED

[Seongjun Kim]

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.

[Seongjun Kim]

Created attachment 231047 [details]
example

[Seongjun Kim]

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?

[Seongjun Kim]

Created attachment 231048 [details]
Patch

[Víctor M. Jáquez L.]

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

[Seongjun Kim]

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

[Víctor M. Jáquez L.]

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)

[Víctor M. Jáquez L.]

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)

[Víctor M. Jáquez L.]

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)

[Víctor M. Jáquez L.]

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)

[Víctor M. Jáquez L.]

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)

[Philippe Normand]

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

[Víctor M. Jáquez L.]

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

[Seongjun Kim]

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

[Philippe Normand]

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

[Philippe Normand]

(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

[Philippe Normand]

Ping? :)

[Seongjun Kim]

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?

[Philippe Normand]

To work around Xvbf errors you can set the USE_NATIVE_XDISPLAY=1 env var when running the layout tests.

[Seongjun Kim]

Fortunately, Xvfb problem is sovled after rebasing latest revision!

I am runing layout test now :)

[Philippe Normand]

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 :/

[Martin Robinson]

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

[Philippe Normand]

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

[Philippe Normand]

Created attachment 249912 [details]
patch

Likely more iterations are needed, I'm not very familiar with the
GraphicsContext stuff :)

[Gwang Yoon Hwang]

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.

[Philippe Normand]

Created attachment 249918 [details]
patch

[Carlos Garcia Campos]

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

[Philippe Normand]

Created attachment 249924 [details]
patch

[Philippe Normand]

Created attachment 249926 [details]
patch

[Philippe Normand]

Created attachment 249927 [details]
patch

[Michael Catanzaro]

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

[Philippe Normand]

This one no longer works... I'll have to dive in again.

[Philippe Normand]

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

[Philippe Normand]

Committed r194528: <http://trac.webkit.org/changeset/194528>