After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 735085 - y4mencode : port y4m encoder to use GstVideoEncoder base class
y4mencode : port y4m encoder to use GstVideoEncoder base class
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2014-08-20 09:38 UTC by RaviKiran
Modified: 2014-09-18 00:00 UTC
See Also:
GNOME target: ---
GNOME version: ---

y4mencoder using GstVideoEncoder (9.57 KB, patch)
2014-08-20 09:38 UTC, RaviKiran
needs-work Details | Review
y4mencoder using GstVideoEncoder (8.66 KB, patch)
2014-08-26 05:08 UTC, RaviKiran
reviewed Details | Review
y4mencoder using GstVideoEncoder (8.67 KB, patch)
2014-09-04 03:47 UTC, RaviKiran
none Details | Review
y4mencoder using GstVideoEncoder (8.73 KB, patch)
2014-09-15 04:48 UTC, RaviKiran
committed Details | Review

Description RaviKiran 2014-08-20 09:38:39 UTC
Created attachment 283953 [details] [review]
y4mencoder using GstVideoEncoder

Currently, y4m encoder inherits GstElementClass.
It should inherit GstVideoEncoder base class.

Patch attached for the proposed changes. Please review.
Comment 1 Thiago Sousa Santos 2014-08-25 19:05:51 UTC
Review of attachment 283953 [details] [review]:

Thanks for taking time to do this port, a few suggestions below.

::: gst/y4m/gsty4mencode.c
@@ +94,3 @@
+  GstElementClass *element_class = GST_ELEMENT_CLASS (klass);
+  GstVideoEncoderClass *venc_class = (GstVideoEncoderClass *) klass;

For consistency, use GST_VIDEO_ENCODER_CLASS or a direct cast for the GST_ELEMENT_CLASS.

@@ +116,3 @@
   filter->sinkpad =
       gst_pad_new_from_static_template (&y4mencode_sink_factory, "sink");

Creating the pads is done in the base class, this pad is just being stored in memory and never will be used.

@@ +174,3 @@
+    GST_ELEMENT_ERROR (y4menc, CORE, NEGOTIATION, (NULL), ("Invalid format"));
+    return ret;

GST_ERROR_OBJECT is a debug log statement, while GST_ELEMENT_ERROR will post an error to the bus. It should only return FALSE here directly and print the log to let base class post the error if needed.

@@ -280,3 @@
-  /* join with data, FIXME, strides are all wrong etc */
-  outbuf = gst_buffer_append (outbuf, buf);

I'd keep using gst_buffer_append here for 3 reasons.

1) Makes it easier to review only the porting patch, as no other changes would be involved.
2) I'm not sure there is a use case that would justify needing special allocated memory for this encoder
3) The code is much simpler with gst_buffer_append

allocating buffers with downstream memory makes, usually, more sense for decoders.

::: gst/y4m/gsty4mencode.h
@@ +47,1 @@
   GstPad *sinkpad,*srcpad;

sinkpad and srcpad aren't needed anymore as the base class already has those.
Comment 2 RaviKiran 2014-08-26 05:08:13 UTC
Created attachment 284476 [details] [review]
y4mencoder using GstVideoEncoder

Thanks for the review. I have included the review suggestions. Please review the new patch.
Comment 3 Thiago Sousa Santos 2014-09-03 23:50:21 UTC
Review of attachment 284476 [details] [review]:

Thanks for the update, seems to only have one minor issue before it can be merged.

::: gst/y4m/gsty4mencode.c
@@ +157,3 @@
+  output_state =
+      gst_video_encoder_set_output_state (encoder,
+      gst_caps_new_empty_simple ("video/x-raw"), state);

This seems wrong, the output caps isn't video/x-raw but "application/x-yuv4mpeg, y4mversion=2" according to the template caps.
Comment 4 RaviKiran 2014-09-04 03:47:17 UTC
Created attachment 285309 [details] [review]
y4mencoder using GstVideoEncoder

Ok. Corrected it. Thanks.
Comment 5 RaviKiran 2014-09-15 04:48:24 UTC
Created attachment 286182 [details] [review]
y4mencoder using GstVideoEncoder

Modified commit message to include bug link.
Comment 6 Thiago Sousa Santos 2014-09-18 00:00:00 UTC
commit 5480f6d2dd49b1168c0af68f31af5b08c96552a8
Author: Ravi Kiran K N <>
Date:   Mon Sep 15 09:08:18 2014 +0530

    y4menc: port y4menc to use GstVideoEncoder base class