Review of attachment 278132 [details] [review]:

Thanks for the patch, it still needs a few adjustments before it can be merged.

Mostly you should remove the chain functions and do the decoding from the handle_frame function.

You can take a look at gst-plugins-good/ext/libpng/gstpngdec.c for a good example of _parse and _handle_frame functions. If you feel that the documentation can be improved to make it easier to understand how to use it, please let us know how to do so.

::: gst/pnm/gstpnmdec.c
@@ +134,2 @@
   }
 }

You shouldn't need to push buffers from your class anymore, the base class will handle that for you when you call _finish_frame.

@@ +258,3 @@
+
+  frame->output_buffer = s->buf;
+  return gst_video_decoder_finish_frame (GST_VIDEO_DECODER (s), frame);

This is where you should do the decoding and then call finish_frame with the output buffer set, instead of having the _chain functions doing the decoding.

The steps that the videodecoder class do are:

1) Get the input data and put into the adapter and then call your _parse function so that you do the parsing and extract the input PNM buffers, one at a time.
2) Your _parse function will analyze the input data and decide how many bytes form a input buffer. It should call "gst_video_decoder_add_to_frame" to add bytes from the input adapter to the current input buffer. When it has a full input frame added, you should call _have_frame.
3) At this point, the videodecoder will get this amount of data and call your _handle_frame with this input buffer, now you should do the decoding and set it to the output of the VideoFrame struct, then call _finish_frame so that the base class gets this output data and pushes downstream.

@@ +335,2 @@
     r = gst_pnmdec_chain_raw (s, src, data);
+  }

You shouldn't call the _chain functions that would push the buffers themselves. Instead, just use the gst_video_decoder_add_to_frame and gst_video_decoder_have_frame when you have a full input frame added. Then do the decoding on the handle_frame function.

Created attachment 278196 [details] [review]
pnmdec: Ported PNM Decoder to use gstVideoDecoder Class

Thanks for the detailed review.

Have incorporated all the review comments except using gst_video_decoder_add_to_frame to add data to frame. Reason for this is, in PNM there are no markers for data, especially for ASCII data its not possible to know how many bytes should be read to form a frame. So have created a function gst_pnmdec_add_to_frame. This keeps adding data to s->buf and finally this data is given to the frame.

Review of attachment 278196 [details] [review]:

Thanks for the quick update, still needs some more work on the parsing part.

::: gst/pnm/gstpnmdec.c
@@ +108,1 @@
   }

You should remove this function, it isn't needed. Also remove the 'buf' from the instance struct. All of the data storage can be done by the videodecoder baseclass

@@ +197,1 @@
 }

Remove this function, too. You should just use the add_to_frame from the base class.

@@ +232,3 @@
+    gst_buffer_unref (buf);
+    s->buf = obuf;
+      o_rowstride = GST_ROUND_UP_4 (i_rowstride);

After handling a buffer you should reset the s->size to its initial state so you get ready to parse the next image.

@@ +255,3 @@
+  if (size < 8)
+    goto need_more_data;
+  data = gst_adapter_take_buffer (adapter, size);

Instead of using gst_adapter_take_buffer, use gst_adapter_map to avoid removing all data from the adapter. Remember to call gst_adapter_unmap when you are done with the data.

@@ +312,3 @@
+  } else {
+    gst_pnmdec_add_to_frame (s, data);
+  }

For parsing the data you have 3 states:

1) You are starting a new frame. Just return NEED_DATA until you have enough bytes to read the headers (the data type and the width and height)

2) You have enough data to read the headers, read it and store the total size of the input frame. Keep returning NEED_DATA until you have that size in your adapter.

3) You have enough data for a full frame, call _add_to_frame with this amount of data and then call have_frame to signal you have a full frame.


In this patch, you are mixing baseclass handling of data with your own custom one and that is not needed.

Thanks for the reply. 

