Bug 132247

Summary: [GStreamer] Use GstMetaVideo
Product: WebKit Reporter: Víctor M. Jáquez L. <vjaquez>
Component: WebKit Misc.Assignee: Víctor M. Jáquez L. <vjaquez>
Severity: Normal CC: agomez, calvaris, cgarcia, commit-queue, eric.carlson, glenn, gns, jer.noble, menard, mrobinson, nicolas, philipj, pnormand, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
WebKitGTK+-2.2.x backport none

Description Víctor M. Jáquez L. 2014-04-28 03:19:14 PDT
In WebKitVideoSink we announce the usage of GstMetaVideo, but we do not use it when handling the video frames. This might break some decoders and filters that rely on buffer's meta, rather that static caps structures.

This patch will enable the use of GstMetaVideo if it is available in the buffer.
Comment 1 Víctor M. Jáquez L. 2014-04-28 03:32:30 PDT
Created attachment 230288 [details]
Comment 2 Nicolas Dufresne 2014-04-28 06:52:25 PDT
Well, this is not quite right. I would strongly recommend using the GstVideoFrame API, gst_video_frame_map()/gst_video_frame_unmap(). This way you don't have to worry how to map a buffer base on whether or not it has GstVideoMeta.
Comment 3 Philippe Normand 2014-04-28 06:57:25 PDT
Oh yes that would be much better indeed. Care to update the patch Victor?
Comment 4 Víctor M. Jáquez L. 2014-04-28 07:04:35 PDT
(In reply to comment #3)
> Oh yes that would be much better indeed. Care to update the patch Victor?

Comment 5 Víctor M. Jáquez L. 2014-04-30 06:48:45 PDT
Created attachment 230476 [details]
Comment 6 Philippe Normand 2014-04-30 08:11:13 PDT
Comment on attachment 230476 [details]

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

> Source/WebCore/ChangeLog:19
> +        Also, since either EFL and GTK+ ports use version of glib greater than
> +        2.31, the guards for it were removed.

This is quite unrelated, I'd rather do this in a separate patch, also cleaning the similar ones in MediaPlayerPrivateGStreamerBase.cpp
Comment 7 Nicolas Dufresne 2014-04-30 08:22:48 PDT
> +    GstVideoInfo videoInfo;
> +    if (!gst_video_info_from_caps(&videoInfo, caps))
> +        return;

You should ideally call gst_video_info_init(&videoInfo), since info_from_caps() will only set what it finds in the caps, leaving random value in C, and non-default in C++. Will help if you run some memory checker. (found 2 times)

> +    int stride = GST_VIDEO_FRAME_PLANE_STRIDE(&m_videoFrame, 0);
Not sure if you guys uses these, but an assert that there is only 1 plane would be a nice to have. So if a dev wants to add planar formats support, it won't endup doing weird things silently.

These are details, looks good, nice work !
Comment 8 Víctor M. Jáquez L. 2014-04-30 10:37:26 PDT
Created attachment 230492 [details]
Comment 9 Víctor M. Jáquez L. 2014-04-30 10:49:29 PDT
Created attachment 230493 [details]
Comment 10 Philippe Normand 2014-04-30 11:41:41 PDT
Comment on attachment 230493 [details]

Ok! Thanks to both Nicolas and Victor :)
Comment 11 WebKit Commit Bot 2014-04-30 16:18:08 PDT
Comment on attachment 230493 [details]

Clearing flags on attachment: 230493

Committed r168060: <http://trac.webkit.org/changeset/168060>
Comment 12 WebKit Commit Bot 2014-04-30 16:18:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Víctor M. Jáquez L. 2014-05-05 06:18:32 PDT
Created attachment 230823 [details]
WebKitGTK+-2.2.x backport