GNOME Bugzilla – Bug 740768
videoaggregator: Expose vmethods to set converters and prepare frames
Last modified: 2014-11-27 18:13:49 UTC
This gives more flexibility to the subclasses and permits to remove the GstVideoAggregatorClass->disable_frame_conversion ugly API. WARNING: This breaks the API as it removes the disable_frame_conversion field API: + GstVideoAggregatorPadClass->set_converter + GstVideoAggregatorPadClass->prepare_frame - GstVideoAggregatorClass->disable_frame_conversion
Created attachment 291573 [details] [review] videoaggregator: Expose vmethods to set converters and prepare frames
Created attachment 291589 [details] [review] videoaggregator: Expose vmethods to set converters and prepare frames This gives more flexibility to the subclasses and permits to remove the GstVideoAggregatorClass->disable_frame_conversion ugly API. WARNING: This breaks the API as it removes the disable_frame_conversion field API: + GstVideoAggregatorPadClass->set_converter + GstVideoAggregatorPadClass->prepare_frame - GstVideoAggregatorClass->disable_frame_conversion
As mentioned on IRC; maybe set_converter() should be set_info() and be called whenever the sinkpad caps are changing in one way or another? Additional to prepare_frame() there should maybe also be an "unprepare_frame". videoaggregator already has something like that to reset whatever it did in prepare_frame().
Comment on attachment 291589 [details] [review] videoaggregator: Expose vmethods to set converters and prepare frames Otherwise looks good to me. I'll add the scaling support to compositor on top of this once this patch is done :)
Created attachment 291628 [details] [review] videoaggregator: Expose vmethods to set converters and prepare/clean frames This gives more flexibility to the subclasses and permits to remove the GstVideoAggregatorClass->disable_frame_conversion ugly API. WARNING: This breaks the API as it removes the disable_frame_conversion field API: + GstVideoAggregatorPadClass->set_format + GstVideoAggregatorPadClass->prepare_frame + GstVideoAggregatorPadClass->clean_frame - GstVideoAggregatorClass->disable_frame_conversion
(In reply to comment #3) > As mentioned on IRC; maybe set_converter() should be set_info() and be called It is done now. > whenever the sinkpad caps are changing in one way or another? Was already the case ifaics > Additional to prepare_frame() there should maybe also be an "unprepare_frame". > videoaggregator already has something like that to reset whatever it did in > prepare_frame(). Indeed, that is usefull! Done.
Created attachment 291635 [details] [review] This patch: + Adds a new GstVideoAggregatorClass->find_best_format vmethod + Add a current_info parametter to GstVideoAggretorPadClass->set_info vmethod
Review of attachment 291635 [details] [review]: ::: gst-libs/gst/video/gstvideoaggregator.c @@ +149,3 @@ + gst_video_converter_free (pad->priv->convert); + + pad->priv->convert = NULL; Maybe the converter should actually be public in the pad. Conversion info already is anyway. @@ +222,3 @@ + + /* We wait until here to set the conversion infos, in case vagg->info changed */ + if (pad->need_conversion_update) { This seems wrong. If the info changes between this and setting up the converter above, then the converter will fail to convert.
Created attachment 291674 [details] [review] videoconvert: Hide all conversion related fields And do not delay the setting of the conversion_info
Attachment 291674 [details] pushed as a61d2e4 - videoconvert: Hide all conversion related fields