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 731943 - pnmenc: PNM Encoder needs to use gstVideoEncoder as base class
pnmenc: PNM Encoder needs to use gstVideoEncoder as base class
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-06-20 04:22 UTC by Sanjay NM
Modified: 2014-07-22 09:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
pnmenc: PNM Encoder needs to use gstVideoEncoder as base class (13.43 KB, patch)
2014-06-20 04:22 UTC, Sanjay NM
needs-work Details | Review
pnmenc: Patch to use gstVideoEncoder class (2.62 KB, patch)
2014-06-27 14:32 UTC, Sanjay NM
none Details | Review
pnmenc: Ported PNM Encoder to use gstvideodecoder class (13.66 KB, patch)
2014-07-22 07:29 UTC, Sanjay NM
committed Details | Review

Description Sanjay NM 2014-06-20 04:22:52 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
Comment 1 Thiago Sousa Santos 2014-06-25 17:34:19 UTC
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.
Comment 2 Sanjay NM 2014-06-27 14:32:02 UTC
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
Comment 3 Thiago Sousa Santos 2014-07-21 21:00:35 UTC
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.
Comment 4 Sanjay NM 2014-07-22 07:29:48 UTC
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)
Comment 5 Thiago Sousa Santos 2014-07-22 09:37:49 UTC
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