GNOME Bugzilla – Bug 655112
videomaxrate: add "max-rate" property and improve caps negotiation
Last modified: 2011-10-29 15:18:31 UTC
Created attachment 192450 [details] [review] proposed patch Currently videomaxrate always claims it's source and sink caps include framerates going from [0/1..maxint/1], but the only thing it can do is drop extra frames. So it's not actually able to transform an input framerate of say 10 fps to 15 fps. Furthermore for use-cases where you want to framerate to be adjusted to a specific maximum dynamically the current need to dynamically adjust a capsfilter behind the maxrate is very cumbersome as there is always a race between setting the filter, the renegotiation happening and buffers with the old caps still being pushed through the capsfilter. Allowing the capsfilter to still have the old caps as a second option doesn't really work as that doesn't guarantee the right re-negotiation will happen (e.g. the old-caps might allow for a pass-through, which base-transform prefers). To solve these issues. Add a property to dynamically adjust the internal maximum rate of videomaxrate, which can ensure the right caps negotation occurs and change the caps to prefer to output a framerate in the range the source provides, but can deliver anything from 0 to MIN( configured maximum, maximum framerate of the source). And as input we require at least the minium the sink requires and prefer a framerate in the range we want to output, but can accept anything that's higher as well.
Could I maybe convince you to add a few simple pipeline tests to the testsuite for this?
Created attachment 193417 [details] [review] Updated patch, support more types of caps Updated patch, use a different way of clamping. by intersecting the incoming caps with the widest framerate range so we can easily get the intersection
(In reply to comment #1) > Could I maybe convince you to add a few simple pipeline tests to the testsuite > for this? You can, quite easily :). I didn't have the time to do that when i wrote the original patch unfortunately, but i'm hoping to have some time for adding those this week.
Updated in branch: http://cgit.collabora.com/git/user/sjoerd/gst-plugins-bad.git/log/?h=videomaxrate Testcase added and some small bugfix done.
Is this still needed now that videorate has support for generating a stream with a maximum bitrate? Do the same changes apply to videorate too?
Actually, I think videorate should be ported over to basetransform and made to support upstream caps negotiation and this code should go in there.
Porting videorate over to basetransform makes it ignore the order of the caps though, which is a nice feature of videorate.
(In reply to comment #6) > Actually, I think videorate should be ported over to basetransform and made to > support upstream caps negotiation and this code should go in there. Patch to support the current videorate code to basetransform is available in #657319
Should this be marked as a duplicate of bug #657319 then?
(In reply to comment #9) > Should this be marked as a duplicate of bug #657319 then? Neh, let's keep #657319 for porting the current videorate code to basetransform, when that's landed i'll update the patches here to apply on top of videorate. smaller chunks should be hopefully easier to track
Ok, let's reopen this one then
Created attachment 196920 [details] [review] videorate: Add more strict caps negotiation First step, make the caps negotiation more strict
Created attachment 196921 [details] [review] videorate: Add test for caps negotiation
Review of attachment 196920 [details] [review]: ::: gst/videorate/gstvideorate.c @@ +274,3 @@ + *max_num = gst_value_get_fraction_numerator (max); + *max_denom = gst_value_get_fraction_denominator (max); + } else if (GST_VALUE_HOLDS_LIST (v)) { It could also be a fraction range and quite often is @@ +327,3 @@ + s2 = gst_structure_copy (s); + + if (videorate->drop_only) { You should probably call gst_base_transform_reconfigure() whenever this property changes now
(In reply to comment #14) > Review of attachment 196920 [details] [review]: > > ::: gst/videorate/gstvideorate.c > @@ +274,3 @@ > + *max_num = gst_value_get_fraction_numerator (max); > + *max_denom = gst_value_get_fraction_denominator (max); > + } else if (GST_VALUE_HOLDS_LIST (v)) { > > It could also be a fraction range and quite often is The previous case checks for that already or am i missing something? > @@ +327,3 @@ > + s2 = gst_structure_copy (s); > + > + if (videorate->drop_only) { > > You should probably call gst_base_transform_reconfigure() whenever this > property changes now Done as part of my next patch :)
Created attachment 196985 [details] [review] Add maxrate property and make caps negotiation more strict git branch at: http://cgit.collabora.com/git/user/sjoerd/gst-plugins-base.git/log/?h=videorate This patch is a squashed version of the various patches (if someone prefers to have them all individually attached, please let me know)
Comment on attachment 196985 [details] [review] Add maxrate property and make caps negotiation more strict Looks good and yes, I missed the fraction range branch. sorry :)
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.