[Webkit-unassigned] [Bug 138562] [GStreamer] GstGL support in the video sink

[bugzilla-daemon at webkit.org]

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

--- Comment #15 from Philippe Normand <pnormand at igalia.com> ---
Comment on attachment 243045
  --> https://bugs.webkit.org/attachment.cgi?id=243045
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!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141229/29bacbfe/attachment-0002.html>