Created attachment 287052 [details]
Adding sample files tested

Will attach the patches in sometime.

You forgot to attach the patch. Why do you have your own LZW implementation though, instead of just using giflib?

Is there no implementation in libav?

I once had a patch for gdkpixbufdec that did this, problem was that you could easily make it go OOM because it would decode all the frames in memory first.

Created attachment 287060 [details] [review]
Adding a new gifdec element

Hi Sebastian,

I am attaching the patch now :)
The same can be tested with the following launch command
gst-launch-1.0 -v filesrc location=../1.gif ! image/gif,framerate=5/1 ! gifdec ! videoconvert ! ximagesink

i was referring to libav to check for gif decoders and there this implementation of lzw was being used. Hence i followed the same.
Probably the next step would be to use giflib instead of our own lzw implementation. Since i had to start somewhere, i did the same by using already available code.


Hi Tim,

There is a gif decoder in libav. But it is disabled.
Even when i enable it, it just shows the first image similar to gdkpixbufdec

I had read comments in forums about you already trying with gdkpixbufdec,
hence i implemented the same by decoding one frame at a time and displaying the same before decoding the next frame.
I parse the gif file, until the end of one image is found, and then post finish frame, which displays the frame. and then continue parsing until end of next image is found.

It did seem to work OK. Please review the changes.

Created attachment 287061 [details] [review]
to get output adapter

This patch is needed for gif decoder element to get the output adapter from the video decoder element.
Please review.

Hi Sebastian/Tim,

Can you please review if this patch is OK? 
For the first version, i thought it will concern mostly about the logic where individual frames are parsed and displayed, and for the next version i will start using giflib.

Or should i start using giflib now itself?

I have been analyzing the possibility of using giflib for this element.
These are my observations.

My implementation is such that, i parse the data, and check if the frame end is reached and display the frame, and continue parsing the rest of the data

The image data(data subblock) in GIF is arranged such that, "The first byte in the sub-block tells you how many bytes of actual data follow. This can be a value from 0 (00) it 255 (FF). After you've read those bytes, the next byte you read will tell you now many more bytes of data follow that one. You continue to read until you reach a sub-block that says that zero bytes follow."

So with my implementation,
i read the first byte of subblock and skip those many bytes using gst_byte_reader_skip, until i reach zero bytes.

In case of gstlib, there is no skip function.
we have read each and every byte using 
DGifGetCode and DGifGetCodeNext functions, to find out if the end of the frame is reached.

In case of reading the complete file and displaying the image, giflib works well.
But since it leads to OOM case for large files, i guess that cannot be used for this implementation.

Please advice on the same

That sounds like a valid reason not to use giflib then :) Is there no better gif library out there? What are web browsers using?

not exactly sure about other libraries.. Probably someone with prior experience could comment on the same :)

Webkit and Blink use Skia to decode GIFs.

https://code.google.com/p/skia/

can someone review this and provide comments :)

Created attachment 296169 [details] [review]
to get output adapter

Rebasing videodecoder code.

Please verify with sample files attached with the below pipeline

gst-launch-1.0 -v filesrc location=../1.gif ! image/gif, framerate=5/1, width=240, height=320 ! gifdec ! videoconvert ! ximagesink

*** Bug 761523 has been marked as a duplicate of this bug. ***

Review of attachment 287060 [details] [review]:

::: gst/gif/gstgif.c
@@ +22,3 @@
+
+#include "gstgifdec.h"
+#include <gst/gst.h>

It's already included in gstgifdec.h

@@ +23,3 @@
+#include "gstgifdec.h"
+#include <gst/gst.h>
+#include <string.h>

Why ?

::: gst/gif/gstgifdec.c
@@ +97,3 @@
+gst_gifdec_flush (GstGifdec * s)
+{
+  s->finaldata = 0;

Please use NULL for this

@@ +141,3 @@
+    reader = &s->reader;
+
+  gst_byte_reader_get_int16_le (reader, &left);

These byte reader methods have return values, I think it's pretty important that you take them into account :)

@@ +256,3 @@
+  s->bits_per_pixel = (v & 0x07) + 1;
+
+  gst_byte_reader_get_int8 (reader, &temp);     /* ignored */

What exactly is ignored ?

::: gst/gif/gstgifdec.h
@@ +38,3 @@
+{
+  GstVideoDecoder decoder;
+  guint16 screen_width;

Why not use a GstVideoInfo to hold some of these fields ?

@@ +55,3 @@
+  gboolean frameread;
+  gboolean parse;
+  gint trace_bytes_left;

Looking at the code, it isn't obvious to me what this is used for, can you either comment this field or provide a more explicit name ?

@@ +57,3 @@
+  gint trace_bytes_left;
+  GstVideoCodecState *input_state;
+  const guint8 *data;

These data fields are a bit surprising, why not pass these as function parameters instead of holding them as state ?

::: gst/gif/gstgiflzw.c
@@ +1,2 @@
+/* LZW decoder
+ * Copyright (C) 2014 Vineeth T M <vineeth.tm@samsung.com>

What in this file do you actually hold a copyright on ? What was the copyright for that LZW GIF decoder you mention ?

Review of attachment 296169 [details] [review]:

::: gst-libs/gst/video/gstvideodecoder.c
@@ +4013,3 @@
+ *
+ * Returns: the instance of the output #GstAdapter used
+ * by the decoder.

You're missing a Since annotation here

::: gst-libs/gst/video/gstvideodecoder.h
@@ -411,2 +411,3 @@
                                                   GstCaps         * filter);
 
+GstAdapter *     gst_video_decoder_get_output_adapter (GstVideoDecoder * decoder);

I have no opinion on the validity of adding this new API, but you'll want to add it to the gtk-doc SECTIONS file

Created attachment 350879 [details] [review]
gifdec: add meson.build

I added a meson build definition to be able to test the plugin in my environment, but it turns out that the proposed plugin is not going to be very helpful to me as is, here are a few comments regarding the actual behaviour:

* The example pipeline doesn't work for me, as width and height also need to be specified in the input caps.

* Is there no way to determine width, height and framerate from the file headers ? (I'm not familiar with that format)

* There should be a typefind function in order to make the plugin usable with uridecodebin / playbin

* The plugin should implement seeking, and ideally segment seeks in order to be able to loop the gif.

* With my test file, (I'll attach it), there are black frames at the beginning of the file

Created attachment 350883 [details]
test file I used, shows black frames at the beginning

Play it back with:

gst-launch-1.0 -v filesrc location=/home/meh/devel/collabora/screenpeaks/test_files/animated.gif ! image/gif, framerate=15/1, width=498, height=255 ! gifdec ! videoconvert ! xvimagesink

I ended up looking at ffmpeg's implementation (http://ffmpeg.org/doxygen/trunk/gifdec_8c-source.html), and the answer to some of my review comments is that the proposed patches are an adapted copy paste of this implementation.

Doing that without retaining the copyrights or even mentioning the original source isn't good practice.

As such, I'm not sure we want to merge any of these patches, as ffmpeg is already wrapped by gst-libav.

Setting to NEEDINFO, to let Sebastian or Tim decide

Closing this, as in any case we don't want to copy paste code from ffmpeg. The way forward would be to enable the gif decoders / demuxers in gst-libav, I may or may not work on that shortly :)

Review of attachment 287060 [details] [review]:

Copy pasted from ffmpeg.

Review of attachment 296169 [details] [review]:

If that's ever needed for another case, or if you think this would make a valid addition nevertheless, please feel free to submit this as another bug.

Review of attachment 350879 [details] [review]:

useless now.