[FFmpeg-devel] [PATCH 3/4] avutil/pixdesc: add av_pix_fmt_desc_has_alpha()

[wm4]

On Thu, 19 Apr 2018 21:44:36 +0200
Hendrik Leppkes <h.leppkes at gmail.com> wrote:

>  ex
> 
> On Thu, Apr 19, 2018 at 9:39 PM, wm4 <nfxjfg at googlemail.com> wrote:
> > On Thu, 19 Apr 2018 21:32:20 +0200
> > Marton Balint <cus at passwd.hu> wrote:
> >  
> >> Signed-off-by: Marton Balint <cus at passwd.hu>
> >> ---
> >>  doc/APIchanges      |  3 +++
> >>  libavutil/pixdesc.h | 11 +++++++++++
> >>  libavutil/version.h |  2 +-
> >>  3 files changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/doc/APIchanges b/doc/APIchanges
> >> index 4f6ac2a031..2a0b6f057a 100644
> >> --- a/doc/APIchanges
> >> +++ b/doc/APIchanges
> >> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> >>
> >>  API changes, most recent first:
> >>
> >> +2018-04-xx - xxxxxxxxxx - lavu 56.16.100 - pixdesc.h
> >> +  Add av_pix_fmt_desc_has_alpha().
> >> +
> >>  -------- 8< --------- FFmpeg 4.0 was cut here -------- 8< ---------
> >>
> >>  2018-04-03 - d6fc031caf - lavu 56.13.100 - pixdesc.h
> >> diff --git a/libavutil/pixdesc.h b/libavutil/pixdesc.h
> >> index 1ab372782a..aef4313ccb 100644
> >> --- a/libavutil/pixdesc.h
> >> +++ b/libavutil/pixdesc.h
> >> @@ -430,4 +430,15 @@ int av_get_pix_fmt_loss(enum AVPixelFormat dst_pix_fmt,
> >>  enum AVPixelFormat av_find_best_pix_fmt_of_2(enum AVPixelFormat dst_pix_fmt1, enum AVPixelFormat dst_pix_fmt2,
> >>                                               enum AVPixelFormat src_pix_fmt, int has_alpha, int *loss_ptr);
> >>
> >> +/**
> >> + * Return true if a pixel format descriptor has alpha channel.
> >> + *
> >> + * @param desc the pixel format descriptor
> >> + * @return 1 if the pixel format descriptor has alpha, 0 otherwise.
> >> + */
> >> +static inline int av_pix_fmt_desc_has_alpha(const AVPixFmtDescriptor *desc)
> >> +{
> >> +    return desc->nb_components == 2 || desc->nb_components == 4 || (desc->flags & AV_PIX_FMT_FLAG_PAL);
> >> +}  
> >
> > Good, idea, but...
> >
> > That doesn't seem very correct. Use AV_PIX_FMT_FLAG_ALPHA? (Although
> > PAL8 doesn't have it set, which is probably a bug. Or should have have
> > a separate PALA8 format?)
> >  
> 
> I agree, we should be using the flag we already have - and at that
> point, we probably also don't need a function to check it?

Yeah, except the weird PAL8 case.

> (The comment above AV_PIX_FMT_FLAG_ALPHA tries to explain the PAL8
> case - basically it is unknown if the palette contains alpha or not?
> or whatever)

It makes no sense to me. We had ambiguous formats before, like
AV_PIX_FMT_RGBA. The last component could be either alpha or padding.
Then AV_PIX_FMT_RGB0 (and friends) were introduced, which distinguish
formats with alpha from formats with padding. But before they were
introduced, the RGBA formats were flagged with AV_PIX_FMT_FLAG_ALPHA.

So the logical curse of action would be either marking PAL8 as
having alpha, or introducing a PALA8 format.

(Personally I'd say it would have been better to describe alpha
behavior with a separate enum. Then we could have distinguished
premultiplied and straight alpha, and could have kept the number of
pixfmts a but lower.)