GNOME Bugzilla – Bug 699077
videorate: add"rate" property to modify clip speed
Last modified: 2017-01-24 01:07:15 UTC
Created attachment 242681 [details] [review] Patch proposal for enhancement PiTiVi video editor needs the possibility to change the speed of a clip over time (for example for slow motion). A good solution for this is to add a speed-rate property in the videorate element. Here attached is a patch proposing a solution. It can be tested with : "gst-launch-1.0 -v videotestsrc pattern=ball ! videorate speed-rate=1.0 ! ximagesink" , and varying speed-rate values. Minimum value is 0.0, which outputs a fixed frame. P.S. : This is my all-time first bug filing and patch attaching, I hope I don't miss something big.
Review of attachment 242681 [details] [review]: Nice start, here's my thought: - Would be nice to have the same property name as with pitch element - When changing speed of a stream this way, you also need to change the position and the duration in the stream (I think pitch element does that properly).
Also, this parameter should be controllable, so we can play with it during playback, this will messup the duration, but it is expected to be wrapped in an other element that will fix it up.
Are you talking about the element in soundtouch (bad) plugin ? Are you suggesting to rename it "rate" ? I talked about it with PiTiVi devs and it could be confusing with framerate... There's also an audiorate element, should it be changed along videorate ? I'll start woringk on the change of position and duration, and the controlling stuff.
rate sounds correct, that's how it's also called in the GstSegment. But I agree that it can be confusing with the fps, needs to be carefully documented.
Also the interface should probably be better modelled like the scaletempo plugin than pitch
(In reply to comment #5) > Also the interface should probably be better modelled like the scaletempo > plugin than pitch Correct me if I'm wrong, but scaletempo element will input a certain segment rate, and ouput a different rate. I think this is quite complex to use if you want to do a smooth transition from 1 rate to another. While the pitch element simply consume upstream faster or slower depending a configured rate.
Yes, that's a slightly different mode of operation. I guess both elements should also get a property (or properties) to select that, and to select the mode of operation. Good point!
By the way : "A value of 0.0 for the rate is not allowed and should be accomplished instead by PAUSING the pipeline." should it be the same for videorate ? (http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gstreamer-GstEvent.html#gst-event-new-seek) Should it be the same for videorate->rate ?
(In reply to comment #8) > Should it be the same for videorate->rate ? From our discussion on #pitivi channel, in this case a rate zero will keep emitting the same frame at configured framerate. It's different from not doing anything (like in pause) and might be useful to implement a frame freeze.
Created attachment 248849 [details] [review] New version of gstvideorate
Created attachment 248850 [details] [review] Test for rate property. May need review
I just added two attachments. The first one improves gstvideorate. Second one is a new test, to check that various rates are possible. I've also just made a little change in the test (thx Mathieu_Du) : variables diff_ts and prev_ts are gint64, not GstClockTime. Also (still in the test) I'm looking at changing prev_ts that is a static variable into something better (thinking of user_data arg).
Review of attachment 248849 [details] [review]: ::: gst/videorate/gstvideorate.c @@ +744,3 @@ + if (GST_CLOCK_TIME_IS_VALID (segment.stop)) + segment.stop = (gint64) (segment.stop / videorate->rate); + segment.time = (gint64) (segment.time / videorate->rate); I believe you also want to scale segment.applied_rate here @@ +917,3 @@ + break; + } + dst_value = (gint64) (videorate->next_ts / videorate->rate); This might give weird values if the rate is changed during playback. Changing the rate should probably keep the current timestamp somewhere else and reset next_ts to 0. And here then use the timestamp offset + next_ts/rate
Review of attachment 248850 [details] [review]: Please add some checks to see if the output buffer timestamps and durations are correctly scaled too, and also do duration and position queries to see if that works correctly too
Created attachment 249441 [details] [review] [PATCH] videorate: Add rate property gst/videorate/gstvideorate.c | 158 +++++++++++++++++++++++++++++++++++++++---- gst/videorate/gstvideorate.h | 10 ++- 2 files changed, 152 insertions(+), 16 deletions(-)
Created attachment 249442 [details] [review] [PATCH] videorate: Add test_rate tests/check/elements/videorate.c | 100 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+)
Created attachment 249443 [details] [review] [PATCH] videorate: Add test_query_duration tests/check/elements/videorate.c | 54 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)
Created attachment 249444 [details] [review] [PATCH] videorate: Add test_query_position tests/check/elements/videorate.c | 66 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
Alright I've added 4 patches : * Add rate property. I've improved it to be able to manage variable rates. I also updated QUERY_POSITION and DURATION; * test_rate is almost unchanged; * test_query_duration: Check whether duration querying works properly; * test_query_position: Check whether position querying works properly; Don't be shy on comments :)
Review of attachment 249441 [details] [review]: ::: gst/videorate/gstvideorate.c @@ +747,3 @@ + segment.stop = (gint64) (segment.stop / videorate->rate); + segment.time = (gint64) (segment.time / videorate->rate); + segment.applied_rate = segment.applied_rate / videorate->rate; I think applied rate should be multiplied, not divided. If rate is two, you want applied rate to be two times higher. @@ +904,3 @@ + duration); + if (GST_CLOCK_TIME_IS_VALID (duration)) { + duration = (gint64) (duration / videorate->rate); While this is what pitch does, I think what would be better is to consider rate changes here. So take the upstream length, subtract the last rate change position, scale by the current rate. Then add that to the last rate change position. @@ +1063,3 @@ intime = in_ts + videorate->segment.base; + /* We have a new rate, which means prevbuf and inbuf don't have the same rate */ I don't understand this code. Can't you just remember the current position (including the previous offset) as offset, and from that on start calculations again from zero (and offset them by the current position)? This looks overly complicated.
Review of attachment 249442 [details] [review]: ::: tests/check/elements/videorate.c @@ +1053,3 @@ + + /* Next expected timestamp with fps 25/1 */ + *expected_ts += 40000000; The timestamps should be scaled by the rate, same goes for the durations. Or am I missing something here? So they won't actually be +=GST_SECOND/25 but +=(GST_SECOND/25) / rate? Or is the framerate kept, just the speed increased (so frames dropped/duplicated)? @@ +1085,3 @@ + + /* Push 0.5sec of buffers, output expected 1sec with rate 0.5 */ + for (ts = 0; ts < 0.5 * GST_SECOND; ts += GST_SECOND / 33) { Shouldn't this be GST_SECOND / 25 (i.e. 25fps)?
Review of attachment 249443 [details] [review]: ::: tests/check/elements/videorate.c @@ +1179,3 @@ + gst_pad_peer_query (mysrcpad, query); + gst_query_parse_duration (query, NULL, &duration); + fail_unless_equals_uint64 (duration, GST_SECOND); After implementing the more intelligent duration query that considers rate changes, please add tests that check if after rate changes the duration scales accordingly with offset.
Review of attachment 249444 [details] [review]: ::: tests/check/elements/videorate.c @@ +1218,3 @@ + + fail_unless_equals_int (gst_pad_push (mysrcpad, inbuf), GST_FLOW_OK); + } Check for the rate=2.0 case for each buffer too @@ +1228,3 @@ + if (ts == 0.75 * GST_SECOND) { + gst_element_query_position (videorate, GST_FORMAT_TIME, &position); + fail_unless_equals_uint64 (position, 0.48 * GST_SECOND); And here just do it for a) every buffer and b) just calculate the correct position by offsetting after the above loop and scaling from the current
It would be nice if this could be automatically activated if: * rate != 1.0 * segment has GST_SEGMENT_FLAG_SKIP And automatically output a stream with: * rate == 1.0 * applied_rate = incoming rate * drop/duplicate packets to end up with a smooth stream
Created attachment 255125 [details] [review] [PATCH] videorate: Add fixed rate property gst/videorate/gstvideorate.c | 159 ++++++++++++++++++++++++++++++++++++++++--- gst/videorate/gstvideorate.h | 4 +- 2 files changed, 151 insertions(+), 12 deletions(-)
Created attachment 255126 [details] [review] [PATCH] videorate: Add test_rate property tests/check/elements/videorate.c | 98 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+)
Created attachment 255127 [details] [review] [PATCH] videorate: Add test_query_duration tests/check/elements/videorate.c | 55 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
Created attachment 255128 [details] [review] [PATCH] videorate: Add test_query_position tests/check/elements/videorate.c | 66 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)
Sebastian, I've just uploaded a fixed-rate version of my patches. So they don't consider rate changes for now. Could they be merged if they're legit? Also, I don't think applied_rate has to be changed. As we talked with Edward during GUADEC, the goal of the rate property (used by pitivi) is that the resulting stream is playing at the *normal* rate (even though it plays differently), it's just the processing before that scaled it.
Sebastian, any time to look at that again ? I'd like to have this :)
Review of attachment 255125 [details] [review]: Generally looks good but the following. Forbid rate changes during states > READY maybe ::: gst/videorate/gstvideorate.c @@ +747,3 @@ + if (GST_CLOCK_TIME_IS_VALID (segment.stop)) + segment.stop = (gint64) (segment.stop / videorate->rate); + segment.time = (gint64) (segment.time / videorate->rate); These calculations assume that the rate was always the same before, and also will never change in the future @@ +955,3 @@ + duration); + if (GST_CLOCK_TIME_IS_VALID (duration)) { + duration = (gint64) (duration / videorate->rate); Same here @@ +975,3 @@ + dst_value = + (gint64) (gst_segment_to_stream_time (&videorate->segment, + GST_FORMAT_TIME, videorate->last_ts / videorate->rate)); and here
Review of attachment 255126 [details] [review]: ::: tests/check/elements/videorate.c @@ +1044,3 @@ +} RateInfo; + +static RateInfo rate_tests[] = { not that it matters much in a test but make it const ;) @@ +1050,3 @@ + .expected_out = 25, + .expected_drop = 8, + .expected_dup = 0}, This is C99 syntax, use something C89 compatible (e.g. don't name fields)
Review of attachment 255128 [details] [review]: ::: tests/check/elements/videorate.c @@ +1204,3 @@ + .rate = 2.0}, + { + .rate = 1.7}, C99 syntax again @@ +1273,3 @@ tcase_add_loop_test (tc_chain, test_rate, 0, G_N_ELEMENTS (rate_tests)); tcase_add_test (tc_chain, test_query_duration); + tcase_add_test (tc_chain, test_query_position); Shouldn't this be a loop_test too?
Joris?
Hi, I'm not dead, I'm passing my exams right now, and searching an internship. I'll get back to gstreamer/pitivi soon hopefully.
This bug got stalled before the 1st Go Pro went out to the market. Nowadays, no slow motion means no PiTiVi for some users (me, at least :P ), which is sad because I love the rest of the project & GStreamer stack. Has there been any progress/intention/roadmap on this since then?
Bugzilla is not a forum, use mailing list for questions.
Hi, sorry I gave up on this quite some time ago. If anyone is willing to work on this again please do so :)
Comment on attachment 255127 [details] [review] [PATCH] videorate: Add test_query_duration What's the status here now? Anybody working on this?
Created attachment 339067 [details] [review] videorate: Add fixed rate property I rebased those patches and merged them all in one single one, I made minor changes in the tests and addressed all your comments: (In reply to Sebastian Dröge (slomo) from comment #31) > Review of attachment 255125 [details] [review] [review]: > > Generally looks good but the following. Forbid rate changes during states > > READY maybe > Added GST_PARAM_MUTABLE_READY which should be good enough. (In reply to Sebastian Dröge (slomo) from comment #32) > Review of attachment 255126 [details] [review] [review]: > > ::: tests/check/elements/videorate.c > @@ +1044,3 @@ > +} RateInfo; > + > +static RateInfo rate_tests[] = { > > not that it matters much in a test but make it const ;) I enhanced sensibly the test and those structs are not const anymore > @@ +1050,3 @@ > + .expected_out = 25, > + .expected_drop = 8, > + .expected_dup = 0}, > > This is C99 syntax, use something C89 compatible (e.g. don't name fields) Well, that syntax is used all around in that file... (In reply to Sebastian Dröge (slomo) from comment #33) > Review of attachment 255128 [details] [review] [review]: ... > @@ +1273,3 @@ > tcase_add_loop_test (tc_chain, test_rate, 0, G_N_ELEMENTS (rate_tests)); > tcase_add_test (tc_chain, test_query_duration); > + tcase_add_test (tc_chain, test_query_position); > > Shouldn't this be a loop_test too? DONE
(Just discovered a big issue with EOS not properly being sent, fixing it :))
Created attachment 339125 [details] [review] videorate: Add fixed rate property Properly handled seqnums avoid the previously mentionned failure (which was happening only when using NLE).
Attachment 339125 [details] pushed as 658ee6f - videorate: Add fixed rate property
commit f9d110f596e3a81722e8af1586fea633f8d6741c Author: Tim-Philipp Müller <tim@centricular.com> Date: Mon Jan 23 19:45:05 2017 +0000 videorate: fix LATENCY query The latency query originally had a fallthrough to the default label at the end as fallback, but that got messed up when the DURATION and POSITION queries were added, so it then fell through to the duration query handler instead. Restore original behaviour. https://bugzilla.gnome.org/show_bug.cgi?id=699077 commit d6c0e9072b4a16c672ffb638befebc66df8d900c Author: Tim-Philipp Müller <tim@centricular.com> Date: Mon Jan 23 19:08:15 2017 +0000 videorate: fix duration and position query handling Duration query would return TRUE and duration=-1. This worked in the unit test because the unit test implementation was a bit broken. Both queries need to access rate with a lock. Fix broken duration query test as well. It relied on broken behaviour by the videorate query handler, and also it was implemented as a downstream query rather than an upstream query. And we must return HANDLED from the probe so that the query we intercept actually returns TRUE. https://bugzilla.gnome.org/show_bug.cgi?id=699077