New imagesequencesrc element
Submitted by César Fabián Orccón Chipana
Assigned to Tim Müller @tpm
Link to original bug (#731890)
Description
Created attachment 278728
It is an example of how to use GstImageSequenceSrc
Hello guys. I've written an element in gst-plugins-bad which I called as GstImageSequenceSrc. It works very similar to GstMultiFileSrc, but there are some differences.
Differences with GstMultiFileSrc.
-
Handles a list of filenames instead of a printf pattern as GstMultiFileSrc does.
- Having a list of filenames is useful because you can set the filenames you want,
in the order you want. For example, you can add more filenames or sort the list. - There are two ways to create this list:
a) Setting a location propertie. This value could look like:
1) "/some/folder/with/images/ or ."
2) "/a/path/img.jpg,/other/path/img2.jpg" or "img1.png,img2.png"
3) "/a/path/.JPEG" or ".png"
b) Setting the filename-list (GList) directly.
- Having a list of filenames is useful because you can set the filenames you want,
-
Creates an "imagesequence://" protocol which allows the playbin to play this element. It handles a 'fake' uri but it is useful finally.
gst-launch-1.0 playbin uri="imagesequencesrc:///some/path/*.jpg?framerate=20/1&loop=1" -
It "detects" the mimetype of the images. You only have to worry about the framerate.
-
It calculates the duration.
Things to improve:
- Seeking: it seeks to the wrong image sometimes (actually, after many seeks).
- The way duration is calculated.
PD: stormer was telling me to support png and jpeg files (both at the same time) but I don't see a usecase.
You can see the most-stable version of this element in
https://github.com/cfoch/gst-plugins-bad/tree/sequences/gst/sequences
Develop:
https://github.com/cfoch/gst-plugins-bad/tree/sequences-develop/gst/sequences
Attachment 278728, "It is an example of how to use GstImageSequenceSrc":
test.c
Activity
Hello. Please I would like you to review my patch and telling me your opinions. Thanks.
Patch 279139, "ImageSequenceSrc":
imagesequencev1.patch
@ndufresnereviewed patch 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.cPlease 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@gmail.com>Put your name (or affiliation) instead of your email.
::: gst/sequences/gstimagesequencesrc.c @@ +1,2 @@ +/* GStreamer + * Copyright (C) 2014 <cfoch.fabian@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.
I've fixed according your review, except:
reverse: I've implement it but I am not sure how to test it (it is not in the patch)
makefile.am: I copy/pasted from another makefile.am (they mix tabs with spaces)
regex: can't we rely the user of the plugin will set a valid "location"?
Please review it. There are many changes and I am not sure if it the way I used the typefind is the best.
Thanks in advance.
Patch 279434, "Fixing according Nicolas' review":
imagesequencev2.patch
@ndufresnesaid:(In reply to comment 4)
reverse: I've implement it but I am not sure how to test it (it is not in the
patch)For testing reverse playback (and seeking in reverse), you can use the playback test in gst-plugins-base (gst-plugins-base/tests/examples/playback/playback-test). To run a custom pipeline:
./playback-test 1 "imagesequencesrc ... ! videoconvert ! autovideosink"
I should be able to give this a second pass soon, unless someone is faster ;-P. A small note, when you update a patch, please mark the old one as obsolete, helps keeping the attachment clean.
Ok guys... I am keep this element simple and ready to merge. This element is using a location pattern as multifilesrc does and a location which is a playlist file.
The playlist file looks like:
{
metadata,framerate=(fraction)1/1
image,location=/home/cfoch/pitivi-git/pitivi/tests/samples/flat_colour1_640x480.png
image,location=/home/cfoch/pitivi-git/pitivi/tests/samples/flat_colour2_640x480.png
#image,location=/home/cfoch/pitivi-git/pitivi/tests/samples/flat_colour3_320x180.png
}It would be nice if someone reviews it and tell me if it is possible to merge or if it is needed to change something.
This patch (I think) it is ready to merge. Would be nice some review and let me know if you will merge it.
Patch 282033, "Ready to merge":
imagesequence.patch@ndufresnereviewed patch 282033:Please use git format-patch. I'll stop there, maybe I'm confused. So what do you want to propose, a playlist base image sequence or a mix of playlist/pattern element ?
::: gst/sequences/gstimagesequence.c @@ +39,3 @@ + GST_VERSION_MINOR, + imagesequencesrc, + "Reads/Writes buffers from sequentially named image files",From playlist ?
::: gst/sequences/gstimagesequencesrc.c @@ +20,3 @@ + * SECTION:element-gstimagesequencesrc + * + * Reads buffers from sequentially named files.Playlist ?
@@ +25,3 @@ + * <title>Example launch line</title> + * |[ + * gst-launch-1.0 -v imagesequencesrc location="0d%.png" framerate="24/1" ! decodebin ! videoconvert ! xvimagesinkDidn't you say it's a playlist ?
@@ +37,3 @@ + * image,location=/path/to/c.png + * ]| + * </refsect2>Please update the doc, it's confused. This could have been implemented with GKeyFile API, or standard m3u rather then your own format/parser no ?
@@ +88,3 @@ + PROP_START_INDEX, + PROP_STOP_INDEX, + PROP_FRAMERATE,Do you still need this one as you seem to say you set the framerate in the playlist ?
@@ +90,3 @@ + PROP_FRAMERATE, + PROP_LOOP, + PROP_FILENAMES,Playlist ?
There was a problem with seeking that happened with non-PNG format. I've fixed it and I've tested it with some seeking scenarios in gst-validate and it works correctly and for different formats (PNG/JPEG/TIFF, I didn't tested more). Please review it. Now I think this is really ready to merge.
Patch 283434, "READY TO MERGE MERGE ":
0001-imagesequencesrc_2.patch@nekohayosaid:Hi Fabian,
In which branch of yours on github can I find your correct/official/final work? I'm confused because I can't find this particular patch in any of them. There's a bunch of branches and I don't know which one is the correct one, please delete the irrelevant ones so I have one branch in pitivi and one branch in gst to test.Done :)
It is sequences and sequences-develop.
You can see the most-stable version of this element in
https://github.com/cfoch/gst-plugins-bad/tree/sequences/gst/sequencesDevelop:
https://github.com/cfoch/gst-plugins-bad/tree/sequences-develop/gst/sequences@nekohayosaid:Thank you Fabian,
I would still recommend deleting the other branches from the github repo if you're not using them anymore, for clarity... anyway, I found two bugs (I'm guessing those are issues in your Pitivi branch rather than the gst plugins bad code):-
Does not support playlist files containing spaces or special characters
https://github.com/cfoch/pitivi-cfoch/issues/1 -
Timeline duration is not updated when removing an image sequence clip
https://github.com/cfoch/pitivi-cfoch/issues/2
It should be noted for whoever is reading this that I did not review the code at all, just checking if the thing actually works as advertised. I'm happy to report that besides the little bugs above, it does work. Congratulations! :)
-
@nekohayosaid:Hmm, I tested the "sequences" branch for both your gst plugins bad repo and your pitivi repo (I supposed I needed to be using the same branch names in both), instead of "imagesequences" or whatever the proper branch is supposed to be for your pitivi repo. This is exactly why I ask to remove obsolete branches.
You still have 7 branches sitting around in https://github.com/cfoch/pitivi-cfoch/branches, 4 of which are related to image sequences — I want only one sequences branch to test to avoid confusion, unless you have really clear names and good reasons to have more than one.
@nekohayosaid:Yeah, marking older patch versions as "obsolete" in bugzilla is important.
For example, it wasn't entirely clear to me just now if attachment#282033is obsoleted by attachment#283434, and if attachment#283434is obsoleted by the current state of your git branch. Keep that crystal-clear at all times please.@ndufresnereviewed patch 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.cCleanup 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.
- Plays a sequence of all images which matches the given printf location
-
- GstImageSequenceSrc has internally an index value which goes form
src->start_index to src->stop_index.
- GstImageSequenceSrc has internally an index value which goes form
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.
- Plays a sequence of all images which matches the given printf location
-
- GstImageSequenceSrc has internally an index value which goes form
src->start_index to src->stop_index.
- GstImageSequenceSrc has internally an index value which goes form
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.
- 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.Hello. I am removing the printf location and, as consecuence, removing the start/stop index, because it doesn't make sense to have them if we don't handle the location pattern. I am "Fixing" some details about documentation. So... finally the element handles image sequences (same mimetype format) using a playlist only.
Patch 286661, "Imagesequencesrc: removed printf location pattern. removed stop/start index. it handles the playlist only.":
gstimagesequencesrc_simple.patch@ndufresnereviewed patch 286661:Here's few more comments. The most important part, move blocking IOs on the playlist to the streaming thread, make sure it can be canelled when doing to READY state (just in case something wrong happens). ->create() is run from the streaming thread, negotiation() also.
::: gst/sequences/gstimagesequencesrc.c @@ +38,3 @@ + * |[ + * gst-launch-1.0 -v imagesequencesrc location="playlist" framerate="24/1" ! decodebin ! videoconvert ! xvimagesink + * ]|Could you add examples for using filename-list and the uri ?
@@ +200,3 @@ + case GST_STATE_CHANGE_READY_TO_PAUSED: + if (src->location) { + gst_imagesequencesrc_parse_location (src);Please move this in the streaming thread. This function do blocking IOs, from the main thread which is first cause of stutter in UIs.
@@ +298,3 @@ + gchar *content = NULL, *escaped_content = NULL, **lines = NULL; + + clean_action_str = g_regex_new ("\\\\\n|#.*\n", G_REGEX_CASELESS, 0, NULL);My personal taste would be to use GDataInputStream and the g_data_input_stream_read_line_utf8() method. What do you think of that ?
@@ +415,3 @@ + filename = gst_structure_get_string (l->data, "location"); + if (filename) { + filenames = realloc (filenames, (i + 2) * sizeof (gchar *));Use GPtrArray here instead. Also, don't call OS method directly, use glib.
ndufrense/stormer, please review my patch.
Patch 323929, "gstimagesequencesrc: GPtrArray instead of GList; test added":
0001-imagesequencesrc-plugin-to-stream-sequences-of-image.patch@tpmsaid:Moving to -good, I think this should just be added to the multifile plugin.
I think I started a branch for integration a while back as well (but that shouldn't stop anyone from merging it or working on it), but I'd like to have a final look at the API (properties) before it gets merged.
@slomoreviewed patch 323929:::: gst/sequences/gstimagesequence.c @@ +1,2 @@ +/* GStreamer + * Copyright (C) 2014 Cesar Fabian Orccon ChipanaWe 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.pngWhat 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 ! xvimagesinkWhy 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_URIThese 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 :)
@tpmsaid:There's G_TYPE_STRV for string array properties for what it's worth.
I would prefer to see support for any kind of playlist file removed from this plugin, especially anything containing caps. If we really must support reading filenames from a file, then it should just be filenames newline-separated IMHO, and no further structure than that. We can typefind files to figure out the caps (if needed). If they can't be figured out from the header, this element is not for such files ;)
gstimagesequencesrc
The purpose of GstImageSequenceSrc is to give the developer an easier way to handle sequences of images. Currently Gstreamer have an element called multifilesrc which is useful to stream image of sequences, but since it is more generic. GstImageSequenceSrc pretends to focus on images. Problems ~~~~~~~~ * Gstreamer's multifilesrc can stream image of sequences, but it is limited to file names following the printf format: "%04d". * It is not possible to tell Gstreamer´s multifilesrc, the duration per frame. * GES's multifilesource repeats the same problems mentioned above. Also, this source implement a "hacky" uri to be used as a GESUriClip. Propositions to solve those problems- Add a property using an array of image file names.
- Add a property using an array of duration.
- In case no array of duration is given,
- Set a framerate through out a property.
- In Gstreamer Editing Services, create:
- GESImageSequenceClip
- GESImageSequenceSource
Use cases / Possible users
* User who wants to create a script or application to render a video from a sequences of image files. * Handle image sequences as a *clip* in Gstreamer Editing Services, for editing purposes. Advantages ~~~~~~~~~~~ * Control over the filenames * Control over the duration of the frame * in Gstreamer editing services, treat imagesequencesrc as a GESClip, and having access to direct elements. Disadvantages ~~~~~~~~~~~~~ * Since gst-launch can´t parse an array of strings, it will not be possible to call imagesequencesrc throughout the pipeline using gst-launch. API Draft ~~~~~~~~~~ A. GstImageSequenceSrc Properties ---------- - filenames: G_TYPE_STRV) * Set an array of file names. * If this value has been already set, it will not be possible to set a new value. (new version maybe?) - durations: G_TYPE_INT * Set an array of duration per file name. * If this value or "framerate" value has been already set, it will not be possible to set a new value for whichever of both. - framerate: G_TYPE_FRACTION * Set the framerate for constant speed. * If this value or "durations" has been already set, it will not be possible to set a new value for whichever of both. B. GESImageSequenceSrc Properties ---------- Properties will be set throughout gst_track_element_set_child_property ... More Ideas (but need more development) ~~~~~~~~~~~ In order to play the sequence from gst-launch, there may be a playlist file. The playlist file can be a standard format such as m3u or pls. - The easiest way would be to add a property "playlist". It means that the element should typefind the playlist file and parsing it depending the format it is. - We could also think on a demuxer. >>>@slomosaid:(In reply to Nicolas Dufresne (stormer) from comment 27)
I'm not fan of forcing the list of filename to be fully in RAM. Would it be
acceptable to use a callback for the element to ask the application for the
next filename ?I also prefer that, I think I also proposed that on IRC at some point. A callback (returning a GstStructure with URI and duration?) plus for simplicity a printf-style URI like in multifilesink plus a framerate property. If no duration is known for a URI it would be based on the framerate, and the framerate is also what goes into the caps.
By this do you mean to emit a signal in gst_push_src_create function? If so, it means we are going to set the duration (and filename), every time we want to create a buffer. GES needs to know the duration of the source. How can we know the duration of source in this case?
Please, review it. According Tim, I have moved it to -good.
Patch 325756, "GstImageSequenceSrc element":
0001-multifile-add-imagesequencesrc-element-to-stream-ima.patchThis is the test. Review it, too, please. It's the first time I use a GstHarness.
Patch 325757, "Test imagesequencesrc":
0002-test-multifile-imagesequencesrc.patchBTW, I copied the code here:
https://github.com/GrokImageCompression/gst-plugins-bad/tree/731890-imagesequencesrc
hmmmmmmm, so this pipeline (using the "good" plugin multifilesrc) seems to do everything that imagesequencesrc was supposed to do:
gst-launch-1.0 multifilesrc location="img.%04d.png" index=0 caps="image/png,framerate=(fraction)12/1" ! \
pngdec ! videoconvert ! videorate ! theoraenc ! oggmux ! \
filesink location="images.ogg"@ndufresnesaid:(In reply to Aaron Boxer from comment 39)
Thanks. Can we not then add seek to multifilesrc ? Rather than making a
whole new plugin ?Only if we add video specific code in multifilesrc, and also make the stream format configurable between BYTE and TIME. That is unlikely to happen.
moved all changes into single patch, for now
Patch 355333, "gstimagesequence: single patch":
0001-imagesequencesrc-plugin-to-stream-sequences-of-image.patchI have added this patch to my gst-plugins-bad repo.
But, when I launch this pipeline
gst-launch-1.0 -v imagesequencesrc location="playlist" framerate="24/1" ! decodebin ! videoconvert ! xvimagesink
I get the error
"no element imagesequencesrc"
Can anyone see what the problem is here? Must be something simple missing.
Thanks!
This patch is now working with the following:
playlist file:
metadata,framerate=(fraction)3/1
image,location=/path/to/a.png
image,location=/path/to/b.png
image,location=/path/to/c.pngand pipeline
gst-launch-1.0 -v imagesequencesrc location="/path/to/playlist" framerate="24/1" ! decodebin ! videoconvert ! ximagesink
or
gst-launch-1.0 -v playbin uri="imagesequence:///path/to/playlist"
Code review please, anyone ?
Patch 355343, "working imagesequencesrc plugin":
0001-imagesequencesrc-plugin-to-stream-sequences-of-image.patchIs there consensus on how this plugin should work?
- Format of playlist file ?
- How to set duration of clip ?
- format of URI ?
- How to set frame rate ?
- Callback to provide the next file ? How would the callback work for a command-line pipeline ?
As slomo suggested above, we can leave the callback for now and just have printf URI plus framerate ?
