Bug 138562 – [GStreamer] GstGL support in the video sink

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

See https://bugzilla.gnome.org/show_bug.cgi?id=723674

[Philippe Normand]

Created attachment 242936 [details]
patch

[Philippe Normand]

Created attachment 243045 [details]
patch

[Matthew Waters]

Comment on attachment 243045 [details]
patch

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

Looks good.  There doesn't seem to be any threading issues.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:316
> +                gst_gl_context_set_error(glMemory->context, "Failed to map intermediate memory");

gst_gl_context_set_error() simply GST_WARNING's in the GstGLContext debug category.  I would avoid using it for new code and call GST_WARNING yourself.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:444
> +    gst_caps_set_features(priv->glCaps, 0, glFeatures);

You probably also want to set the format here as well unless you're never going to deal with YUV formats.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:600
> +    gst_gl_handle_set_context(element, context, &sink->priv->display, &sink->priv->context);

shouldn't this be &sink->priv->otherContext or nullptr if you don't care?

[Julien Isorce]

Comment on attachment 243045 [details]
patch

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

It is nice than now with gstgl we can share the texture list between gstgl context and external contexts. Your patch will be a good example! I have put some remarks to open the discussion.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:316
>> +                gst_gl_context_set_error(glMemory->context, "Failed to map intermediate memory");
> 
> gst_gl_context_set_error() simply GST_WARNING's in the GstGLContext debug category.  I would avoid using it for new code and call GST_WARNING yourself.

Also I would go for gst_buffer_map instead of gst_memory_map as you do not actually use the mapInfo. And I would put this gst_buffer_map just before gst_buffer_n_memory. You just want to "protect" the calls that access the memory, even right ?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:319
> +            gboolean copied = gst_gl_memory_copy_into_texture(glMemory, textureID, textureType, size.width(), size.height(), GST_VIDEO_INFO_PLANE_STRIDE(&videoInfo, 0), true /* respecify */);

For future I think you can even try to improve BitmapTextureGL to create for example a BitmapTextureGstGL that own a ref on the gstbuffer. And internally wrap its gl texture from gstgl. Then also improve the textureMapperPool so that you can attach your BitmapTextureGstGL into the pool. In order to avoid this GPU/GPU copy.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:325
> +    }

Generally speaking about this block, correct me if I am wrong but in case of the decoder gives you raw data (I mean system mem data) then this block make the path even worst no ?  Without the block the raw data is uploaded through webkit::texture->updateContents(..), and with this block it is uploaded through "gst_gl_upload_perform_with_buffer" but with the additional GPU-GPU frame copy.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:329
>      GstVideoGLTextureUploadMeta* meta;

Why do you need to big block that call gst_gl_memory_copy_into_texture ? I think glupload should set the meta so that the block below that calls gst_video_gl_texture_upload_meta_upload should handle that for you. Especially for eglimage, it will convert the eglimage directly to the webkit's textureID acquired above.
Ah after going through more of your patch, I think this first big block could only be when decoders output raw data. The second block below about meta upload should be for vaapi and omx cases.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:184
> +        if (glxSharingContext)

glx: so what is the gstdecoder involve in your tests ? vaapi ? or only decoders that output raw frames for now ?
Because in case your decoder gives you raw data, like gst/avdec_h264, then you have 2 options: 1: let webkit do the texture upload as existing, 2:use gstgl+avoid_GPU_GPU_frame_copy.  I think option 2 is better also because you could set in webkit::VideoSinkGstreamer.cpp that it supports same rgb and yuv variants as glimagesink. (for example if decode outputs YUV then you avoid software conversion to RGB)
And in case of vaapidec or omxvideodec, the gst gl upload meta stuffs should do. For these zero-copy paths you only need to keep the context sharing setup.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:472
> +    }

You do not need to map/unmap here.

[Philippe Normand]

Thanks for the reviews :) I won't have time today but I'll update the patch next week according to your comments.

[Zan Dobersek]

Comment on attachment 243045 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:432
> +    GThreadSafeMainLoopSource timeoutSource;
> +    gst_object_ref(sink);
> +    timeoutSource.schedule("[WebKit][gst] GL context wrap", std::function<void()>(std::bind(wrapGLContextCallback, sink)), G_PRIORITY_DEFAULT, [sink] {
> +        gst_object_unref(sink);
> +    });

