I suppose if the output framerate is variable, videorate should only drop frames?

If the output framerate is variable it should only drop/clip overlapping frames IMHO, yes.

Did this ever work before in older versions? If so, how?

I just noticed it with 1.4 because of bug 734425. I have a videorate in a pipeline with no restriction on the output framerate at all, so the 0/1 got passed through to the src side of the videorate.

A Ubuntu server running 1.2.4 seems to be affected, as well.

One question is: should the output framerate be variable by default, or shouldn't videorate rather try to fixate to 25fps or 30fps or whatever, since the whole point of adding a videorate element is usually to get a fixed framerate output?

I think it shouldn't force a fixed framerate unless downstream requests one or the relevant properties are set on videorate. Otherwise, which framerate would you like to default to?

You fixate to something nearest to 25fps or 30fps, or perhaps wait for the first two buffers before deciding based on the interval of those two.

But someone who puts a videorate into a pipeline expects a perfect stream to come out the other end. That requires a decision on some fixed framerate.

Yes, indeed. Automatic frame rate detection would be nice now that avdec_h264 does not do it anymore.

Though, arguably a "perfect" stream can still have a variable frame rate. IMO perfect just means that all frames have proper time stamps and durations. In that case videorate could just clip (and drop) frames and insert a single variable-duration gap frame for any gaps it encounters.

Perhaps a property could be used to force a fixed frame rate; or downstream could set caps with the framerate field being a LIST of acceptable rates or a FRACTION_RANGE that does not contain 0/1 but starts at a minimum acceptable rate.

That's not what videorate used to do and/or was supposed to do. People expect it to output a perfect and fixed-rate stream.  I don't mind adding a property that makes it do otherwise, but it should be opt-in, we shouldn't redefine the traditional functionality of videorate imho.

So videorate should:

To find input framerate:

If input caps have framerate > 0/1, set input framerate to that value.
Otherwise, if 'variable-rate' property is TRUE, set input framerate to 0/1.
Otherwise, set input framerate inferred from the the first two frames that arrive.

To fixate output caps:

If the input framerate is not acceptable in the output caps but 0/1 is, use 0/1 in the output caps.
Otherwise, fixate output caps using input framerate.

To find output framerate:

If output caps have framerate > 0/1, set output framerate to that value.
Otherwise, set output framerate to input framerate.

Created attachment 292379 [details] [review]
videorate: Do not loop forever pushing first buffer when variable framerate

In the case the framerate is variable (represented by framerate=0/1),
we currently end up loop pushing the first buffer and then recompute
diff1 and diff2 without updating the videorate->next_ts at all
leading to infinitely looping pushing that first buffer.

In the case of variable framerate, we should just compute the next_ts
as previous_pts + previous_duration.

Created attachment 292382 [details] [review]
videorate: Detect framerate if not forced to variable downstream

In case upstream does not provide videorate with framerate information,
it will detect the current framerate from the buffer it received,
but if downstream forces the use of variable framerate (most probably
through the use of a caps filter with framerate = 0 / 1), videorate will
respect that.

I figured we do not need a property but if we notice that downstream forces framerate to 0/1 then it means we need to do variable framerate.

If we go that way we will need the 2 patches so videorate can work in both modes.

Created attachment 292387 [details] [review]
videorate: Detect framerate if not forced to variable downstream

In case upstream does not provide videorate with framerate information,
it will detect the current framerate from the buffer it received,
but if downstream forces the use of variable framerate (most probably
through the use of a caps filter with framerate = 0 / 1), videorate will
respect that.

Review of attachment 292387 [details] [review]:

Shouldn't force_variable_rate be cleared in basetransform->stop?

Created attachment 292612 [details] [review]
videorate: Detect framerate if not forced to variable downstream

In case upstream does not provide videorate with framerate information,
it will detect the current framerate from the buffer it received,
but if downstream forces the use of variable framerate (most probably
through the use of a caps filter with framerate = 0 / 1), videorate will
respect that.

> Shouldn't force_variable_rate be cleared in basetransform->stop?

Indeed we should, fixed that.

Comment on attachment 292379 [details] [review]
videorate: Do not loop forever pushing first buffer when variable framerate

Maybe we should always set the duration to the timestamp differences. The duration on buffers is often inaccurate.

Review of attachment 292612 [details] [review]:

::: gst/videorate/gstvideorate.c
@@ +922,3 @@
 
+static void
+gst_video_rate_check_variable_rate (GstVideoRate * videorate,

Please add some comments here what this function tries to do :)

