[Bug 737339] gifdec: new element

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=737339

Mathieu Duponchelle <mduponchelle1 at gmail.com> changed:

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

--- Comment #14 from Mathieu Duponchelle <mduponchelle1 at gmail.com> ---
Review of attachment 287060:
 --> (https://bugzilla.gnome.org/review?bug=737339&attachment=287060)

::: 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 at samsung.com>

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

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.