[FFmpeg-devel] [PATCH 2/4] avfilter/avfiltergraph: fix has_alpha in pick_format

[wm4]

On Sun, 22 Apr 2018 13:44:19 +0200 (CEST)
Marton Balint <cus at passwd.hu> wrote:

> On Fri, 20 Apr 2018, Michael Niedermayer wrote:
> 
> > On Thu, Apr 19, 2018 at 09:32:19PM +0200, Marton Balint wrote:  
> >> A pixel format which has a palette also has alpha, without this patch the
> >> format negotiation might have preferred formats without alpha even if the
> >> source had alpha.
> >>
> >> Signed-off-by: Marton Balint <cus at passwd.hu>
> >> ---
> >>  libavfilter/avfiltergraph.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
> >> index 4cc6892404..e18f733e23 100644
> >> --- a/libavfilter/avfiltergraph.c
> >> +++ b/libavfilter/avfiltergraph.c
> >> @@ -679,7 +679,7 @@ static int pick_format(AVFilterLink *link, AVFilterLink *ref)
> >>
> >>      if (link->type == AVMEDIA_TYPE_VIDEO) {
> >>          if(ref && ref->type == AVMEDIA_TYPE_VIDEO){
> >> -            int has_alpha= av_pix_fmt_desc_get(ref->format)->nb_components % 2 == 0;
> >> +            int has_alpha= av_pix_fmt_desc_get(ref->format)->nb_components % 2 == 0 || (av_pix_fmt_desc_get(ref->format)->flags & AV_PIX_FMT_FLAG_PAL);  
> >
> > This causes various output files to grow in size with a unused alpha plane
> >
> > a random example: (there are likels better examples)
> > ./ffmpeg -y -i ~/tickets/1116/1023.bmp -vf negate fileX.bmp
> >
> > Iam not sure unconditionally treating all palettes as if they have
> > non fully opaque entries is a good idea.  
> 
> Obviously not, but it is already treated this way in most places. Having a 
> bigger image with alpha is better than losing alpha. And the user can 
> always force losing alpha with a filter, and sometimes he has to do that 
> anyway because for example fre0r filters have no way of signalling if they 
> use alpha or not...
> 
> >
> > Doesnt this patchset replace a problem by another problem ?
> > It may be better to not rush this and find a complete solution
> >
> > a 2nd pixfmt and or scaning the 256 entries for their alpha values
> > is maybe the direction we could go  
> 
> I don't think scaning can work for anything other than still images, as 
> the palette can change per-frame AFAIK. Introducing a new pixel format 
> seems the better approach, however keeping in mind compatibility and 
> existing code, I'd rather make the newly introduced one the one without 
> alpha, and move things gradually to it. Still seems like a fair amount of 
> work...

PAL8 has the same problem as RGBA formats: you can't know whether it's
going to use alpha or not. (Indeed, there are codecs which can update
the palette, and who says filters won't change the palette?) With PAL8
it's just faster to verify for a particular frame. But in general, you
have to know in advance, and have to signal it in advance. I think that
works relatively well with PIXFMT_RGBA vs. PIXFMT_RGB0.

So I would argue it's fine to change PAL8 to having alpha, and if we
find cases that would really benefit from it, add PIXFMT_PAL8_RGB0. (Or
_RGBX or _NO_ALPHA or whatever.)