Would GMainLoopSource::scheduleAndDeleteOnDestroy() fit better here? I don't think GThreadSafeMainLoopSource is required here in any case.

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

Testing the patch I got hit by http://www.w3.org/2010/05/video/mediaevents.html

This the gst (gl related) log:

0:00:04.765747396 12231      0x2633720 INFO               glcontext gstglcontext.c:1338:gst_gl_context_create_thread: Enabling GL context debugging
0:00:04.765781113 12231      0x1b22350 INFO               glcontext gstglcontext.c:907:gst_gl_context_create: gl thread created

** (WebKitWebProcess:12231): CRITICAL **: gst_gl_context_new_wrapped: assertion '(display_api & available_apis) != GST_GL_API_NONE' failed
0:00:04.772857003 12231 0x7f869c0e5e80 DEBUG               glmemory gstglmemory.c:603:_gl_mem_init: new GL texture memory:0x7f86a40f7630 format:4 dimensions:854x480 size:1639680
0:00:04.773751979 12231 0x7f869c0e5e80 DEBUG               glmemory gstglmemory.c:603:_gl_mem_init: new GL texture memory:0x7f86a40f77b0 format:4 dimensions:854x480 size:1639680
0:00:04.776882953 12231 0x7f869c0e5e80 DEBUG               glupload gstglupload.c:754:_upload_find_method:<glupload0> attempting upload with uploader GLMemory
0:00:04.776919275 12231 0x7f869c0e5e80 DEBUG               glupload gstglupload.c:754:_upload_find_method:<glupload0> attempting upload with uploader EGLImage
0:00:04.776927610 12231 0x7f869c0e5e80 DEBUG               glupload gstglupload.c:754:_upload_find_method:<glupload0> attempting upload with uploader UploadMeta
0:00:04.776939099 12231 0x7f869c0e5e80 DEBUG               glupload gstglupload.c:754:_upload_find_method:<glupload0> attempting upload with uploader Raw Data
0:00:04.776960106 12231 0x7f869c0e5e80 DEBUG               glmemory gstglmemory.c:603:_gl_mem_init: new GL texture memory:0x7f86a40f7930 format:4 dimensions:854x480 size:1639680
0:00:18.544331134 12231      0x2633720 ERROR              glcontext gstglcontext.c:1046:_gst_gl_debug_callback:<glcontextglx0> high: GL error from API id:1, GL_INVALID_OPERATION in glBindTexture(non-gen name)
0:00:18.544425112 12231      0x2633720 ERROR              glcontext gstglcontext.c:1046:_gst_gl_debug_callback:<glcontextglx0> high: GL error from API id:1, GL_INVALID_OPERATION in glTexSubImage2D(invalid texture image)

and the last two messages are repeated indefinitely.

[Philippe Normand]

With gst git master?

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