@@ +932,3 @@
+
+  g_object_get (GST_BASE_TRANSFORM_SRC_PAD (videorate), "caps", &srcpadcaps,
+      NULL);

Why not just gst_pad_get_current_caps()?

@@ +933,3 @@
+  g_object_get (GST_BASE_TRANSFORM_SRC_PAD (videorate), "caps", &srcpadcaps,
+      NULL);
+  gst_video_guess_framerate (GST_BUFFER_PTS (buffer), &fps_n, &fps_d);

This function expects a buffer duration, not a pts. But even if you use a duration here, what will happen if that duration was inaccurate or the average duration of frames changes over time?

@@ +941,3 @@
+  pad = gst_pad_get_peer (GST_BASE_TRANSFORM_SRC_PAD (videorate));
+  if (pad == NULL)
+    pad = gst_object_ref (GST_BASE_TRANSFORM_SRC_PAD (videorate));

If there is no peer, why not just go out of here immediately? Checking the videorate srcpad if it accepts the caps seems useless.

@@ +958,3 @@
+
+  gst_pad_push_event (GST_BASE_TRANSFORM_SRC_PAD (videorate),
+      gst_event_new_caps (tmpcaps));

This seems potentially problematic as it bypasses the basetransform caps handling. Maybe should be gst_base_transform_reconfigure_src()?

Review of attachment 292612 [details] [review]:

::: gst/videorate/gstvideorate.c
@@ +932,3 @@
+
+  g_object_get (GST_BASE_TRANSFORM_SRC_PAD (videorate), "caps", &srcpadcaps,
+      NULL);

DONE

@@ +933,3 @@
+  g_object_get (GST_BASE_TRANSFORM_SRC_PAD (videorate), "caps", &srcpadcaps,
+      NULL);
+  gst_video_guess_framerate (GST_BUFFER_PTS (buffer), &fps_n, &fps_d);

Basically I take the timestamp of the second buffer to compute the framerate should be buffer->pts - prev->ts actually here (DONE), and indeed I do not know if it will be the average and I make the assumption that timestamps from upstream are correct.

@@ +941,3 @@
+  pad = gst_pad_get_peer (GST_BASE_TRANSFORM_SRC_PAD (videorate));
+  if (pad == NULL)
+    pad = gst_object_ref (GST_BASE_TRANSFORM_SRC_PAD (videorate));

Indeed, DONE

@@ +958,3 @@
+
+  gst_pad_push_event (GST_BASE_TRANSFORM_SRC_PAD (videorate),
+      gst_event_new_caps (tmpcaps));

And What do I do with the current buffers? IIUC that implies I need to wait for the next buffer to be chained.

Created attachment 292691 [details] [review]
videorate: Detect framerate if not forced to variable downstream

In case upstream does not provide videorate with framerate information,
it will detect the current framerate from the buffer it received,
but if downstream forces the use of variable framerate (most probably
through the use of a caps filter with framerate = 0 / 1), videorate will
respect that.

Just to be clear, what I think should happen is:

 1) if downstream forces a particular framerate, we should use that

 2) if upstream caps contain a non-0/1 framerate, we should use that and pass it on downstream (if possible; otherwise fixate_to_nearest)

 3) if upstream framerate is 0/1 and downstream doesn't force a particular framerate, we try to guess based on buffer intervals and use that as output framerate

 4) What does videorate do if upstream framerate=0/1 and downstream rate=0/1 ? passthrough?

3) could even have two cases, one where a buffer duration is set (which could be used to guess the framerate then, and one where none is set, in which case one would have to look at two buffers). Looking at interval is probably preferable though.

4) No, videorate should never be passthrough. Even if a variable rate is provided and wanted, it still needs to be able to clip frames or inject frames to ensure the timestamps and durations match (the stream is perfect).

(In reply to comment #21)
> Just to be clear, what I think should happen is:
> 
>  1) if downstream forces a particular framerate, we should use that
> 
>  2) if upstream caps contain a non-0/1 framerate, we should use that and pass
> it on downstream (if possible; otherwise fixate_to_nearest)
> 
>  3) if upstream framerate is 0/1 and downstream doesn't force a particular
> framerate, we try to guess based on buffer intervals and use that as output
> framerate

That is precisely what that patch does.

>  4) What does videorate do if upstream framerate=0/1 and downstream rate=0/1 ?
> passthrough?

downstream forces 0/1 so we will do variable framerate.

Can we get tests for all the possible cases?


