The crossfade-ratio is also broken in that if one modifies the the crossfade example in -bad to not set the background to black (and use the default checkerboard), during the transition the checkerboard shows through during the transition.

The goal of that API is to be straight forward to use and just work and my goal would be to have the same API for other video mixers. The glmixer API is far from fulfilling that goal to me.

Adding API for different blending modes is orthogonal to that API to me.

I have to concur with Thibault, while we did tie the addition of the crossfade-ratio with the new "additive" blend mode, this does not necessarily mean crossfade-ratio would have to go away, I must say that looking at glmixer's API it is really not clear if and how I can achieve the desired behaviour at a glance, while crossfade-ratio seems like a higher-level solution that I can grok more easily, maybe that's just me though :)

The problem with having such a specific property is that it makes extending difficult later. How should the crossfade-ratio property behave in combination with having set a specific blending mode?

Also having to keep the crossfade-ratio property would mean that all this special-casing in compositor has to stay, while with a generic blending configuration system there is not even the need for special-casing. You simply blend the different inputs on top of each other with the correct blending operations.

And having such a covenience property is nice, but maybe this should instead be the matter of some higher-level crossfade-compositor? The current behaviour of the property if more than 2 pads are there (and what Matthew wrote above) is also not very intuitive or useful.

(In reply to Sebastian Dröge (slomo) from comment #5)
> And having such a covenience property is nice, but maybe this should instead
> be the matter of some higher-level crossfade-compositor? The current
> behaviour of the property if more than 2 pads are there (and what Matthew
> wrote above) is also not very intuitive or useful.

That would work for me ;)

Does the API in glvideomixer also need reviewing before any move to -base?

(In reply to Tim-Philipp Müller from comment #7)
> Does the API in glvideomixer also need reviewing before any move to -base?

That one seems fine to me. It's directly exposing (more or less) how the GL API works.

> > Does the API in glvideomixer also need reviewing before any move to -base?
> 
> That one seems fine to me. It's directly exposing (more or less) how the GL
> API works.

So what's the plan then? Make up an abstraction that mirrors the glvideomixer API or something yet entirely different?

I think something entirely different, but something that can later be extended nicely with new blending operators. Something enum based :)

Thibault, for your specific use case would you be fine with e.g. an API that kind of maps to the Cairo API? You would set (in Cairo terms) SOURCE as operator for the bottom pad (completely overwrites the background) with alpha set to "crossfade-ratio), and then the top pad would be ADD and the alpha would be (1-"crossfade-ratio").

I'm not sure what behaviour you want for more than two pads (do you use this?), and the current implementation also seems broken as it bleeds through the background

(In reply to Sebastian Dröge (slomo) from comment #10)
> I think something entirely different, but something that can later be
> extended nicely with new blending operators. Something enum based :)
>
> Thibault, for your specific use case would you be fine with e.g. an API that
> kind of maps to the Cairo API? You would set (in Cairo terms) SOURCE as
> operator for the bottom pad (completely overwrites the background) with
> alpha set to "crossfade-ratio), and then the top pad would be ADD and the
> alpha would be (1-"crossfade-ratio").

Well, ideally we hide that away in a crossfade element as discussed before. Also this way we could basically keep the current code but expose that element, and we could have a `glcrossfade` element that has the exact same API.
 
> I'm not sure what behaviour you want for more than two pads (do you use
> this?), and the current implementation also seems broken as it bleeds
> through the background

The behaviour is basically "crossfade from the previous result of blending" I currently don't use it but we thought about the API in a way that would allow us to have 1 single compositor in the pipeline. To be honest I do not care so much about that feature and removing it is fine to me.

I'm asking so that we can figure out how that "videocrossfade" element should look like :)

For compositor I was thinking of adding a simple "source operator" property on the pads, which currently would have properties for SOURCE (just copy over), OVER (normal mode as of now), ADD (what the current crossfade property enables).

Then we have the current default behaviour preserved, and a way to do exactly what the crossfade property does right now but with more control over what happens instead of hardcoding one specific way of combining things.

