[Bug 731890] New imagesequencesrc element

[GStreamer (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

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

--- Comment #22 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 323929:
 --> (https://bugzilla.gnome.org/review?bug=731890&attachment=323929)

::: gst/sequences/gstimagesequence.c
@@ +1,2 @@
+/* GStreamer
+ * Copyright (C) 2014 Cesar Fabian Orccon Chipana

We have 2016 now :)

@@ +32,3 @@
+plugin_init (GstPlugin * plugin)
+{
+  return gst_element_register (plugin, "imagesequencesrc", GST_RANK_NONE,

Why no rank?

@@ +33,3 @@
+{
+  return gst_element_register (plugin, "imagesequencesrc", GST_RANK_NONE,
+      gst_imagesequencesrc_get_type ());

GST_TYPE_IMAGESEQUENCESRC

@@ +40,3 @@
+    imagesequencesrc,
+    "Reads/Writes buffers from a playlist of image file or from a location
(printf) pattern.",
+    plugin_init, VERSION, "LGPL", PACKAGE_NAME, GST_PACKAGE_ORIGIN)

GST_LICENSE

::: gst/sequences/gstimagesequencesrc.c
@@ +31,3 @@
+ * image,location=/path/to/a.png
+ * image,location=/path/to/b.png
+ * image,location=/path/to/c.png

What about using a more "standard" playlist format? m3u or PLS for example? You
could have the framerate as metadata in there, and optionally also allow
durations

@@ +37,3 @@
+ *
+ * |[
+ * gst-launch-1.0 -v imagesequencesrc location="playlist" framerate="24/1" !
decodebin ! videoconvert ! xvimagesink

Why is the framerate in the playlist, or in the URI, or as a property? Too many
ways to do the same :)

@@ +98,3 @@
+  PROP_LOOP,
+  PROP_FILENAMES,
+  PROP_URI

These properties all seem a bit redundant. At least of location and URI, one
should go away

@@ +137,3 @@
+  segment->time = segment->start;
+
+  src->index = DEFAULT_START_INDEX;

Shouldn't the index become whatever index would be at the seek position? Also
here you don't prevent seeks with negative rates, do you actually handle them?

@@ +165,3 @@
+          "Set a list of filenames directly instead of a location pattern."
+          "If you *get* the current list, you will obtain a copy of it.",
+          G_TYPE_PTR_ARRAY, G_PARAM_READWRITE));

A GPtrArray for properties is not such a good idea. This should be a
GValueArray for introspection purposes

@@ +169,3 @@
+      gst_param_spec_fraction ("framerate", "Framerate",
+          "Set the framerate to internal caps.",
+          1, 1, G_MAXINT, 1, -1, -1,

Why negative? Should probably be 0/1 (or 1/G_MAXINT) to G_MAXINT/1 with 1/1
being the default

@@ +183,3 @@
+
+  gst_element_class_set_static_metadata (gstelement_class,
+      "ImageSequenceSrc plugin", "Src/File",

Source/File/Image or something like that. Not Src

@@ +211,3 @@
+        gst_imagesequencesrc_parse_location (src);
+      }
+      ret = GST_ELEMENT_CLASS (parent_class)->change_state (element,

Move the chaining up outside the switch statement

@@ +240,3 @@
+  GstImageSequenceSrc *src = GST_IMAGESEQUENCESRC (bsrc);
+
+  if (src->caps) {

Why do we need this? What if you want to have a mixed playlist with JPG, PNG,
etc? I think it's ok to always return ANY here and just configure the correct
caps on the srcpad for each buffer. Always returning the current caps is
strictly speaking not correct

@@ +269,3 @@
+
+  switch (GST_QUERY_TYPE (query)) {
+    case GST_QUERY_DURATION:

You could also implement the POSITION query in TIME/DEFAULT format. And
DURATION in DEFAULT format.

@@ +305,3 @@
+  gchar *content = NULL, *escaped_content = NULL, **lines = NULL;
+
+  clean_action_str = g_regex_new ("\\\\\n|#.*\n", G_REGEX_CASELESS, 0, NULL);

Cache the regex

@@ +383,3 @@
+    }
+
+    structures = g_list_append (structures, structure);

That's expensive, repeated append to a GList is quadratic. Use a GQueue or even
a GPtrArray

@@ +452,3 @@
+  src->duration =
+      gst_util_uint64_scale (GST_SECOND * src->filenames->len, src->fps_d,
+      src->fps_n);

Maybe guard against fps_n == 0 here?

@@ +471,3 @@
+
+  gst_caps_replace (&src->caps, new_caps);
+  gst_pad_set_caps (GST_BASE_SRC_PAD (src), new_caps);

gst_base_src_set_caps() and also check if the caps actually changed first

@@ +489,3 @@
+    {
+      src->fps_n = gst_value_get_fraction_numerator (value);
+      src->fps_d = gst_value_get_fraction_denominator (value);

This probably should recalculate the duration? Or you disallow changing the
property in >READY

@@ +504,3 @@
+      src->uri = g_strdup (g_value_get_string (value));
+      location = gst_uri_get_location (src->uri);
+      gst_imagesequencesrc_set_location (src, location);

Maybe use GstUri to also parse things like framerates and stuff from the URI?

@@ +575,3 @@
+  if (src->caps)
+    gst_caps_unref (src->caps);
+  g_ptr_array_free (src->filenames, FALSE);

Dispose can be called multiple times, you have to set all the things to NULL

@@ +605,3 @@
+  }
+
+  filename = g_ptr_array_index (src->filenames, src->index);

Maybe also assert that src->index < src->filenames->len

@@ +616,3 @@
+  buf = gst_buffer_new ();
+  gst_buffer_append_memory (buf,
+      gst_memory_new_wrapped (0, data, size, 0, size, data, g_free));

gst_buffer_new_wrapped()

@@ +627,3 @@
+  }
+
+  buffer_duration = gst_util_uint64_scale (GST_SECOND, src->fps_d,
src->fps_n);

This gives accumulating rounding errors. Instead:
  timestamp = index * fps_d / fps_n;
  duration = (index + 1) * fps_d / fps_n - timestamp;
  index++;

or similar

@@ +630,3 @@
+
+  /* Setting timestamp */
+  GST_BUFFER_PTS (buf) = GST_BUFFER_DTS (buf) = src->index * buffer_duration;

Keep DTS at GST_CLOCK_TIME_NONE

@@ +634,3 @@
+  GST_BUFFER_OFFSET (buf) = src->offset;
+  GST_BUFFER_OFFSET_END (buf) = src->offset + size;
+  src->offset += size;

You probably also want to reset this offset for seeking

@@ +689,3 @@
+  GstImageSequenceSrc *src = GST_IMAGESEQUENCESRC (handler);
+
+  g_object_set (src, "uri", uri, NULL);

Probably want to check if it's a valid URI here.

::: tests/check/elements/imagesequence.c
@@ +121,3 @@
+
+  suite_add_tcase (s, tc_chain);
+  tcase_add_test (tc_chain, test_properties);

You probably want at least to check if playing with a printf style URI works,
with a playlist works... and for each also check the timestamps of all the
buffers and check if they actually arrived. Bonus points for seeking tests too
:)

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