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.

That part is fixed now ↑

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.

(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 :)

Thanks for you information!

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.

(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".

(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".

Indeed I meant http://phabricator.freedesktop.org/diffusion/GSTBAD/browse/master/gst-libs/gst/base/gstaggregator.h;82c0189b2842e8729e82a4e73491dffc977bc7c2$218

What the current state of play here?

Any major issues that have not been mentioned yet?

Main issue I can find in the backlog is Jan's comment about the awkward interaction of the gl subclasses with the conversion stuff.

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.

> PROP_PAD_IGNORE_EOS

Why is this video specific ?

>gstvideoaggregator.c:1972
>  gst_buffer_pool_set_config (pool, config);

This is missing the little validation dance. We may endup with an unconfigured pool, then fail activation later (instead of falling back to generic video pool).

Just so we don't forget:

<ndufresne> __tim, ok, did a quick review, next step would be to dig into the interections between glmixer/compositor and this
<ndufresne> the caps/alpha thingy seems like a miss-fit

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?

That is my understanding too, and it's not clear to me even that that is a blocker, we could just decide to leave it as-is :)

Ok I looked at all this again, the following needs to be done before moving IMHO. Let's target that for 1.16 :)

1) 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

2) 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.

3) Merge the videoaggregatorpad.h into the main header. There's no need for it to be separate.

4) 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!)

5) 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.

6) 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!

These are now all done, from my point of view we're ready to move GstVideoAggregator, compositor and the GL mixers.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/137.