Need bit of clarification regarding parsing,
PPM if its stored as raw data then its possible to know how many input bytes are to be read (w * h) for gray and (w * h * 3) for RGB, but if its in ascii then input number of bytes is not possible to calculate as data will be stored as number between 0 to 255. 0 takes 1 byte and value above 99 takes 3 bytes.
So to overcome this problem the current parser parses ascii converts data to raw, stores it in buffer. Now from size of that buffer we can know how many pixels are read.
I am not very clear on how to know the number of bytes to read after reading header (Point 2). Is there any way I get a notification saying end of stream (or data) using which I can read till end of data and then call _add_to_frame with this ? Is this a better way than the current approach ?

(In reply to comment #4)
> Thanks for the reply. 
> 
> Need bit of clarification regarding parsing,
> PPM if its stored as raw data then its possible to know how many input bytes
> are to be read (w * h) for gray and (w * h * 3) for RGB, but if its in ascii
> then input number of bytes is not possible to calculate as data will be stored
> as number between 0 to 255. 0 takes 1 byte and value above 99 takes 3 bytes.
> So to overcome this problem the current parser parses ascii converts data to
> raw, stores it in buffer. Now from size of that buffer we can know how many
> pixels are read.
> I am not very clear on how to know the number of bytes to read after reading
> header (Point 2). Is there any way I get a notification saying end of stream
> (or data) using which I can read till end of data and then call _add_to_frame
> with this ? Is this a better way than the current approach ?

Let me re-review the patch on the light of these details.

Review of attachment 278196 [details] [review]:

Did a second review with the ascii scenario in consideration.

::: gst/pnm/gstpnmdec.c
@@ +255,3 @@
+  if (size < 8)
+    goto need_more_data;
+  data = gst_adapter_take_buffer (adapter, size);

The problem with using take_buffer will remove data from the adapter and then later when you call _add_to_frame it won't be adding the same data you inspected. Use the _map to avoid this.

@@ +312,3 @@
+  } else {
+    gst_pnmdec_add_to_frame (s, data);
+  }

The procedure above still holds, even if it is still ascii. When ascii, you can go doing your conversion to the binary format while parsing and storing it to a instance variable, but you should still do the _add_to_frame of the data that you already parsed.

The difference is that when you get your handle_frame called you shouldn't use the input buffer but the buffer that you have already assembled internally (for ascii converted data).

Please add a comment for the different handling of ascii input for future readers.

I am making the changes, will submit the patch with changes tomorrow

Created attachment 278327 [details] [review]
pnmdec: PNMDecoder has to use gstVideoDecoder class

Attached is the updated patch with all review comments. Have tested it with both binary and ascii files. 

Will update documentation once this patch is accepted.

Review of attachment 278327 [details] [review]:

Great, it is working and the patch looks good. Just some minor tweaks/cleanup below and I'll merge it.

Please submit the new patch with the documentation fixes.

::: gst/pnm/gstpnmdec.c
@@ +296,3 @@
+            gst_video_decoder_set_output_state (GST_VIDEO_DECODER (s), format,
+            s->mngr.info.width, s->mngr.info.height, s->input_state);
+        gst_video_decoder_negotiate (GST_VIDEO_DECODER (s));

Remember to check the result of this. If it fails, return NOT_NEGOTIATED for upstream.

@@ +304,3 @@
+        offset = s->mngr.data_offset;
+        data = gst_adapter_take_buffer (adapter, offset);
+        gst_buffer_unref (data);

You can use gst_adapter_flush to remove the first bytes of an adapter.

::: gst/pnm/gstpnmdec.h
@@ +42,3 @@
+  GstVideoDecoder decoder;
+  GstVideoCodecState *input_state;
+  GstVideoCodecState *output_state;

You're not really using input_state or output_state, better to remove those attributes.

Make sure to unref the output_state you get from the set_output_state call.

Created attachment 278385 [details] [review]
Removed unused Variables, Added comments

Thanks for your comments. Did the below changes as you suggested.

Removed unused Variables, did unref for output_state, Added comments.

Joined the two patches into one and pushed, thanks for the updates.

commit 310fe9f78086ded4b68326d46df8eae293736199
Author: Sanjay NM <sanjay.nm@samsung.com>
Date:   Thu Jun 12 16:38:35 2014 +0530

    pnmdec: use GstVideoDecoder Class
    
    https://bugzilla.gnome.org/show_bug.cgi?id=731400