After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 699077 - videorate: add"rate" property to modify clip speed
videorate: add"rate" property to modify clip speed
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.x
Other Linux
: Normal enhancement
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 708545
 
 
Reported: 2013-04-27 20:49 UTC by Joris Valette
Modified: 2017-01-24 01:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch proposal for enhancement (4.22 KB, patch)
2013-04-27 20:49 UTC, Joris Valette
none Details | Review
New version of gstvideorate (8.86 KB, patch)
2013-07-10 16:46 UTC, Joris Valette
needs-work Details | Review
Test for rate property. May need review (3.85 KB, patch)
2013-07-10 16:47 UTC, Joris Valette
needs-work Details | Review
[PATCH] videorate: Add rate property (12.83 KB, patch)
2013-07-17 20:50 UTC, Joris Valette
needs-work Details | Review
[PATCH] videorate: Add test_rate (3.85 KB, patch)
2013-07-17 20:50 UTC, Joris Valette
needs-work Details | Review
[PATCH] videorate: Add test_query_duration (2.43 KB, patch)
2013-07-17 20:50 UTC, Joris Valette
needs-work Details | Review
[PATCH] videorate: Add test_query_position (2.78 KB, patch)
2013-07-17 20:50 UTC, Joris Valette
needs-work Details | Review
[PATCH] videorate: Add fixed rate property (11.33 KB, patch)
2013-09-17 17:34 UTC, Joris Valette
none Details | Review
[PATCH] videorate: Add test_rate property (3.57 KB, patch)
2013-09-17 17:35 UTC, Joris Valette
none Details | Review
[PATCH] videorate: Add test_query_duration (2.53 KB, patch)
2013-09-17 17:35 UTC, Joris Valette
none Details | Review
[PATCH] videorate: Add test_query_position (2.65 KB, patch)
2013-09-17 17:36 UTC, Joris Valette
none Details | Review
videorate: Add fixed rate property (19.52 KB, patch)
2016-11-03 19:59 UTC, Thibault Saunier
none Details | Review
videorate: Add fixed rate property (19.76 KB, patch)
2016-11-04 14:36 UTC, Thibault Saunier
committed Details | Review

Description Joris Valette 2013-04-27 20:49:23 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.
Comment 1 Nicolas Dufresne (ndufresne) 2013-04-27 22:44:27 UTC
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).
Comment 2 Nicolas Dufresne (ndufresne) 2013-04-27 22:51:15 UTC
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.
Comment 3 Joris Valette 2013-04-28 08:05:58 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2013-04-29 09:17:52 UTC
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.
Comment 5 Sebastian Dröge (slomo) 2013-04-29 09:20:21 UTC
Also the interface should probably be better modelled like the scaletempo plugin than pitch
Comment 6 Nicolas Dufresne (ndufresne) 2013-04-29 15:04:26 UTC
(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.
Comment 7 Sebastian Dröge (slomo) 2013-04-29 17:04:35 UTC
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!
Comment 8 Joris Valette 2013-05-08 14:23:13 UTC
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 ?
Comment 9 Nicolas Dufresne (ndufresne) 2013-05-08 15:53:36 UTC
(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.
Comment 10 Joris Valette 2013-07-10 16:46:59 UTC
Created attachment 248849 [details] [review]
New version of gstvideorate
Comment 11 Joris Valette 2013-07-10 16:47:53 UTC
Created attachment 248850 [details] [review]
Test for rate property. May need review
Comment 12 Joris Valette 2013-07-10 17:06:29 UTC
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).
Comment 13 Sebastian Dröge (slomo) 2013-07-11 10:38:12 UTC
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
Comment 14 Sebastian Dröge (slomo) 2013-07-11 10:44:47 UTC
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
Comment 15 Joris Valette 2013-07-17 20:50:15 UTC
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(-)
Comment 16 Joris Valette 2013-07-17 20:50:22 UTC
Created attachment 249442 [details] [review]
[PATCH] videorate: Add test_rate

 tests/check/elements/videorate.c | 100 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)
Comment 17 Joris Valette 2013-07-17 20:50:27 UTC
Created attachment 249443 [details] [review]
[PATCH] videorate: Add test_query_duration

 tests/check/elements/videorate.c | 54 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
Comment 18 Joris Valette 2013-07-17 20:50:32 UTC
Created attachment 249444 [details] [review]
[PATCH] videorate: Add test_query_position

 tests/check/elements/videorate.c | 66 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)
Comment 19 Joris Valette 2013-07-17 20:56:26 UTC
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 :)
Comment 20 Sebastian Dröge (slomo) 2013-07-18 10:09:13 UTC
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.
Comment 21 Sebastian Dröge (slomo) 2013-07-18 10:16:42 UTC
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)?
Comment 22 Sebastian Dröge (slomo) 2013-07-18 10:19:10 UTC
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.
Comment 23 Sebastian Dröge (slomo) 2013-07-18 10:22:53 UTC
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
Comment 24 Edward Hervey 2013-07-29 08:50:48 UTC
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
Comment 25 Joris Valette 2013-09-17 17:34:32 UTC
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(-)
Comment 26 Joris Valette 2013-09-17 17:35:18 UTC
Created attachment 255126 [details] [review]
[PATCH] videorate: Add test_rate property

 tests/check/elements/videorate.c | 98 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)
Comment 27 Joris Valette 2013-09-17 17:35:54 UTC
Created attachment 255127 [details] [review]
[PATCH] videorate: Add test_query_duration

 tests/check/elements/videorate.c | 55 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)
Comment 28 Joris Valette 2013-09-17 17:36:02 UTC
Created attachment 255128 [details] [review]
[PATCH] videorate: Add test_query_position

 tests/check/elements/videorate.c | 66 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)
Comment 29 Joris Valette 2013-09-17 17:40:54 UTC
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.
Comment 30 Mathieu Duponchelle 2013-09-19 22:46:25 UTC
Sebastian, any time to look at that again ? I'd like to have this :)
Comment 31 Sebastian Dröge (slomo) 2013-10-07 14:08:02 UTC
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
Comment 32 Sebastian Dröge (slomo) 2013-10-07 14:10:13 UTC
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)
Comment 33 Sebastian Dröge (slomo) 2013-10-07 14:11:29 UTC
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?
Comment 34 Sebastian Dröge (slomo) 2014-01-06 09:29:07 UTC
Joris?
Comment 35 Joris Valette 2014-01-06 12:41:16 UTC
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.
Comment 36 Yajo 2016-09-26 13:48:16 UTC
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?
Comment 37 Nicolas Dufresne (ndufresne) 2016-09-26 14:04:53 UTC
Bugzilla is not a forum, use mailing list for questions.
Comment 38 Joris Valette 2016-09-26 14:12:49 UTC
Hi, sorry I gave up on this quite some time ago.
If anyone is willing to work on this again please do so :)
Comment 39 Sebastian Dröge (slomo) 2016-11-01 18:56:39 UTC
Comment on attachment 255127 [details] [review]
[PATCH] videorate: Add test_query_duration

What's the status here now? Anybody working on this?
Comment 40 Thibault Saunier 2016-11-03 19:59:25 UTC
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
Comment 41 Thibault Saunier 2016-11-03 20:37:39 UTC
(Just discovered a big issue with EOS not properly being sent, fixing it :))
Comment 42 Thibault Saunier 2016-11-04 14:36:16 UTC
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).
Comment 43 Thibault Saunier 2016-11-04 17:02:26 UTC
Attachment 339125 [details] pushed as 658ee6f - videorate: Add fixed rate property
Comment 44 Tim-Philipp Müller 2017-01-24 01:07:15 UTC
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