[Bug 730523] pnmdec: PBM (Bitmap) decoding support is not present

[GStreamer (bugzilla.gnome.org)]

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

Thiago Sousa Santos <thiagossantos> changed:

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

--- Comment #8 from Thiago Sousa Santos <thiagossantos at gmail.com> 2014-05-27 14:30:02 UTC ---
Review of attachment 277287:
 --> (https://bugzilla.gnome.org/review?bug=730523&attachment=277287)

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.

-- 
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.