One thing that is not clear to me is why the current crossfade code is doing ADD for the two pads, and then OVER for the background. Wouldn't you want the first pad to be directly replacing the background and the other pad crossfade into that one then? That's the reason for currently the background bleeding through.

Thibault, so how do we go ahead with this? I still don't understand what exactly you need in pitivi and the phabricator link to the original ticket is 404 now.

Especially it's not clear to me what exact blending behaviour you need (both with only two pads, but also with >2 and what should happen with the background), and why maybe the "weird" behaviours (see what I wrote above but also Matthew) in the current crossfade mode are actually required.

Once I understood that, I can come up with an API proposal for both compositor and a possible crossfadecompositor (or maybe a non-compositor-based crossfade element with just two pads that can also be extended later for other fades, see shapewipe and the smpte transitions for example).

(In reply to Sebastian Dröge (slomo) from comment #13)
> Thibault, so how do we go ahead with this? I still don't understand what
> exactly you need in pitivi and the phabricator link to the original ticket
> is 404 now.

Here is the issue: https://gitlab.gnome.org/GNOME/pitivi/issues/2044 (once more I am happy I imported the history :-))

> Especially it's not clear to me what exact blending behaviour you need (both
> with only two pads, but also with >2 and what should happen with the
> background), and why maybe the "weird" behaviours (see what I wrote above
> but also Matthew) in the current crossfade mode are actually required.

We require the current crossfade behaviour with 2 pads, our plans was to rely on the > 2 pads behavior but we can live without it without much trouble, we do not care about the background issue.

> Once I understood that, I can come up with an API proposal for both
> compositor and a possible crossfadecompositor (or maybe a
> non-compositor-based crossfade element with just two pads that can also be
> extended later for other fades, see shapewipe and the smpte transitions for
> example).

Cool, thinking about it I think we want a "videotransition" element which would allow to crossfade or use smpte transition (that would simplify a bit some code in GESVideoTransition actually) and maybe allow using either the compositor or glvideomixer.

Once we agree on the way forward I can also give a hand on some of the points.

(In reply to Thibault Saunier from comment #14)
>
> > Especially it's not clear to me what exact blending behaviour you need (both
> > with only two pads, but also with >2 and what should happen with the
> > background), and why maybe the "weird" behaviours (see what I wrote above
> > but also Matthew) in the current crossfade mode are actually required.
> 
> We require the current crossfade behaviour with 2 pads, our plans was to
> rely on the > 2 pads behavior but we can live without it without much
> trouble, we do not care about the background issue.

Do you know why you need the current behaviour though, with regards to background handling? Why do you blend both pads, and then blend them with the background (instead of just overwriting the background)?

The GitLab issue / former phab issue mentions that what you need is alpha-additive blending between the two pads in question.

That is (note that dst is the background, so the first pad with its own alpha property applied already!):
>   out[alpha] = min(1, src[alpha] * alpha + dst[alpha])
>   out[color] = (src[color] + dst[color]) / out[alpha]

Which without pre-multiplied alpha is
>   out[alpha] = min(1, src[alpha] * alpha + dst[alpha])
>   out[color] = src[color] * src[alpha] * alpha + dst[color] * dst[alpha]

The "additive" code however currently calculates (according to its comments):

>   out[alpha] = src[alpha] * alpha + dst[alpha] * (1 - src[alpha] * alpha) ** 2
>   out[color] = src[color] * src[alpha] * alpha + dst[color] * (1 - src[alpha] * alpha) ** 2
>   (note the ** 2 here !)

I'm not sure what the idea behind that calculation is.

-------

From what I understand what you want overall is that the alpha is the sum of both alphas with crossfading ratio built-in:

>   out[alpha] = min(1, src[alpha] * alpha_src * crossfade + dst[alpha] * alpha_dst * (1 - crossfade), where alpha_X are the alpha properties of the relevant pads
>   out[color] = src[color] * src[alpha] * alpha_src * crossfade + dst[color] * dst[alpha] * alpha_dst * (1 - crossfade)

Which is what you get from what I proposed above:

1) first pad on background with operator=SRC, alpha=alpha_dst * (1 - crossfade)

>  out[alpha] = src[alpha] * alpha_dst * (1 - crossfade)
>  out[color] = src[color]