(In reply to comment #8)
> With gst git master?

Yep.

[Matthew Waters]

(In reply to comment #7)
> Testing the patch I got hit by
> http://www.w3.org/2010/05/video/mediaevents.html
> 
> This the gst (gl related) log:

You're missing the beginning of the log which contains some other useful information on how the GL api was chosen and what limited the API.

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

0:00:04.436295684  4752       0x6fc350 INFO               gldisplay gstgldisplay.c:147:gst_gl_display_new: creating a display, user choice:(NULL) (platform: (NULL))

** (WebKitWebProcess:4752): CRITICAL **: gst_gl_context_new_wrapped: assertion '(display_api & available_apis) != GST_GL_API_NONE' failed
0:00:04.436896976  4752       0x6fc350 INFO               glcontext gstglcontext.c:277:gst_gl_context_new: creating a context, user choice:(null)
0:00:04.436936743  4752       0x6fc350 INFO                glwindow gstglwindow.c:213:gst_gl_window_new: creating a window, user choice:(null)
0:00:04.446047276  4752      0x1640450 INFO                glwindow gstglcontext_glx.c:303:gst_gl_context_glx_choose_format: GLX Version: 1.4
0:00:04.446087292  4752      0x1640450 DEBUG               glwindow gstglcontext_glx.c:121:_describe_fbconfig: ID: 117
0:00:04.446093990  4752      0x1640450 DEBUG               glwindow gstglcontext_glx.c:123:_describe_fbconfig: double buffering: 1
0:00:04.446099277  4752      0x1640450 DEBUG               glwindow gstglcontext_glx.c:125:_describe_fbconfig: red: 8
0:00:04.446104281  4752      0x1640450 DEBUG               glwindow gstglcontext_glx.c:127:_describe_fbconfig: green: 8
0:00:04.446108829  4752      0x1640450 DEBUG               glwindow gstglcontext_glx.c:129:_describe_fbconfig: blue: 8
0:00:04.446113135  4752      0x1640450 DEBUG               glwindow gstglcontext_glx.c:131:_describe_fbconfig: alpha: 8
0:00:04.446117791  4752      0x1640450 DEBUG               glwindow gstglcontext_glx.c:133:_describe_fbconfig: depth: 24
0:00:04.446122141  4752      0x1640450 DEBUG               glwindow gstglcontext_glx.c:135:_describe_fbconfig: stencil: 8
0:00:04.446283327  4752      0x1640450 INFO               glcontext gstglcontext.c:1238:gst_gl_context_create_thread: Attempting to create opengl context. user chosen api(s) (any), compiled api support (opengl o
pengl3) display api (any)
0:00:04.446321137  4752      0x1640450 DEBUG               glwindow gstglcontext_glx.c:194:gst_gl_context_glx_create_context:<glwindowx11-0> trying to create a GL 3.1 core context
0:00:04.446910565  4752      0x1640450 INFO               glcontext gstglcontext.c:1248:gst_gl_context_create_thread: created context
0:00:04.447130884  4752      0x1640450 INFO               glcontext gstglcontext.c:1264:gst_gl_context_create_thread: available GL APIs: opengl3
0:00:04.447160429  4752      0x1640450 INFO               glcontext gstglcontext.c:1083:_create_context_info: GL_VERSION: 3.3 (Core Profile) Mesa 10.3.2
0:00:04.447167402  4752      0x1640450 INFO               glcontext gstglcontext.c:1085:_create_context_info: GL_SHADING_LANGUAGE_VERSION: 3.30
0:00:04.447173133  4752      0x1640450 INFO               glcontext gstglcontext.c:1087:_create_context_info: GL_VENDOR: Intel Open Source Technology Center
0:00:04.447179709  4752      0x1640450 INFO               glcontext gstglcontext.c:1089:_create_context_info: GL_RENDERER: Mesa DRI Intel(R) Haswell Mobile 
0:00:04.447242654  4752      0x1640450 DEBUG              glcontext gstglcontext.c:1308:gst_gl_context_create_thread:<glcontextglx0> GL_EXTENSIONS: GL_ARB_ES2_compatibility GL_ARB_ES3_compatibility GL_ARB_base_i
nstance GL_ARB_blend_func_extended GL_ARB_buffer_storage GL_ARB_clear_buffer_object GL_ARB_clear_texture GL_ARB_compressed_texture_pixel_storage GL_ARB_conditional_render_inverted GL_ARB_copy_buffer GL_ARB_copy_
image GL_ARB_conservative_depth GL_ARB_debug_output GL_ARB_depth_buffer_float GL_ARB_depth_clamp GL_ARB_derivative_control GL_ARB_draw_buffers GL_ARB_draw_buffers_blend GL_ARB_draw_elements_base_vertex GL_ARB_dr
aw_instanced GL_ARB_explicit_attrib_location GL_ARB_explicit_uniform_location GL_ARB_fragment_coord_conventions GL_ARB_fragment_shader GL_ARB_framebuffer_object GL_ARB_framebuffer_sRGB GL_ARB_get_program_binary 
GL_ARB_gpu_shader5 GL_ARB_half_float_pixel GL_ARB_half_float_vertex GL_ARB_instanced_arrays GL_ARB_internalformat_query GL_ARB_invalidate_subdata GL_ARB_map_buffer_alignment GL_ARB_map_buffer_range GL_ARB_multi_
bind GL_ARB_occlusion_query2 GL_ARB_pixel_buffer_object GL_ARB_point_sprite GL_ARB_provoking_vertex GL_ARB_robustness GL_ARB_sample_shading GL_ARB_sampler_objects GL_ARB_seamless_cube_map GL_ARB_seamless_cubemap
_per_texture GL_ARB_separate_shader_objects GL_ARB_shader_atomic_counters GL_ARB_shader_bit_encoding GL_ARB_shader_objects GL_ARB_shader_texture_lod GL_ARB_shading_language_packing GL_ARB_shading_language_420pac
k GL_ARB_sync GL_ARB_texture_buffer_object GL_ARB_texture_buffer_object_rgb32 GL_ARB_texture_buffer_range GL_ARB_texture_compression_bptc GL_ARB_texture_compression_rgtc GL_ARB_texture_cube_map_array GL_ARB_text
ure_float GL_ARB_texture_gather GL_ARB_texture_mirror_clamp_to_edge GL_ARB_texture_multisample GL_ARB_texture_non_power_of_two GL_ARB_texture_query_levels GL_ARB_texture_query_lod GL_ARB_texture_rectangle GL_ARB
_texture_rgb10_a2ui GL_ARB_texture_rg GL_ARB_texture_storage GL_ARB_texture_storage_multisample GL_ARB_texture_view GL_ARB_texture_swizzle GL_ARB_timer_query GL_ARB_uniform_buffer_object GL_ARB_vertex_array_bgra
 GL_ARB_vertex_array_object GL_ARB_vertex_attrib_binding GL_ARB_vertex_shader GL_ARB_vertex_type_10f_11f_11f_rev GL_ARB_vertex_type_2_10_10_10_rev GL_ARB_viewport_array GL_EXT_abgr GL_EXT_blend_equation_separate
 GL_EXT_draw_buffers2 GL_EXT_draw_instanced GL_EXT_framebuffer_blit GL_EXT_framebuffer_multisample GL_EXT_framebuffer_multisample_blit_scaled GL_EXT_framebuffer_sRGB GL_EXT_packed_depth_stencil GL_EXT_packed_flo
at GL_EXT_pixel_buffer_object GL_EXT_provoking_vertex GL_EXT_shader_integer_mix GL_EXT_texture_array GL_EXT_texture_compression_dxt1 GL_ANGLE_texture_compression_dxt3 GL_ANGLE_texture_compression_dxt5 GL_EXT_tex
ture_compression_rgtc GL_EXT_texture_compression_s3tc GL_EXT_texture_filter_anisotropic GL_EXT_texture_integer GL_EXT_texture_shared_exponent GL_EXT_texture_snorm GL_EXT_texture_sRGB GL_EXT_texture_sRGB_decode G
L_EXT_texture_swizzle GL_EXT_timer_query GL_EXT_transform_feedback GL_EXT_vertex_array_bgra GL_OES_EGL_image GL_OES_read_format GL_KHR_debug GL_3DFX_texture_compression_FXT1 GL_AMD_conservative_depth GL_AMD_draw
_buffers_blend GL_AMD_seamless_cubemap_per_texture GL_AMD_shader_trinary_minmax GL_AMD_vertex_shader_layer GL_AMD_vertex_shader_viewport_index GL_APPLE_object_purgeable GL_ATI_blend_equation_separate GL_ATI_text
ure_float GL_IBM_multimode_draw_arrays GL_MESA_pack_invert GL_MESA_texture_signed_rgba GL_NV_conditional_render GL_NV_depth_clamp GL_NV_packed_depth_stencil GL_S3_s3tc 
0:00:04.447454397  4752      0x1640450 INFO               glcontext gstglcontext.c:1338:gst_gl_context_create_thread: Enabling GL context debugging
0:00:04.447489672  4752       0x6fc350 INFO               glcontext gstglcontext.c:907:gst_gl_context_create: gl thread created
** (WebKitWebProcess:4752): CRITICAL **: gst_gl_context_new_wrapped: assertion '(display_api & available_apis) != GST_GL_API_NONE' failed
0:00:04.454232922  4752      0x1622850 DEBUG               glmemory gstglmemory.c:603:_gl_mem_init: new GL texture memory:0x7fc75c0f7630 format:4 dimensions:854x480 size:1639680
0:00:04.455053461  4752      0x1622850 DEBUG               glmemory gstglmemory.c:603:_gl_mem_init: new GL texture memory:0x7fc75c0f77b0 format:4 dimensions:854x480 size:1639680
0:00:04.458152552  4752      0x1622850 DEBUG               glupload gstglupload.c:754:_upload_find_method:<glupload0> attempting upload with uploader GLMemory
0:00:04.458197394  4752      0x1622850 DEBUG               glupload gstglupload.c:754:_upload_find_method:<glupload0> attempting upload with uploader EGLImage
0:00:04.458205531  4752      0x1622850 DEBUG               glupload gstglupload.c:754:_upload_find_method:<glupload0> attempting upload with uploader UploadMeta
0:00:04.458216904  4752      0x1622850 DEBUG               glupload gstglupload.c:754:_upload_find_method:<glupload0> attempting upload with uploader Raw Data
0:00:04.458240276  4752      0x1622850 DEBUG               glmemory gstglmemory.c:603:_gl_mem_init: new GL texture memory:0x7fc75c0f7930 format:4 dimensions:854x480 size:1639680
0:00:13.302055589  4752      0x1640450 ERROR              glcontext gstglcontext.c:1046:_gst_gl_debug_callback:<glcontextglx0> high: GL error from API id:1, GL_INVALID_OPERATION in glBindTexture(non-gen name)
0:00:13.302168079  4752      0x1640450 ERROR              glcontext gstglcontext.c:1046:_gst_gl_debug_callback:<glcontextglx0> high: GL error from API id:1, GL_INVALID_OPERATION in glTexSubImage2D(invalid textur
e image)
0:00:13.317916014  4752      0x1640450 ERROR              glcontext gstglcontext.c:1046:_gst_gl_debug_callback:<glcontextglx0> high: GL error from API id:1, GL_INVALID_OPERATION in glBindTexture(non-gen name)
0:00:13.317992554  4752      0x1640450 ERROR              glcontext gstglcontext.c:1046:_gst_gl_debug_callback:<glcontextglx0> high: GL error from API id:1, GL_INVALID_OPERATION in glTexSubImage2D(invalid textur
e image)

[Philippe Normand]

I'm trying to add YUV support to the sink but for my test video the gstglupload seems to fail... The buffers don't have any gl memory attached and the rendered frames look weird, probably because the fallback rendering path I have in the player assumes the frames have only one plane :)

[Philippe Normand]

Created attachment 243761 [details]
I420 rendering gone bad

[Philippe Normand]

Victor, first make sure glimagesink works: gst-launch ... video-sink=glimagesink

[Philippe Normand]

Comment on attachment 243045 [details]
patch

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

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:316
>>> +                gst_gl_context_set_error(glMemory->context, "Failed to map intermediate memory");
>> 
>> gst_gl_context_set_error() simply GST_WARNING's in the GstGLContext debug category.  I would avoid using it for new code and call GST_WARNING yourself.
> 
> Also I would go for gst_buffer_map instead of gst_memory_map as you do not actually use the mapInfo. And I would put this gst_buffer_map just before gst_buffer_n_memory. You just want to "protect" the calls that access the memory, even right ?

Yep

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:319
>> +            gboolean copied = gst_gl_memory_copy_into_texture(glMemory, textureID, textureType, size.width(), size.height(), GST_VIDEO_INFO_PLANE_STRIDE(&videoInfo, 0), true /* respecify */);
> 
> For future I think you can even try to improve BitmapTextureGL to create for example a BitmapTextureGstGL that own a ref on the gstbuffer. And internally wrap its gl texture from gstgl. Then also improve the textureMapperPool so that you can attach your BitmapTextureGstGL into the pool. In order to avoid this GPU/GPU copy.

Yes that's a good idea :)

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:325
>> +    }
> 
> Generally speaking about this block, correct me if I am wrong but in case of the decoder gives you raw data (I mean system mem data) then this block make the path even worst no ?  Without the block the raw data is uploaded through webkit::texture->updateContents(..), and with this block it is uploaded through "gst_gl_upload_perform_with_buffer" but with the additional GPU-GPU frame copy.

