What pipeline are you using to test this? Doesn't seem to work here.

Also I get a warning: GStreamer-WARNING **: gstpad.c:4618:store_sticky_event:<pnmdec0:src> Sticky event misordering, got 'segment' before 'caps'

Do you get that, too?

Another thing, please avoid erasing empty lines or doing code style changes that are unrelated to your patch. Makes it harder to review and track changes.

I am using 
gst-launch-1.0 filesrc location=MARBLES.PBM ! pnmdec ! filesink location=out.pbm

I get raw output from this. I add PBM header to this raw output and view it in gimp to see if the output if fine. It worked well. Encoding it back to PNM may not work as PNM Encoder may not support Bitmap encoding yet. I can add that support if required.

I am away from my setup, I will test again tomorrow and update on the warning.

Will take care to retain empty lines. To debug I had added printf statements. When I deleted them at the end may be few empty lines got deleted. Sorry for that.

I was trying to directly display the decoded data using:

"gst-launch-1.0 filesrc ! pnmdec ! imagefreeze ! autovideosink"

It seems that it didn't even produce data here.

I am getting the warning you have mentioned.
I tried with original pnmdec without my patch, even with that notice the same thing. Is there anyway I can fix this ?

Setting pipeline to PAUSED ...
Pipeline is PREROLLING ...

(gst-launch-1.0:5286): GStreamer-WARNING **: gstpad.c:4618:store_sticky_event:<pnmdec0:src> Sticky event misordering, got 'segment' before 'caps'

(gst-launch-1.0:5286): GStreamer-WARNING **: gstpad.c:4618:store_sticky_event:<filesink0:sink> Sticky event misordering, got 'segment' before 'caps'
Pipeline is PREROLLED ...

You would need to delay all events that must be after the caps event to actually come after the caps event. Or just port the decoder to GstVideoDecoder.

Created attachment 277287 [details] [review]
pnmdec: Support to decode PBM BInary files

Attached is the patch to decode PBM Binary file. It is tested with below pipeline and is working well. It was not possible to render Binary data using the below pipeline, so added code to convert binary to Gray to provide the output.

Still Sticky event warning is seen. Working on moving to GstVideoDecoder. Will submit that as a separate patch

gst-launch-1.0 filesrc location=MARBLES.PBM ! pnmdec ! videoconvert ! imagefreeze ! autovideosink

Request to please review this patch.

Review of attachment 277287 [details] [review]:

Your patch is correct and just needs some minor fixes, the issues are all with the code that was already in pnmdec.

Because of the current state of the code I'd strongly suggest that you first get it ported to videodecoder, then we sort out these minor issues to make it properly work with already existing scenarios and then we get the PBM support on top of it. What do you think? This patch of yours would mostly remain unmodified and you would just have to copy and paste the code to the new ported version.

::: gst/pnm/gstpnmdec.c
@@ +103,3 @@
+  gint bytes, i;
+  guchar *bufdatabin, *bufdatagray;
+  GstMemory *smemory, *dmemory;

You don't need to use GstMemory here you can directly map with gst_buffer_map/_unmap, it should make the code simpler.

@@ +105,3 @@
+  GstMemory *smemory, *dmemory;
+
+  GstMapInfo sinfo, dinfo;

Would you mind elaborating this comment a bit more? Mention that the image is a binary (black/white) image and there is no support for it in gstvideo yet?

Put a TODO into the comment so someone know that there is some work pending here. The ideal solution would be to have binary image support in gstvideo in gst-plugins-base and have this conversion happening in videoconvert.

Anyway let's do this by steps so first we get it working here and then we can improve with moving it to -base and videoconvert.

@@ +162,1 @@
   }

This part of your patch is correct with the given state of the code but this whole if(gst_buffer_get_size ...) seems misplaced.

Imagine that you already have some data in s->buf and you receive a new buffer that is exactly s->size. It would get processed before the data that it received before and would likely lead to errors. This is not the usual use case for pnm data but gstreamer elements don't know much about their surroundings and should be resilient to multiple scenarios when possible.

@@ +184,3 @@
+      memset (&s->mngr, 0, sizeof (GstPnmInfoMngr));
+      s->size = 0;
+      s->size = 0;

This is where the buffer size should be checked. And the check should be for >= s->size instead of just for == s->size. (Again, this is not from your patch, it is something that was already there).

The reason is that you might receive more data than you need and pnmdec would process the only s->size data and leave the remaining data there to be merged with any subsequent data it would receive.

This == here is what prevents it from working for me.

Created attachment 278574 [details] [review]
pnmdec: Added support for PBM Decoding

Updated the patch to work with gstVideoDecoder.
Have tested and works well.

Request to please review

Thanks for the patch. Please notice the commit message format for the next contributions:

elementname: short description

commit ed1a6641718320972658da3bbccfeaf5329a86ee
Author: Sanjay NM <sanjay.nm@samsung.com>
Date:   Tue Jun 17 12:50:17 2014 +0530

    pnmdec: Added PBM Support
    
    https://bugzilla.gnome.org/show_bug.cgi?id=730523

plz fix Makefile.am for dependency on GstAdapter

libgstpnm_la_LIBADD = $(GST_PLUGINS_BASE_LIBS) $(GST_BASE_LIBS) $(GST_LIBS) -lgstvideo-@GST_API_VERSION@