2) second pad with operator=ADD, alpha=alpha_src * crossfade

>  out[alpha] = min(1, src[alpha] * alpha_src * crossfade + dst[alpha]) = {inserting step 1 to do it all in one go} min(1, src[alpha] * alpha_src * crossfade + dst[alpha] * alpha_dst * (1 - crossfade))
>  out[color] = src[color] * src[alpha] * alpha_src * crossfade + dst[color] * dst[alpha] = {inserting step 1 to do it all in one go} src[color] * src[alpha] * alpha_src * crossfade + dst[color] * dst[alpha] * alpha_dst * (1 - crossfade)


Also Bugzilla is kind of annoying for writing comments like this.

I do not remember the details of the formula tbh but what you state is not correct, we have both alpha and crossfade-ratio so that for example you have a clip with an alpha and a transition, it *should* bleed through because of the alpha channel of the first clip, the formula takes that into account.

> Do you know why you need the current behaviour though, with regards to background handling? Why do you blend both pads, and then blend them with the background (instead of just overwriting the background)?

Now I remember and this behavior is expected, basically if the input clip has alpha, that alpha needs to be kept, and in that case the background is consider as an "input clip". We do not want the result of a transition to be opaque as you describe.

Ah so you want something like "background OVER (in1 ADD in2)", and the difficult part here are the parenthesis? I.e. "(background OVER in1) ADD in2" would be possible but that has a different result. What you want would need a second compositor in my proposal.

Cairo defines the DEST_OVER operator for such a thing, which is OVER but the other way around. So what you want could be implemented as "(in1 ADD in2) DEST_OVER background" but blending the background last is a bit weird.

I believe glvideomixer also can't do what you want currently, please correct me Matthew :)

----

So to go one step back, what you want is that if the crossfade of the two pads is not opaque, then the background should shine through. Not based on the alpha of the first pad, but based on the combined alpha of both pads. Which kind of violates the painter's algorithm as implemented by compositor and which is why the crossfading code requires so many special cases everywhere.

Maybe a cleaner approach would then be a crossfade/transition element, and then the output can be composited over a background? Or your background would be a third input of the compositor and applied with DEST_OVER? What is your background btw, just black or fully transparent?

(In reply to Sebastian Dröge (slomo) from comment #17)
> Ah so you want something like "background OVER (in1 ADD in2)", and the
> difficult part here are the parenthesis? I.e. "(background OVER in1) ADD
> in2" would be possible but that has a different result. What you want would
> need a second compositor in my proposal.
> 
> Cairo defines the DEST_OVER operator for such a thing, which is OVER but the
> other way around. So what you want could be implemented as "(in1 ADD in2)
> DEST_OVER background" but blending the background last is a bit weird.
> 
> I believe glvideomixer also can't do what you want currently, please correct
> me Matthew :)
> 
> ----
> 
> So to go one step back, what you want is that if the crossfade of the two
> pads is not opaque, then the background should shine through. Not based on
> the alpha of the first pad, but based on the combined alpha of both pads.
> Which kind of violates the painter's algorithm as implemented by compositor
> and which is why the crossfading code requires so many special cases
> everywhere.

There is 1 if and a special function call to handle that, not sure I would call that "so many special cases everywhere" :D

> Maybe a cleaner approach would then be a crossfade/transition element, and
> then the output can be composited over a background? Or your background
> would be a third input of the compositor and applied with DEST_OVER? What is
> your background btw, just black or fully transparent?

That could work, not 100% convinced it the best option. Why do you think the background should be a third input? The background is black.

(In reply to Sebastian Dröge (slomo) from comment #17)
> I believe glvideomixer also can't do what you want currently, please correct
> me Matthew :)

This is all correct for glvideomixer. Mixing (in1 ADD in2) and placing that OVER the background is not possible with the current painters algorithm within one glvideomixer.