Right I focused only on raw data for now.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:329
>>      GstVideoGLTextureUploadMeta* meta;
> 
> Why do you need to big block that call gst_gl_memory_copy_into_texture ? I think glupload should set the meta so that the block below that calls gst_video_gl_texture_upload_meta_upload should handle that for you. Especially for eglimage, it will convert the eglimage directly to the webkit's textureID acquired above.
> Ah after going through more of your patch, I think this first big block could only be when decoders output raw data. The second block below about meta upload should be for vaapi and omx cases.

For va-api at least yes but for OMX I'm not sure... the updateContents() call isn't very efficient so I think I'd rather have a new block for EGL above.

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:184
>> +        if (glxSharingContext)
> 
> glx: so what is the gstdecoder involve in your tests ? vaapi ? or only decoders that output raw frames for now ?
> Because in case your decoder gives you raw data, like gst/avdec_h264, then you have 2 options: 1: let webkit do the texture upload as existing, 2:use gstgl+avoid_GPU_GPU_frame_copy.  I think option 2 is better also because you could set in webkit::VideoSinkGstreamer.cpp that it supports same rgb and yuv variants as glimagesink. (for example if decode outputs YUV then you avoid software conversion to RGB)
> And in case of vaapidec or omxvideodec, the gst gl upload meta stuffs should do. For these zero-copy paths you only need to keep the context sharing setup.

