[Bug 735085] y4mencode : port y4m encoder to use GstVideoEncoder base class

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=735085
  GStreamer | gst-plugins-good | git

Thiago Sousa Santos <thiagossantos> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #283953|none                        |needs-work
             status|                            |

--- Comment #1 from Thiago Sousa Santos <thiagossantos at gmail.com> 2014-08-25 19:05:51 UTC ---
Review of attachment 283953:
 --> (https://bugzilla.gnome.org/review?bug=735085&attachment=283953)

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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.