GNOME Bugzilla – Bug 731943
pnmenc: PNM Encoder needs to use gstVideoEncoder as base class
Last modified: 2014-07-22 09:38:01 UTC
Created attachment 278813 [details] [review] pnmenc: PNM Encoder needs to use gstVideoEncoder as base class PNM Encoder has a FIXME to use gstVideoEncoder class. Recently decoder was ported to use gstVideoDecoder Class. This patch ports the PNMEncoder to use gstVideoEncoder Class Request to review
Review of attachment 278813 [details] [review]: Thanks for this patch, it is nearly ready. Just some minor nitpicks pointed below. ::: gst/pnm/gstpnmenc.c @@ +203,3 @@ + size = size * 4; + size += strlen (header); + You can use one of the gst_video_encoder_allocate_output_*() functions here. @@ +206,3 @@ + } else { + size += strlen (header); + /* Per component 4 bytes are used in case of ASCII encoding */ Same here. @@ +213,3 @@ + goto done; + } + if (gst_buffer_map (frame->input_buffer, &imap, GST_MAP_READ) == FALSE) { You should unmap the output_buffer here to avoid keeping it mapped. @@ +226,3 @@ + if (pnmenc->info.type == GST_PNM_TYPE_PIXMAP) { + o_rowstride = 3 * pnmenc->info.width; + memcpy (omap.data, header, strlen (header)); You can get the rowstride from the video format with GST_VIDEO_FRAME_COMP_STRIDE IIRC.
Created attachment 279405 [details] [review] pnmenc: Patch to use gstVideoEncoder class Thanks a lot for your valuable feedback. Thanks for letting me know more on the APIs, its a good learning for me. As suggested have made the necessary changes and resubmitting the patch
A few things to fix: 1) When submitting a new version of a patch, please resubmit the full patch instead of an amendment patch, this makes easier for reviewers to try and push. 2) There seems to be some memory corruption in this ported version as the command: "gst-launch-1.0 videotestsrc num-buffers=5 ! pnmenc ! multifilesink location=/tmp/test_%02d.pnm" Aborts with some issue inside malloc. Using valgrind should easily point out the failure. Try to run valgrind with something like: G_DEBUG=gc-friendly G_SLICE=always-malloc valgrind --trace-children=yes gst-launch-1.0 videotestsrc num-buffers=5 ! pnmenc ! multifilesink location=/tmp/test_%02d.pnm and look for "Invalid write" entries.
Created attachment 281357 [details] [review] pnmenc: Ported PNM Encoder to use gstvideodecoder class While copying data, header size was not excluded. This was causing memory overrun. Have fixed it. Have run valgrind too to verify. ==26032== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Thanks for the quick update. Pushed the patch. commit 00b33e2068c2d4d0344452181274c2a08ac1a2be Author: Sanjay NM <sanjay.nm@samsung.com> Date: Tue Jul 22 12:42:36 2014 +0530 pnmenc: Port PNM Encoder to use GstVideoEncoder Class https://bugzilla.gnome.org/show_bug.cgi?id=731943