GNOME Bugzilla – Bug 735085
y4mencode : port y4m encoder to use GstVideoEncoder base class
Last modified: 2014-09-18 00:00:35 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.
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.
Created attachment 284476 [details] [review] y4mencoder using GstVideoEncoder Thanks for the review. I have included the review suggestions. Please review the new patch.
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.
Created attachment 285309 [details] [review] y4mencoder using GstVideoEncoder Ok. Corrected it. Thanks.
Created attachment 286182 [details] [review] y4mencoder using GstVideoEncoder Modified commit message to include bug link.
commit 5480f6d2dd49b1168c0af68f31af5b08c96552a8 Author: Ravi Kiran K N <ravi.kiran@samsung.com> Date: Mon Sep 15 09:08:18 2014 +0530 y4menc: port y4menc to use GstVideoEncoder base class https://bugzilla.gnome.org/show_bug.cgi?id=735085