[Bug 731890] [Pitivi] New imagesequencesrc element

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #17 from César Fabián Orccón Chipana <cfoch at live.com> 2014-09-11 23:56:49 UTC ---
(In reply to comment #16)
> Review of attachment 283434 [details]:
> 
> 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.
(In reply to comment #16)
> Review of attachment 283434 [details]:
> 
> 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.

Thanks, stormer.
1. I am going to remove the printf pattern location. By removing it, it means
forget about stop-index and start-index: it does not make sense to have a
start-index and a stop-index property for a list of files (the start is 0 and
the stop is the len - 1, actually I've realized stop is not necessary).

"""
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 ?
"""
2. Yes! I want to do that. But after the patch with the changes described in 1.

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