(In reply to Thibault Saunier from comment #18)
> (In reply to Sebastian Dröge (slomo) from comment #17)
> > the crossfading code requires so many special cases everywhere.
> 
> There is 1 if and a special function call to handle that, not sure I would
> call that "so many special cases everywhere" :D

It's enough to cause implementing any other blending modes to always account for the peculiar behaviour of 'crossfade-ratio' using non-standard blending for the painter's algorithm.

(In reply to Thibault Saunier from comment #18)

> There is 1 if and a special function call to handle that, not sure I would
> call that "so many special cases everywhere" :D

Not just the current code but what Matthew said. It makes extending the current compositor more complicated.

Also it's a special case in behaviour that always has to be kept in mind: both pads must be the same size or one of the pads is cropped to the other (I assume?), the positioning only has effect for one of the pads and is ignored for the other one (or is the positioning of one pad relative to the top-left corner of the other pad, and the other pad relative to the background?).

And if you have e.g. 3 pads and crossfading is applied on the second and third, will the output be "BACKGROUND OVER ((A ADD B) ADD C))" or "(BACKGROUND OVER (A ADD B)) ADD C"?

It's non-trivial additional cognitive load for figuring out the overall composition, and the interactions with other configuration on the compositor is not obvious at all.


That is my main concern with all this really, it seems like an ad-hoc solution for a current problem which might paint us into a corner later.

> > Maybe a cleaner approach would then be a crossfade/transition element, and
> > then the output can be composited over a background? Or your background
> > would be a third input of the compositor and applied with DEST_OVER? What is
> > your background btw, just black or fully transparent?
> 
> That could work, not 100% convinced it the best option. Why do you think the
> background should be a third input? The background is black.

I was not saying that it should be a third input, just that this would be how you could get the current behaviour with the painter's algorithm :)

I still think a proper crossfading/transition element for this use-case would be more useful and also more extendable in the future for other kinds of transitions without then having to find a way how to implement those inside compositor without making things more awkward.

(In reply to Sebastian Dröge (slomo) from comment #20)
> (In reply to Thibault Saunier from comment #18)
> 
> > There is 1 if and a special function call to handle that, not sure I would
> > call that "so many special cases everywhere" :D
> 
> Not just the current code but what Matthew said. It makes extending the
> current compositor more complicated.
> 
> Also it's a special case in behaviour that always has to be kept in mind:
> both pads must be the same size or one of the pads is cropped to the other
> (I assume?), the positioning only has effect for one of the pads and is
> ignored for the other one (or is the positioning of one pad relative to the
> top-left corner of the other pad, and the other pad relative to the
> background?).

Everything is relative to the background, no restriction on the sizes no... so no special casing in that regard.

> And if you have e.g. 3 pads and crossfading is applied on the second and
> third, will the output be "BACKGROUND OVER ((A ADD B) ADD C))" or
> "(BACKGROUND OVER (A ADD B)) ADD C"?

It is `BACKGROUND OVER ((A ADD B) ADD C)` in the current implementation iirc.

> It's non-trivial additional cognitive load for figuring out the overall
> composition, and the interactions with other configuration on the compositor
> is not obvious at all.
> 
> 
> That is my main concern with all this really, it seems like an ad-hoc
> solution for a current problem which might paint us into a corner later.

I agree on that, adding more blending type will be more complex/not obvious/ because of that feature.

> > > Maybe a cleaner approach would then be a crossfade/transition element, and
> > > then the output can be composited over a background? Or your background
> > > would be a third input of the compositor and applied with DEST_OVER? What is
> > > your background btw, just black or fully transparent?
> > 
> > That could work, not 100% convinced it the best option. Why do you think the
> > background should be a third input? The background is black.
> 
> I was not saying that it should be a third input, just that this would be
> how you could get the current behaviour with the painter's algorithm :)
>
> I still think a proper crossfading/transition element for this use-case
> would be more useful and also more extendable in the future for other kinds
> of transitions without then having to find a way how to implement those
> inside compositor without making things more awkward.

I guess we can go that route. Just move current code to another element and remove from compositor?

Maybe as a way forward:
1) Remove crossfade code from compositor, implement a SRC and ADD mode (we have the code for that already). That allows doing crossfading like now if all inputs are fully opaque. This can then be moved to -base with videoaggregator and everything.

2) Copy the old compositor to a new crossfadecompositor (better name please). That can then be used by pitivi for the time being until we implement a nicer and better crossfade/transition element. Which IMHO should also have additional constraints (like no positioning, only same input sizes, only two pads, ...).

(In reply to Sebastian Dröge (slomo) from comment #22)
> Maybe as a way forward:
> 1) Remove crossfade code from compositor, implement a SRC and ADD mode (we
> have the code for that already). That allows doing crossfading like now if
> all inputs are fully opaque. This can then be moved to -base with
> videoaggregator and everything.

Adding modes is not mandatory imo, but well, if you want :-)

> 2) Copy the old compositor to a new crossfadecompositor (better name
> please). That can then be used by pitivi for the time being until we
> implement a nicer and better crossfade/transition element. Which IMHO should
> also have additional constraints (like no positioning, only same input
> sizes, only two pads, ...).

Well, let's just have call it videotransition and have it have 2 pads, and let it grow in the future.

I do not think we want this kind of restriction at all, the end user can transition whatever he want, no reason to not let it do that!

> Well, let's just call it videotransition and have it have 2 pads, and let it grow in the future.

That was mean to have a "?" :-)