For now I'll leave YUV support aside :)

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:432
>> +    });
> 
> Would GMainLoopSource::scheduleAndDeleteOnDestroy() fit better here? I don't think GThreadSafeMainLoopSource is required here in any case.

That works yeah

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:444
>> +    gst_caps_set_features(priv->glCaps, 0, glFeatures);
> 
> You probably also want to set the format here as well unless you're never going to deal with YUV formats.

Ok!

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:472
>> +    }
> 
> You do not need to map/unmap here.

Ok!

[Philippe Normand]

Created attachment 243795 [details]
patch

[Zan Dobersek]

Comment on attachment 243795 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:308
> +    GstMemory* bufferMemory;

This can be moved into the scope of the gst_buffer_n_memory(buffer) >= 1 if-statement.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:316
> +            GstGLMemory* glMemory = reinterpret_cast<GstGLMemory*>(bufferMemory);

I usually go with 'auto* <variable>' when using reinterpret_cast, but as you prefer really.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:320
> +            gboolean copied = gst_gl_memory_copy_into_texture(glMemory, textureID, textureType, size.width(), size.height(), GST_VIDEO_INFO_PLANE_STRIDE(&videoInfo, 0), true /* respecify */);
> +            if (copied) {

If you find it readable enough, gst_gl_memory_copy_into_texture() call could sit in the if-statement directly.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:385
> +    if (priv->pool)
> +        gst_object_unref(priv->pool);
> +    priv->pool = nullptr;

This ...

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:393
> +    if (priv->upload) {
> +        gst_object_unref(priv->upload);
> +        priv->upload = nullptr;
> +    }

... should match with this, specifically whether priv->upload should be nullified unconditionally, or priv->pool nullified conditionally.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:496
> +    GstAllocator* allocator = nullptr;
> +    GstAllocationParams params;

These two could be declared in the later #if block, where they are used.

[Julien Isorce]

Comment on attachment 243795 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:314
> +    if (gst_buffer_n_memory(buffer) >= 1) {

Could you try to add "!gst_buffer_get_video_gl_texture_upload_meta(buffer) && " ? Because it is the job of the next block.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:571
> +#if GST_GL_HAVE_PLATFORM_EGL

Remove all references to egl/eglimage if you have not tested with eglimage

[Philippe Normand]

Created attachment 244248 [details]
patch

[Philippe Normand]

Created attachment 244249 [details]
patch

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

Comment on attachment 244249 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:309
> +    if (!gst_buffer_map(buffer, &mapInfo, static_cast<GstMapFlags>(GST_MAP_READ | GST_MAP_GL))) {

Mapping might be expensive if the buffer stored in the GPU's memory. When using GstVideoGLTextureUploadMeta, this mapping is no necessary, but here, the meta usage is tried after the buffer mapping.

Also, AFAIK, gst_video_frame_map() is preferred over gst_buffer_map()

I'll post a patch which goes above yours addressing this issue.

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

Created attachment 246263 [details]
Re-order the upload method sequence and use gst_video_frame_map()

This patch goes above attachment 244249 [details]

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

(In reply to comment #14)
> Victor, first make sure glimagesink works: gst-launch ...
> video-sink=glimagesink

A pipeline with glimagesink for me works since comment 7.

And right now I'm using current master of gstreamer/gst-plugins-bad and the same behavior is shown. 

Furthermore, when you have several different videos in the same web page, the same video is reproduced on each video. This is observable here: http://www.quirksmode.org/html5/tests/video.html

[Philippe Normand]

Comment on attachment 244249 [details]
patch

We should try to use glimagesink directly, that would allow us to remove the webkit video-sink all together at some point.

[Philippe Normand]

Another approach would be to use glimagesink but we first need to fix these 2 bugs:

https://bugzilla.gnome.org/show_bug.cgi?id=739681
https://bugzilla.gnome.org/show_bug.cgi?id=704807

[Philippe Normand]

Created attachment 248411 [details]
Experimental support for texture sharing with glimagesink

[Philippe Normand]

Created attachment 249237 [details]
patch

This is a first draft but works on X11/GL and DispmanX/EGL already.

[WebKit Commit Bot]

Attachment 249237 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperGL.h:175:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 249239 [details]
patch

[Gwang Yoon Hwang]

Comment on attachment 249239 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:317
> +    textureGL->wrapGstSample(m_sample);

You don't have to create (or acquire) BitmapTextureGL at this code path.
Because TextureMapperGL have a interface to draw a platform texture directly as you used in BitmapTextureGL::wrapGstSample.
You can get a platform texture and draw it directly in MediaPlayerPrivateGStreamerBase::paintToTextureMapper
By this way, BitmapTextureGL and TextureMapperGL don't have to know about the Gstreamer.

[Philippe Normand]

Created attachment 249319 [details]
patch

Much simpler indeed :)

[Gwang Yoon Hwang]

(In reply to comment #31)
> Created attachment 249319 [details]
> patch
> 
> Much simpler indeed :)

Yes! and this patch reduces merge conflicts with mine :)

[Carlos Garcia Campos]

Comment on attachment 249319 [details]
patch

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

> Source/WebCore/ChangeLog:15
> +        [GStreamer] GstGL support in the video sink
> +        https://bugs.webkit.org/show_bug.cgi?id=138562
> +
> +        Use GStreamer's glimagesink for video rendering instead of our
> +        custom video sink if a recent-enough version of GstGL is found
> +        during the build. When glimagesink is used it passes a texture to
> +        the media player which then wraps it inside a TextureMapper
> +        texture later used for actual rendering.
> +
> +        Using this new code path will allow us to remove our custom sink
> +        entirely in the long term.
> +
> +        No new test, existing media tests cover video rendering already.

Reviewed by line is missing in the changelog.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:354
> +    g_return_if_fail(GST_IS_SAMPLE(sample));

Is it really possible that the client-draw signal is emitted with a NULL or invalid sample?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:360
> +        if (m_sample)
> +            gst_sample_unref(m_sample);
> +        m_sample = gst_sample_ref(sample);

m_sample should probably be a GRefPtr

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:369
> +            if (supportsAcceleratedRendering() && m_player->client().mediaPlayerRenderingCanBeAccelerated(m_player) && client())

I would check client() is not nullptr, before asking the m_player->client().

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:373
> +        }, std::chrono::milliseconds(0), G_PRIORITY_HIGH, nullptr, g_main_context_get_thread_default());

