[Bug 731890] New imagesequencesrc element

[GStreamer (bugzilla.gnome.org)]

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

Nicolas Dufresne <nicolas.dufresne> changed:

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

--- Comment #3 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2014-06-24 19:32:17 UTC ---
Review of attachment 279139:
 --> (https://bugzilla.gnome.org/review?bug=731890&attachment=279139)

Partial review, but few comments already. I'll let you address that, and give
it a second pass when we get V2.

::: gst/sequences/Makefile.am
@@ +4,3 @@
+libgstsequences_la_SOURCES = \
+    gstimagesequencesrc.c   \
+    gstimagesequence.c

Please fix the indentation, make can easily get confused when mixing tabs and
spaces. Outside of rules, I recommend using spaces.

::: gst/sequences/gstimagesequence.c
@@ +1,2 @@
+/* GStreamer
+ * Copyright (C) 2014 <cfoch.fabian at gmail.com>

Put your name (or affiliation) instead of your email.

::: gst/sequences/gstimagesequencesrc.c
@@ +1,2 @@
+/* GStreamer
+ * Copyright (C) 2014 <cfoch.fabian at gmail.com>

Same here

@@ +113,3 @@
+do_seek (GstBaseSrc * bsrc, GstSegment * segment)
+{
+  /* FIXME: Improve precision of seeking */

Does this means seeking isn't frame accurate ? why ?

@@ +126,3 @@
+
+  if (reverse) {
+    GST_FIXME_OBJECT (src, "Handle reverse playback");

Why not supporting it ? Isn't it trivial in this case ?

@@ +136,3 @@
+  } else {
+    src->index = 0;
+    GST_WARNING_OBJECT (src, "No FPS set, can not seek");

I'm thinking you are not checking the right thing in is_seekable()

@@ +170,3 @@
+      g_param_spec_boolean ("loop", "Loop",
+          "Whether to repeat from the beginning when all files have been
read.",
+          FALSE, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

Have you tested that seeking work when this is enabled ?

@@ +172,3 @@
+          FALSE, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
+  g_object_class_install_property (gobject_class, PROP_FILENAMES,
+      g_param_spec_pointer ("filenames-list", "Filenames GList",

Pointer is kind of the worst, and is not binding friendly (will leak in
python). You might need to dig around to make sure life-time works, but it's
probably a boxed.

@@ +181,3 @@
+          "index is incremented by one for each buffer read.",
+          0, INT_MAX, DEFAULT_INDEX,
+          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

Is this needed ?

@@ +188,3 @@
+          "is reached, the index will be set to the value start-index.",
+          0, INT_MAX, DEFAULT_START,
+          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

It's not clear what is this used for, looks like copy pasted from multifilesrc,
but documentation does not match.

@@ +193,3 @@
+          "Stop value of index.  The special value -1 means no stop.",
+          -1, INT_MAX, DEFAULT_STOP,
+          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

Same.

@@ +201,3 @@
+
+  gst_element_class_set_static_metadata (gstelement_class,
+      "ImageSequenceSrc plugin", "Src/File", "FIXME Description",

Please fix.

@@ +294,3 @@
+
+  filename = (gchar *) src->iter_list->data;
+  mime = g_content_type_guess (filename, NULL, 9, NULL);

Can you explain why using mime instead of GStreamer typefind ?

@@ +300,3 @@
+
+static gboolean
+gst_imagesequencesrc_set_location (GstImageSequenceSrc * src,

Did go through properly, but you could consider the GRegex, and iterate of
files in base path instead ?

@@ +376,3 @@
+    }
+  } else {
+    src->location = NULL;

Just free and set to NULL instead of this else case.

@@ +390,3 @@
+  /* Calculating duration */
+  src->duration = GST_SECOND * (src->stop_index - src->start_index + 1) *
+      src->fps_d / src->fps_n;

Please, as some care to avoid division by zero. Also, use the
gst_util_uint64_scale utility to avoid overflow.

@@ +436,3 @@
+      /* FIXME: This is hacky */
+      if (!mime)
+        mime = "image/jpeg";

Please fix. This imply that props are set in specific order or something, we
don't want this. Make sure that you compute stuff on state transition instead.

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