[PLUGIN MOVE] Move GstVideoAggregator to gst-plugins-base
Submitted by Thibault Saunier @thiblahute
Link to original bug (#739011)
Description
The GstVideoAggregator should be moved to gst-plugins-base and compositor should replace the old videomixer.
Depends on
Blocking
Activity
@slomosaid:As discussed on IRC, the current API with the GstVideoConverter is a bit magic and ugly :)
Instead of having a boolean flag in the class for that, there should be vfuncs for setting up converters and preparing frames. This would also be beneficial for compositors as it might want to do more than the automatic conversion (e.g. rescaling), and also for glvideomixer which currently has its own "prepare frames" thing inside the aggregated vfunc.
@thiblahutesaid:(In reply to Jian Li from comment 3)
Is there a plan for this moving? Which version will have GstVideoAggregator
in gst-plugins-base? I think this can simplify the developing of a new
hardware based video compositor plugin.It is too late for it to be included in 1.6, we will try to get it in for 1.8.
Any help is welcome :)
@thaytansaid:Just a note - the interaction glmixer/glvideomixer has with gstvideoaggregator is awkward at the moment. They end up setting several of the vfuncs to NULL - I guess to avoid the default logic around converters, scaling and alpha.
It seems like there might be scope for moving some of that functionality down the stack, or making the default implementations more generic, with specialisations for software aggregators that need explicit converters in the sink pad path.
Either that, or the input conversions that glmixer is currently running itself on the sink pad path should be done by integrating with the gstvideoaggregator converters logic, I'm not sure.
One small suggestion: Add a vfunc to aggregator that is called whenever one of the sink pads receives a CAPS event. In my custom compositor, I need to recheck the configuration if one of the input streams changes its caps. Right now, I have to use my own sink_event function for that. It would be cleaner if there were a dedicated vfunc.
@thiblahutesaid:(In reply to Carlos Rafael Giani from comment 7)
One small suggestion: Add a vfunc to aggregator that is called whenever one
of the sink pads receives a CAPS event. In my custom compositor, I need to
recheck the configuration if one of the input streams changes its caps.
Right now, I have to use my own sink_event function for that. It would be
cleaner if there were a dedicated vfunc.I am not sure we want that, we already added a vfunc in the pad class to handle events, we should not go and add vfunc for each event type.
(In reply to Thibault Saunier from comment 8)
I am not sure we want that, we already added a vfunc in the pad class to
handle events, we should not go and add vfunc for each event type.Fair point. But don't you mean GstAggregatorClass? I see no vfuncs in the pad class other than "flush".
@thiblahutesaid:(In reply to Carlos Rafael Giani from comment 9)
(In reply to Thibault Saunier from comment 8)
I am not sure we want that, we already added a vfunc in the pad class to
handle events, we should not go and add vfunc for each event type.Fair point. But don't you mean GstAggregatorClass? I see no vfuncs in the
pad class other than "flush".@ndufresnesaid:A quick .h review let me know that it needs work.
In the Class structure:
/* Output caps */
GstVideoInfo info;Comment is wrong, maybe call this src_info or out_info ?
GstCaps * (*update_caps) (GstVideoAggregator * videoaggregator,
I'm not sure I understand from reading the doc. I'm sure it's useful though.
GstFlowReturn (*aggregate_frames) (GstVideoAggregator * videoaggregator,
GstBuffer * outbuffer);
GstFlowReturn (*get_output_buffer) (GstVideoAggregator * videoaggregator,
GstBuffer ** outbuffer);If one implements both, what does it give over implementing aggregate() ? Just asking APIs whise. Also, get_output_buffer, should it be something like allocate_output_buffer ?
GstCaps *sink_non_alpha_caps;
Undocumented, I can barely guess what this is about.
@slomosaid:Ok so to summarize: The only thing missing here for moving it in this cycle is someone untangling the conversion and alpha handling. Which might mean to move them to new vfuncs / changed vfuncs so that glmixer can skip that behaviour / implement its own behaviour in a less awkward way.
Correct?
@slomosaid:Ok I looked at all this again, the following needs to be done before moving IMHO. Let's target that for 1.16 :)
- Make the API more similar with the GstAudioAggregator. vfunc names, function names, what vfuncs are there, when are they called
Go through the API one by one and match
- Conversion should probably be done the same way as in audioaggregator: a new pad subclass that does conversion, and the current videoaggregatorpad does not do any conversion at all. This should come with vfuncs as needed so that there is no magic negotiation happening in the videoaggregator/videoaggregatorpad base classes.
This should also already solve the special alpha handling and special caps negotiation that is currently there. It would be handled in the converter pad subclass, not generically. because generically all pad caps would have to match. GL mixer could then implement its own code for this, as it makes sense.
Note that there is no conversion on the srcpad happening in videoaggregator, and it does not have to.
-
Merge the videoaggregatorpad.h into the main header. There's no need for it to be separate.
-
The pad ignore-eos property should probably be renamed. What it does is that the last frame is going to be used forever instead of stopping at timestamp+duration. It does not really ignore EOS.
See also comment 13, which was confused by that (it is raw video specific!)
-
Don't implement childproxy interface in the base class but let subclasses decide about that, just like with audioaggregator. Subclasses might not actually want this implementation but a different one.
-
Buffer pool configuration validation, see comment 14 (also API for getting the buffer pool, decide/propose_allocation, etc like in audioaggregator)
After that I'm fine with moving it. Generally looks all good to me!
assigned to @tpm