What's the point of scheduling after a 0 delay? Why not using schedule() instead? G_PRIORITY_HIGH is too much, it will affect any other source in the context.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:374
> +        g_cond_wait(&m_drawCondition, &m_drawMutex);

Why does this need to be blocking? what thread is this supposed to be run? We should probably add an ASSERT(isMainThread()) or !isMainThread.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:443
> +    GstBuffer* buffer = gst_sample_get_buffer(m_sample);

You could do this later, it's not needed if caps is NULL or gst_video_info_from_caps returns FALSE.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:457
> +    guint textureID = *(guint *) videoFrame.data[0];

Use C++ style cast for this.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:506
> +        videoSink = m_webkitVideoSink.get();
> +        return videoSink;

why do you need the local variable? can't you just return m_webkitVideoSink.get()?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:82
> +#if USE(GSTREAMER_GL)
> +    void triggerDraw(GstSample*);
> +#endif
>      void triggerRepaint(GstSample*);

Since you are reusing the m_repaintHandler, maybe you could reuse the trigger function too, since they share some code. You could add the specific code of triggerDraw into triggerRepaint inside a #if USE(GSTREAMER_GL) block.

[Philippe Normand]

(In reply to comment #33)
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:354
> > +    g_return_if_fail(GST_IS_SAMPLE(sample));
> 
> Is it really possible that the client-draw signal is emitted with a NULL or
> invalid sample?
> 

No, I'll remove this g_return_if_fail.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:360
> > +        if (m_sample)
> > +            gst_sample_unref(m_sample);
> > +        m_sample = gst_sample_ref(sample);
> 
> m_sample should probably be a GRefPtr
> 

Ok I can do that in a separate patch.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:369
> > +            if (supportsAcceleratedRendering() && m_player->client().mediaPlayerRenderingCanBeAccelerated(m_player) && client())
> 
> I would check client() is not nullptr, before asking the m_player->client().
> 

I checked MediaPlayer, the client is never null there and no one reported a crash about this either.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:373
> > +        }, std::chrono::milliseconds(0), G_PRIORITY_HIGH, nullptr, g_main_context_get_thread_default());
> 
> What's the point of scheduling after a 0 delay? Why not using schedule()
> instead? G_PRIORITY_HIGH is too much, it will affect any other source in the
> context.
> 