(In reply to Thibault Saunier from comment #23)

> I do not think we want this kind of restriction at all, the end user can
> transition whatever he want, no reason to not let it do that!

Not having such restrictions means more complicated internals and it restricts how you can change the internals / extend the internals :) But that's for somewhere else to discuss I guess.

(In reply to Sebastian Dröge (slomo) from comment #25)
> (In reply to Thibault Saunier from comment #23)
> 
> > I do not think we want this kind of restriction at all, the end user can
> > transition whatever he want, no reason to not let it do that!
> 
> Not having such restrictions means more complicated internals and it
> restricts how you can change the internals / extend the internals :) But
> that's for somewhere else to discuss I guess.

Well, this issue is exactly about that... you want to simplify the internals, at the cost of flexibility and even removing useful features (2 pads only (which is arguably not a mandatory feature), and now adding restriction which must *not* exist from a end user video editing app perspective).

(In reply to Sebastian Dröge (slomo) from comment #25)
> Not having such restrictions means more complicated internals and it
> restricts how you can change the internals / extend the internals :) But
> that's for somewhere else to discuss I guess.

But having such restrictions means the described element would be useless in pitivi's context, currently compositor provides a usable solution to a real use case and we should strive to preserve it.

Considering that new mixing stack was initially implemented to serve pitivi's use case better, it feels a bit awkward to now decide that its use case is not in scope for the version that eventually gets promoted :)

(In reply to Thibault Saunier from comment #26)
> (In reply to Sebastian Dröge (slomo) from comment #25)
> > (In reply to Thibault Saunier from comment #23)
> > 
> > > I do not think we want this kind of restriction at all, the end user can
> > > transition whatever he want, no reason to not let it do that!
> > 
> > Not having such restrictions means more complicated internals and it
> > restricts how you can change the internals / extend the internals :) But
> > that's for somewhere else to discuss I guess.
> 
> Well, this issue is exactly about that... you want to simplify the
> internals, at the cost of flexibility and even removing useful features (2
> pads only (which is arguably not a mandatory feature), and now adding
> restriction which must *not* exist from a end user video editing app
> perspective).

My point here about compositor is not that. Half of the story is the code complexity, the other part is a non-intuitive interface that you wouldn't expect from something called a "compositor".

We generally build elements that are simple and do one thing, and then you can combine them with others or use them multiple times even. And by doing that you can still get whatever blending behaviour you want without making the API weird.


For cross-fading multiple pads might make sense, thinking of something like a crossfader+concat for example but that wouldn't be useful for pitivi. I don't know what pitivi exactly needs for cross-fading but if there's a use-case for cross-fading the first two pads, and then the result and a third pad, then sure, go for it. But that would also be implementable by two 2-pad crossfade elements then and would perform exactly the same operations then (and be more or less the same performance-wise).

(In reply to Sebastian Dröge (slomo) from comment #28)
> (In reply to Thibault Saunier from comment #26)
> > (In reply to Sebastian Dröge (slomo) from comment #25)
> > > (In reply to Thibault Saunier from comment #23)
> > > 
> > > > I do not think we want this kind of restriction at all, the end user can
> > > > transition whatever he want, no reason to not let it do that!
> > > 
> > > Not having such restrictions means more complicated internals and it
> > > restricts how you can change the internals / extend the internals :) But
> > > that's for somewhere else to discuss I guess.
> > 
> > Well, this issue is exactly about that... you want to simplify the
> > internals, at the cost of flexibility and even removing useful features (2
> > pads only (which is arguably not a mandatory feature), and now adding
> > restriction which must *not* exist from a end user video editing app
> > perspective).
> 
> My point here about compositor is not that. Half of the story is the code
> complexity, the other part is a non-intuitive interface that you wouldn't
> expect from something called a "compositor".
>
> We generally build elements that are simple and do one thing, and then you
> can combine them with others or use them multiple times even. And by doing
> that you can still get whatever blending behaviour you want without making
> the API weird.

This is just a compositing mode to me, I am not sure what is weird about that API but I do believe it is very simple to use and generally useful to have.
 
> For cross-fading multiple pads might make sense, thinking of something like
> a crossfader+concat for example but that wouldn't be useful for pitivi. I
> don't know what pitivi exactly needs for cross-fading but if there's a
> use-case for cross-fading the first two pads, and then the result and a
> third pad, then sure, go for it. But that would also be implementable by two
> 2-pad crossfade elements then and would perform exactly the same operations
> then (and be more or less the same performance-wise).

It is not specifically Pitivi but GES, basically in there we use the compositor as a layer based compositor, transition being part of a compositing mode between two of them basically.

As a way forward here, I'd suggest that I implement my proposed changes in compositor so we have something to talk about, some time before the conference/hackfest. And port the compositor/crossfade example to use that compositor (two of them actually) to show that it can provide the same behaviour in a composable way by chaining compositors (you need two if the inputs are not opaque).

Created attachment 374069 [details] [review]
compositor: Implement different operators via per-pad property

This removes the crossfade-ratio property and replaces it with an
operator property. Currently this implements the following operators:

- SOURCE: Copy over the source and don't look at the destination
- OVER: Default blending of the source over the destination
- ADD: Like OVER but simply adding the alpha instead

See the example for how to implement crossfading with this.

Review of attachment 374069 [details] [review]:

::: tests/examples/compositor/crossfade.c
@@ +58,1 @@
+  g_object_set (sinkpad, "operator", info->is_last ? 2 : 0, NULL);

This works for 2 streams, need to implement the correct behaviour for more than 2 streams tomorrow

@@ +94,3 @@
       NULL);
 
+  gst_util_set_object_arg (G_OBJECT (compositor), "background", "checker");

Currently has checkers background to make sure that the background does not leak through, should be black again later :)

Created attachment 374079 [details] [review]
compositor: Implement different operators via per-pad property

This removes the crossfade-ratio property and replaces it with an
operator property. Currently this implements the following operators:

- SOURCE: Copy over the source and don't look at the destination
- OVER: Default blending of the source over the destination
- ADD: Like OVER but simply adding the alpha instead

See the example for how to implement crossfading with this.

Created attachment 374097 [details] [review]
video-transition: Port to the new 'operator' API in compositor

(In reply to Thibault Saunier from comment #34)
> Created attachment 374097 [details] [review] [review]
> video-transition: Port to the new 'operator' API in compositor

Ported the GES code to use the new API, seems to be working just fine.

Comment on attachment 374079 [details] [review]
compositor: Implement different operators via per-pad property

Attachment 374079 [details] pushed as aae25e0 - compositor: Implement different operators via per-pad property

Comment on attachment 374097 [details] [review]
video-transition: Port to the new 'operator' API in compositor

Please push :) It does not apply cleanly to my version of ges for some reason but looks correct.

Thanks!

Attachment 374097 [details] pushed as 3c7f488 - video-transition: Port to the new 'operator' API in compositor

Created attachment 374102 [details] [review]
fix undeclared functions

This change gave me compiler warnings because of undeclared functions in the orc code.
Here's a proposed patch which fixes that.

Review of attachment 374102 [details] [review]:

Thanks Johan!