GNOME Bugzilla – Bug 731890
New imagesequencesrc element
Last modified: 2018-11-03 14:53:15 UTC
Created attachment 278728 [details] 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. * 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
Can you attach you patch on the bug, so we can make comment ?
Created attachment 279139 [details] [review] ImageSequenceSrc Hello. Please I would like you to review my patch and telling me your opinions. Thanks.
Review of attachment 279139 [details] [review]: 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@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.
Created attachment 279434 [details] [review] Fixing according Nicolas' review 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.
(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.
Created attachment 282033 [details] [review] Ready to merge This patch (I think) it is ready to merge. Would be nice some review and let me know if you will merge it.
Review of attachment 282033 [details] [review]: 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 ! xvimagesink Didn'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 ?
Created attachment 283434 [details] [review] READY TO MERGE *MERGE* 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.
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/sequences Develop: https://github.com/cfoch/gst-plugins-bad/tree/sequences-develop/gst/sequences
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! :)
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.
Comment on attachment 282033 [details] [review] Ready to merge Please mask older version as obsolete. Also, reply to reviews stating if you are fixing it somehow, or explaing why it's write. Just dropping a new patch does not help the reviewer.
Yeah, marking older patch versions as "obsolete" in bugzilla is important. For example, it wasn't entirely clear to me just now if attachment #282033 [details] is obsoleted by attachment #283434 [details], and if attachment #283434 [details] is obsoleted by the current state of your git branch. Keep that crystal-clear at all times please.
Review of attachment 283434 [details] [review]: 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] [review]: > > 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] [review]: > > 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.
Created attachment 286661 [details] [review] Imagesequencesrc: removed printf location pattern. removed stop/start index. it handles the playlist only. 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.
Review of attachment 286661 [details] [review]: 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.
Created attachment 323929 [details] [review] gstimagesequencesrc: GPtrArray instead of GList; test added ndufrense/stormer, please review my patch.
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.
Review of attachment 323929 [details] [review]: ::: 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 :)
I agree about using a m3u or PLS playlist file. Which one should I choose? Should I parse it ¨manually¨ or taking advantage of some library?
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.
If you agree with this proposal, I will modify GstImageSequenceSrc to work that way. Please, review it, and if there is something you think should be changed, tell me. If everything were okay for you, I will start to code.
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 ?
(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?
You could add a property for the duration too :) What about this for starters: make a minimal element that only takes a printf-style URI and a framerate property? And then we can add all the other things on top of that and discuss them separately.
Created attachment 325756 [details] [review] GstImageSequenceSrc element Please, review it. According Tim, I have moved it to -good.
Created attachment 325757 [details] [review] Test imagesequencesrc This is the test. Review it, too, please. It's the first time I use a GstHarness.
+1 for this feature. I can do some testing on the patch, take a look at the code.
Here is a pipeline that almost works: gst-launch-1.0 -v multifilesrc location="/home/aaron/play/test1.jpg" ! decodebin ! videoconvert ! imagefreeze but there is an error: "streaming stopped, reason not-linked"
BTW, 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"
Correction: this pipeline was what I meant to paste: gst-launch-1.0 multifilesrc location="/home/aaron/play/test.%04d.jpg" index=0 caps="image/jpeg,framerate=\(fraction\)12/1" ! decodebin ! videoconvert ! ximagesink
For forward playback ok, but for seeking it's not complete.
Thanks. Can we not then add seek to multifilesrc ? Rather than making a whole new plugin ?
(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.
Thanks, Nicolas.
Created attachment 355333 [details] [review] gstimagesequence: single patch moved all changes into single patch, for now
I 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!
autoreconf ?
Thanks. I actually did a full autogen.sh, and I see that the "sequences" plugin will be built, but I still get the same error.
Blacklisted ? (gst-inspect-1.0 -b) Plugin name might miss-match it's file?
Bingo!!
Created attachment 355343 [details] [review] working imagesequencesrc plugin 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.png and 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 ?
Actually, I will work through slomo's review above from last year.
Is there consensus on how this plugin should work? 1) Format of playlist file ? 2) How to set duration of clip ? 3) format of URI ? 4) How to set frame rate ? 5) 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 ?
César, are you still interested in working on your plugin ?
(In reply to Aaron Boxer from comment #51) > César, are you still interested in working on your plugin ? Yes, but I think I will not have too much time do it at least for the next (almost) two months because of GSoC. Aaron, but if you have interest go ahead.
Thanks. I don't have time either :) but I will try to move things forward.
What's the minimum that we need to have it merged?(In reply to Aaron Boxer from comment #50) > Is there consensus on how this plugin should work? > > 1) Format of playlist file ? > 2) How to set duration of clip ? > 3) format of URI ? > 4) How to set frame rate ? > 5) 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 ? +1 for printf URI plus framerate - do we need anything else for the initial mergeable commit? Would it make sense to remove for example the playlist support, so we don't have to think about the playlist details? It should be relatively easy to add it later.
(In reply to Alex Băluț from comment #54) > What's the minimum that we need to have it merged?(In reply to Aaron Boxer > from comment #50) > > Is there consensus on how this plugin should work? > > > > 1) Format of playlist file ? > > 2) How to set duration of clip ? > > 3) format of URI ? > > 4) How to set frame rate ? > > 5) 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 ? > > +1 for printf URI plus framerate - do we need anything else for the initial > mergeable commit? > > Would it make sense to remove for example the playlist support, so we don't > have to think about the playlist details? It should be relatively easy to > add it later. Yes, could do. https://github.com/GrokImageCompression/gst-plugins-bad/tree/731890-imagesequencesrc has the latest version of the patch, by the way.
Anybody interested to remove the "playlist" functionality from the patch, to make it slimmer and easier to review/merge? We can easily add it later if there is interest.
A did a quick review from *my* last patch and I removed the playlist feature. I was talking with Alex about the future of this patch. I was supposed to review it because it wasn't clear to me what Aaron Boxer did, since IIRC he squashed his changes with my commit. But unfortunately I didn't expect I would be too busy and I don't have time until December to do something. I hope someone who is interested put its hands on it. It has been some time ago since the last patch I proposed. Sorry, but know I am just quickly scrolling up and I don't see there were reviews for my last patch. I also had the feeling that there wasn't an agreement when I was working on this. And I would like to know from a GStreamer mantainer again what is the basic functionality. I wouldn't like the next person (or maybe I if nobody claims this bug until December) to work twice on it.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/121.