Oh yeah that's what I did first, I'll switch back to a simple schedule :)

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:374
> > +        g_cond_wait(&m_drawCondition, &m_drawMutex);
> 
> Why does this need to be blocking? what thread is this supposed to be run?
> We should probably add an ASSERT(isMainThread()) or !isMainThread.
> 

This signal is emitted from the sink streaming thread but the texture can't be used from there, hence the switch to main thread. And it needs to be blocking because the sample emitted is freed from the sink after the signal emission completed.

[Philippe Normand]

Created attachment 249326 [details]
patch

[Carlos Garcia Campos]

(In reply to comment #34)
> (In reply to comment #33)
> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:354
> > > +    g_return_if_fail(GST_IS_SAMPLE(sample));
> > 
> > Is it really possible that the client-draw signal is emitted with a NULL or
> > invalid sample?
> > 
> 
> No, I'll remove this g_return_if_fail.
> 
> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:360
> > > +        if (m_sample)
> > > +            gst_sample_unref(m_sample);
> > > +        m_sample = gst_sample_ref(sample);
> > 
> > m_sample should probably be a GRefPtr
> > 
> 
> Ok I can do that in a separate patch.
> 
> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:369
> > > +            if (supportsAcceleratedRendering() && m_player->client().mediaPlayerRenderingCanBeAccelerated(m_player) && client())
> > 
> > I would check client() is not nullptr, before asking the m_player->client().
> > 
> 
> I checked MediaPlayer, the client is never null there and no one reported a
> crash about this either.

What I meant was doing 

if (supportsAcceleratedRendering() && client() && m_player->client().mediaPlayerRenderingCanBeAccelerated(m_player))

so that we don't even use the media player client if client() is nullptr. But I guess that hardly ever happens, so it's probably not so important.

> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:373
> > > +        }, std::chrono::milliseconds(0), G_PRIORITY_HIGH, nullptr, g_main_context_get_thread_default());
> > 
> > What's the point of scheduling after a 0 delay? Why not using schedule()
> > instead? G_PRIORITY_HIGH is too much, it will affect any other source in the
> > context.
> > 
> 
> Oh yeah that's what I did first, I'll switch back to a simple schedule :)
> 
> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:374
> > > +        g_cond_wait(&m_drawCondition, &m_drawMutex);
> > 
> > Why does this need to be blocking? what thread is this supposed to be run?
> > We should probably add an ASSERT(isMainThread()) or !isMainThread.
> > 
> 
> This signal is emitted from the sink streaming thread but the texture can't
> be used from there, hence the switch to main thread. And it needs to be
> blocking because the sample emitted is freed from the sink after the signal
> emission completed.

Can't you just take a reference of the sample then?

[Philippe Normand]

Created attachment 249327 [details]
patch

[Philippe Normand]

Created attachment 249729 [details]
patch

[Carlos Garcia Campos]

Comment on attachment 249729 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:363
> +            // Rendering should be done from the main thread
> +            // because this is where the GL APIs were initialized.
> +            ASSERT(isMainThread());

I think the important thing here is not that the lambda runs in the main thread, we are scheduling this in the default main context so it will be in the main thread. What matter is that MediaPlayerPrivateGStreamerBase::triggerRepaint is not called in the main thread, because it's going to be blocked waiting for the task to complete. So, I would remove this assert and add ASSERT(!isMainThread()); at the beginning of MediaPlayerPrivateGStreamerBase::triggerRepaint.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:425
> +        RefPtr<BitmapTexture> texture = updateTexture(textureMapper);
> +        if (texture)

This could be if (RefPtr<BitmapTexture> texture = updateTexture(textureMapper)) I think.

[Philippe Normand]

Committed r182227: <http://trac.webkit.org/changeset/182227>