(In reply to comment #19)
> @@ +933,3 @@
> +  g_object_get (GST_BASE_TRANSFORM_SRC_PAD (videorate), "caps", &srcpadcaps,
> +      NULL);
> +  gst_video_guess_framerate (GST_BUFFER_PTS (buffer), &fps_n, &fps_d);
> 
> Basically I take the timestamp of the second buffer to compute the framerate
> should be buffer->pts - prev->ts actually here (DONE), and indeed I do not know
> if it will be the average and I make the assumption that timestamps from
> upstream are correct.

That assumption was wrong and only worked for the case if the first buffer has a timestamp of 0 :) Thanks for fixing.

> @@ +958,3 @@
> +
> +  gst_pad_push_event (GST_BASE_TRANSFORM_SRC_PAD (videorate),
> +      gst_event_new_caps (tmpcaps));
> 
> And What do I do with the current buffers? IIUC that implies I need to wait for
> the next buffer to be chained.

Yes. Maybe basetransform needs new API for your use case here?

(In reply to comment #25)
> Can we get tests for all the possible cases?

Would need to take the time for that.
 
> > And What do I do with the current buffers? IIUC that implies I need to wait for
> > the next buffer to be chained.
> 
> Yes. Maybe basetransform needs new API for your use case here?

I can not see a way to do that with the baseclass now, I do not think it breaks right now but it might be safe in the future.

What API do you think we should add?

Created attachment 297447 [details] [review]
basetransform: Add a method to let subclasses cleanly update srcpad caps

API:
    gst_base_transform_update_src

Created attachment 297448 [details] [review]
videorate: Detect framerate if not forced to variable downstream

In case upstream does not provide videorate with framerate information,
it will detect the current framerate from the buffer it received,
but if downstream forces the use of variable framerate (most probably
through the use of a caps filter with framerate = 0 / 1), videorate will
respect that.

And add some unit tests

Comment on attachment 297447 [details] [review]
basetransform: Add a method to let subclasses cleanly update srcpad caps

This would probably be better name gst_base_transform_set_caps() instead. Also this should probably trigger reconfiguration on the next buffer (to select proper buffer pools and allocators).

(In reply to Sebastian Dröge (slomo) from comment #29)
> Comment on attachment 297447 [details] [review] [review]
> basetransform: Add a method to let subclasses cleanly update srcpad caps
> 
> This would probably be better name gst_base_transform_set_caps() instead.
> Also this should probably trigger reconfiguration on the next buffer (to
> select proper buffer pools and allocators).

The problem with set_caps is that we already have a GstBaseTransform->set_caps vmethod which has a different semantic.

I will gst_pad_mark_reconfigure (srcpad) in that method, sure.

Created attachment 297740 [details] [review]
basetransform: Add a method to let subclasses cleanly update srcpad caps

API:
    gst_base_transform_update_src

set_src_caps()? update_src_caps()?

Created attachment 297774 [details] [review]
videorate: Detect framerate if not forced to variable downstream

In case upstream does not provide videorate with framerate information,
it will detect the current framerate from the buffer it received,
but if downstream forces the use of variable framerate (most probably
through the use of a caps filter with framerate = 0 / 1), videorate will
respect that.

And add some unit tests

Created attachment 297775 [details] [review]
basetransform: Add a method to let subclasses cleanly update srcpad caps

API:
    gst_base_transform_update_src

Ping? :)

Let's get it in, what are you waiting for? ;)

Attachment 297775 [details] pushed as 506afa5 - basetransform: Add a method to let subclasses cleanly update srcpad caps

Attachment 292379 [details] pushed as 1cda538 - videorate: Do not loop forever pushing first buffer when variable framerate
Attachment 297774 [details] pushed as ae86dec - videorate: Detect framerate if not forced to variable downstream

Was missing the since

commit 43f2f925a97e4f81f7f57449a9b35d6f95973da9
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Apr 2 17:32:42 2015 -0400

    basetransform: Add Since mark for new method

Bug #748635 reports a regression which might be related to these patches (did not investigate yet).

(In reply to Tim-Philipp Müller from comment #41)
> Bug #748635 reports a regression which might be related to these patches
> (did not investigate yet).

I just notice that these patches introduce videorate->updating_caps, which may get set to TRUE once, and is never reset. This will cause miss-behaviour when doing to NULL state and restarting. They also access unprotected class members within the transform function which can get triggered by the streaming thread but also through linking. This could potentially be an issue in dynamic pipelines. I believe best would be to file new bugs considering the time this has been introduced.