[Bug 731890] [Pitivi] New imagesequencesrc element

[GStreamer (bugzilla.gnome.org)]

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

Nicolas Dufresne (stormer) <nicolas.dufresne> changed:

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

--- Comment #16 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> 2014-09-11 19:41:14 UTC ---
Review of attachment 283434:
 --> (https://bugzilla.gnome.org/review?bug=731890&attachment=283434)

Please address this first row of comment. In general I think this element is
trying to do too much. I would suggest to pick one useful way, e.g. playlist,
and stick with that. Drop stuff (which seems broken anyway) like
start-index/stop-index, seeking can handle that anyway.

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

Cleanup indentation here, and extra space before \

@@ +24,3 @@
+               -ldl \
+     -:PASSTHROUGH LOCAL_ARM_MODE:=arm \
+               LOCAL_MODULE_PATH:='$$(TARGET_OUT)/lib/gstreamer-0.10' \

What the ? 0.10. Is the needed at all ?

::: gst/sequences/gstimagesequencesrc.c
@@ +21,3 @@
+ *
+ * Reads buffers from a location. The location is or a printf pattern
+ * or a playlist of files. Check below to see examples.

You state 3 ways of using the element, but only document the playlist.

@@ +27,3 @@
+
+ * Plays a sequence of all images which matches the given printf location
pattern at 24 fps.
+ * GstImageSequenceSrc has internally an index value which goes form
src->start_index to src->stop_index.

This drop-in information lacks context. In what is it related to the following
example ?

@@ +39,3 @@
+ * image,location=/path/to/b.png
+ * image,location=/path/to/c.png
+ * ]|

Would be nice to elaborate more on the playlist format. Can we add mixed type,
can metadata be added multiple time ? Can we mix image type, e.g. png/jpeg ?

@@ +52,3 @@
+#include "gstimagesequencesrc.h"
+#include <gst/base/gsttypefindhelper.h>
+#include <glib/gstdio.h>

Please split local and system header

@@ +152,3 @@
+      g_param_spec_string ("location", "File Location",
+          "Pattern to create file names of input files. File names are created
"
+          "by calling sprintf() with the pattern and the current index.",

There is no mention that this location can be a playlist.

@@ +212,3 @@
+  g_assert (src->stop_index >= 0 && max_stop >= 0);
+  g_assert (src->stop_index >= src->index && src->stop_index <= max_stop);
+  GST_DEBUG_OBJECT (src, "Set (stop-index) property to (%d)",
src->stop_index);

Did you forget to set the stop index ?

@@ +243,3 @@
+        gst_imagesequencesrc_parse_location (src);
+        gst_imagesequencesrc_set_start_index (src);
+        gst_imagesequencesrc_set_stop_index (src);

So going to pause will likely reset what have been set through properties. I
can't find code that would handle that.

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