Re: [FFmpeg-devel] The patch series about premultiplied alpha

[Nicolas George <george@nsup.org>]

Thanks for this reply. I suggest we suspend our involvement of the TC
and threat to push as is and resume discussion. Let me stop assuming you
want to push post haste even if it is sloppy, let you stop assuming that
I want to block everything because I am a contrarian or whatever, and
let us both assume we both want to make this patch series as good as
possible.

If you agree, then just leave out Cc:tc when replying.

Niklas Haas (HE12025-08-03):
> I don't think this is the main source of disagreement, so I will be brief.
> In a nutshell, I think that it's not generally FFmpeg's job to go beyond a
> working explanation of image processing concepts that are already well-
> established in the industry. We also don't document in detail how and why
> chroma subsampling is used, or how HDR metadata is interpreted, etc.
> 
> It's easy enough for any competent engineer to find out this information,
> from sources which will surely outlive the FFmpeg project, e.g.:
> https://en.wikipedia.org/wiki/Alpha_compositing#Straight_versus_premultiplied
> 
> That said, I don't think this is a relevant point of discussion, so as a
> compromise I suggest to just expand the doxygen comments above the newly
> added `enum AVAlphaMode` to elaborate at least on what the practical
> difference is and why you would choose one over the other.

I think one or two sentences and that link would be useful for people
who do not know it but get bitten by it.

But more importantly, one or two paragraphs are needed to explain the
specifics of how FFmpeg chose to implement support for it, with in
particular links to the documentation of the various tools at the
disposal of the user to fix or work around errors it might produce: -vf
premultiply, -vf unpremultiply, -vf setparams need to be referenced all
in one place.

> I argue that a simple flag can not possibly solve this problem in any
> meaningful way. The nuance here is that the problem is not one of only
> supporting premul vs only supporting straight alpha, but rather the fact that
> vf_vstack (e.g.) wants its two inputs to merely *agree* on their alpha type.

You could have explained that in the first place. (Granted, I also could
have taken more words to express my recommendations. Let us write
precisely and let us read each other carefully.)

I think it is inaccurate to say that the flag I propose does not solve
the problem, it is more accurate to say that it solves the problem
inefficiently.

If we add the flag like I suggest, we obviously do not set it on the
stack filters. That means they will only accept straight frames and
produce valid output. For me, that means the problem is solved.

But it has the side effect of also rejecting the case where all frames
are premultiplied, a case that would have produced valid output.

I think we can live with that, especially since:

- Users can work around it by adding unpremultiply in front and
  premultiply after: inefficient but it works; of if they know what they
  are doing, setparam.

- Later, if the solution above is deemed too inconvenient, somebody can
  add the ≤5 lines of code in vf_stack.c to check that all frames match.


**In short**, the flag system I propose really kills all situations
where mismatched expectations would cause garbled output, at the cost of
also killing a few valid cases, but we have the means to work around
that issue and we can fix them on a case-by-case basis.


> Furthermore, Nicolas has the situation reversed: it is not the case that some
> filters "support premultiplied input", and the rest implicitly do not. Rather,
> *all* filters support both alpha modes, with basically no current exceptions
> except for a few obscure ones. (vf_overlay_qsv and vf_overlay_cuda in particular
> come to mind, though I suppose vf_fade also needs to be updated to support
> fading premultiplied input frames - that's a trivial fix and I will add it
> to my PR in a bit)

I think you underestimate the number of filters that would cause invalid
output when subjected to premultiplied frames.

Any filter that manipulates the contents of the frame beyond just
copying pixel values should be suspected of causing invalid output. In
particular, every filter listed in `git grep drawutils` should be
suspected: 44 files, including important ones (drawtext, pad,
subtitles) (you mentioned fade: it is on the list). Use of framesync is
also a clue that it might not work correctly, that raises the count to
79 files.

On the other hand, this command can produce a list of filters that will
probably not cause any issue because they do not access frame data and
are candidate to add the flag:

diff -U0 <(ls vf_*.c) <(git grep -lP 'drawutils|framesync|->data' -- 'vf_*.c') | grep '^-'

By the way, I am willing to put aside my reluctance of contributing with
all the bullshit going on in order to help implementing this flag
system. But I suspect it is so simple to implement that it is not worth
it.


Niklas Haas (HE12025-08-03):
> By the way, just to be clear:
> 
> As I am a member of the TC, I believe there to be a conflict of interest
> and will recuse myself from any internal discussions on the matter.

Thanks for your integrity. Not everybody had it.

> P.s. Out of curiosity, what happens if the remaining TC were to vote 2-2 on
> the issue? Would my vote be taken as an implicit tiebreaker?

I say: the leader decides.

Regards,

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".