| Message ID | 20180419212503.8203-1-cus@passwd.hu |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 19 Apr 2018 23:25:03 +0200 Marton Balint <cus@passwd.hu> wrote: > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > doc/APIchanges | 3 +++ > libavutil/pixdesc.c | 3 +-- > libavutil/pixdesc.h | 8 ++------ > libavutil/tests/pixdesc.c | 4 ---- > libavutil/version.h | 2 +- > 5 files changed, 7 insertions(+), 13 deletions(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index 4f6ac2a031..d9b457e080 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_FLAG_ALPHA to AV_PIX_FMT_PAL8. > + > -------- 8< --------- FFmpeg 4.0 was cut here -------- 8< --------- > > 2018-04-03 - d6fc031caf - lavu 56.13.100 - pixdesc.h > diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c > index 8ed52751c1..a8be7b66e6 100644 > --- a/libavutil/pixdesc.c > +++ b/libavutil/pixdesc.c > @@ -288,7 +288,7 @@ static const AVPixFmtDescriptor av_pix_fmt_descriptors[AV_PIX_FMT_NB] = { > .comp = { > { 0, 1, 0, 0, 8, 0, 7, 1 }, > }, > - .flags = AV_PIX_FMT_FLAG_PAL, > + .flags = AV_PIX_FMT_FLAG_PAL | AV_PIX_FMT_FLAG_ALPHA, > }, > [AV_PIX_FMT_YUVJ420P] = { > .name = "yuvj420p", > @@ -2432,7 +2432,6 @@ void ff_check_pixfmt_descriptors(void){ > av_assert0(d->log2_chroma_h <= 3); > av_assert0(d->nb_components <= 4); > av_assert0(d->name && d->name[0]); > - av_assert0((d->nb_components==4 || d->nb_components==2) == !!(d->flags & AV_PIX_FMT_FLAG_ALPHA)); > av_assert2(av_get_pix_fmt(d->name) == i); > > for (j=0; j<FF_ARRAY_ELEMS(d->comp); j++) { > diff --git a/libavutil/pixdesc.h b/libavutil/pixdesc.h > index 1ab372782a..4f9c5a271f 100644 > --- a/libavutil/pixdesc.h > +++ b/libavutil/pixdesc.h > @@ -167,12 +167,8 @@ typedef struct AVPixFmtDescriptor { > > /** > * The pixel format has an alpha channel. This is set on all formats that > - * support alpha in some way. The exception is AV_PIX_FMT_PAL8, which can > - * carry alpha as part of the palette. Details are explained in the > - * AVPixelFormat enum, and are also encoded in the corresponding > - * AVPixFmtDescriptor. > - * > - * The alpha is always straight, never pre-multiplied. > + * support alpha in some way, including AV_PIX_FMT_PAL8. The alpha is always > + * straight, never pre-multiplied. > * > * If a codec or a filter does not support alpha, it should set all alpha to > * opaque, or use the equivalent pixel formats without alpha component, e.g. > diff --git a/libavutil/tests/pixdesc.c b/libavutil/tests/pixdesc.c > index 7fbfeea96c..34e2bea932 100644 > --- a/libavutil/tests/pixdesc.c > +++ b/libavutil/tests/pixdesc.c > @@ -37,10 +37,6 @@ int main(void){ > skip = 0; > } > av_log(NULL, AV_LOG_INFO, "pix fmt %s avg_bpp:%d colortype:%d\n", desc->name, av_get_padded_bits_per_pixel(desc), get_color_type(desc)); > - if ((!(desc->flags & AV_PIX_FMT_FLAG_ALPHA)) != (desc->nb_components != 2 && desc->nb_components != 4)) { > - av_log(NULL, AV_LOG_ERROR, "Alpha flag mismatch\n"); > - err = 1; > - } > } > return err; > } > diff --git a/libavutil/version.h b/libavutil/version.h > index 387421775f..23567000a3 100644 > --- a/libavutil/version.h > +++ b/libavutil/version.h > @@ -79,7 +79,7 @@ > */ > > #define LIBAVUTIL_VERSION_MAJOR 56 > -#define LIBAVUTIL_VERSION_MINOR 15 > +#define LIBAVUTIL_VERSION_MINOR 16 > #define LIBAVUTIL_VERSION_MICRO 100 > > #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ Probably fine. While I like it, we also have to be careful about the consequences. Does it change FATE or the results of that pixfmt choosing function, avcodec_find_best_pix_fmt_of_list()? Are there any formats that decode to PAL8, but write garbage to the alpha component of the palette?
On Fri, 20 Apr 2018, wm4 wrote: > On Thu, 19 Apr 2018 23:25:03 +0200 > Marton Balint <cus@passwd.hu> wrote: > >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> doc/APIchanges | 3 +++ >> libavutil/pixdesc.c | 3 +-- >> libavutil/pixdesc.h | 8 ++------ >> libavutil/tests/pixdesc.c | 4 ---- >> libavutil/version.h | 2 +- >> 5 files changed, 7 insertions(+), 13 deletions(-) >> >> diff --git a/doc/APIchanges b/doc/APIchanges >> index 4f6ac2a031..d9b457e080 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_FLAG_ALPHA to AV_PIX_FMT_PAL8. >> + [..] > > Probably fine. While I like it, we also have to be careful about the > consequences. Does it change FATE or the results of that pixfmt choosing > function, avcodec_find_best_pix_fmt_of_list()? Fate passes. I am not sure about avcodec_find_best_pix_fmt_of_list(), but since pixdesc_has_alpha() in avutil/pixdesc.c already considered PAL8 as a format with alpha, I don't think it changes. > Are there any formats > that decode to PAL8, but write garbage to the alpha component of the > palette? If there are, then those should be fixed. The damage was already done when it was decided to consider PAL8 a format with alpha, so in some cases garbage was already used... Regards, Marton
On Sun, 22 Apr 2018 13:24:11 +0200 (CEST) Marton Balint <cus@passwd.hu> wrote: > On Fri, 20 Apr 2018, wm4 wrote: > > > On Thu, 19 Apr 2018 23:25:03 +0200 > > Marton Balint <cus@passwd.hu> wrote: > > > >> Signed-off-by: Marton Balint <cus@passwd.hu> > >> --- > >> doc/APIchanges | 3 +++ > >> libavutil/pixdesc.c | 3 +-- > >> libavutil/pixdesc.h | 8 ++------ > >> libavutil/tests/pixdesc.c | 4 ---- > >> libavutil/version.h | 2 +- > >> 5 files changed, 7 insertions(+), 13 deletions(-) > >> > >> diff --git a/doc/APIchanges b/doc/APIchanges > >> index 4f6ac2a031..d9b457e080 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_FLAG_ALPHA to AV_PIX_FMT_PAL8. > >> + > > [..] > > > > > Probably fine. While I like it, we also have to be careful about the > > consequences. Does it change FATE or the results of that pixfmt choosing > > function, avcodec_find_best_pix_fmt_of_list()? > > Fate passes. I am not sure about avcodec_find_best_pix_fmt_of_list(), but > since pixdesc_has_alpha() in avutil/pixdesc.c already considered PAL8 as a > format with alpha, I don't think it changes. Oh, interesting point. So this whole discussion is moot anyway, since it always suggested RGBA when converting PAL8 to a non-paletted RGB format? > > Are there any formats > > that decode to PAL8, but write garbage to the alpha component of the > > palette? > > If there are, then those should be fixed. The damage was already done when > it was decided to consider PAL8 a format with alpha, so in some cases > garbage was already used... Agreed.
On Sun, 22 Apr 2018, wm4 wrote: > On Sun, 22 Apr 2018 13:24:11 +0200 (CEST) > Marton Balint <cus@passwd.hu> wrote: > >> On Fri, 20 Apr 2018, wm4 wrote: >> >> > On Thu, 19 Apr 2018 23:25:03 +0200 >> > Marton Balint <cus@passwd.hu> wrote: >> > >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> >> --- >> >> doc/APIchanges | 3 +++ >> >> libavutil/pixdesc.c | 3 +-- >> >> libavutil/pixdesc.h | 8 ++------ >> >> libavutil/tests/pixdesc.c | 4 ---- >> >> libavutil/version.h | 2 +- >> >> 5 files changed, 7 insertions(+), 13 deletions(-) >> >> >> >> diff --git a/doc/APIchanges b/doc/APIchanges >> >> index 4f6ac2a031..d9b457e080 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_FLAG_ALPHA to AV_PIX_FMT_PAL8. >> >> + >> >> [..] >> >> > >> > Probably fine. While I like it, we also have to be careful about the >> > consequences. Does it change FATE or the results of that pixfmt choosing >> > function, avcodec_find_best_pix_fmt_of_list()? >> >> Fate passes. I am not sure about avcodec_find_best_pix_fmt_of_list(), but >> since pixdesc_has_alpha() in avutil/pixdesc.c already considered PAL8 as a >> format with alpha, I don't think it changes. > > Oh, interesting point. So this whole discussion is moot anyway, since > it always suggested RGBA when converting PAL8 to a non-paletted RGB > format? Only if has_alpha was true when the user called avcodec_find_best_pix_fmt_of_list() or av_find_best_pix_fmt_of_2(). For avfiltergraph format negotiation or for ffmpeg.c automatic codec pixel format selection, pal8 was not considered as a format with alpha (nb_components % 2 == 0 check was used), so they called av_find_best_pix_fmt_of_2() with has_alpha=false, therefore alpha loss was not penalized. A subsequent patch takes care of that. Regards, Marton
On Mon, 23 Apr 2018 20:25:40 +0200 (CEST) Marton Balint <cus@passwd.hu> wrote: > On Sun, 22 Apr 2018, wm4 wrote: > > > On Sun, 22 Apr 2018 13:24:11 +0200 (CEST) > > Marton Balint <cus@passwd.hu> wrote: > > > >> On Fri, 20 Apr 2018, wm4 wrote: > >> > >> > On Thu, 19 Apr 2018 23:25:03 +0200 > >> > Marton Balint <cus@passwd.hu> wrote: > >> > > >> >> Signed-off-by: Marton Balint <cus@passwd.hu> > >> >> --- > >> >> doc/APIchanges | 3 +++ > >> >> libavutil/pixdesc.c | 3 +-- > >> >> libavutil/pixdesc.h | 8 ++------ > >> >> libavutil/tests/pixdesc.c | 4 ---- > >> >> libavutil/version.h | 2 +- > >> >> 5 files changed, 7 insertions(+), 13 deletions(-) > >> >> > >> >> diff --git a/doc/APIchanges b/doc/APIchanges > >> >> index 4f6ac2a031..d9b457e080 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_FLAG_ALPHA to AV_PIX_FMT_PAL8. > >> >> + > >> > >> [..] > >> > >> > > >> > Probably fine. While I like it, we also have to be careful about the > >> > consequences. Does it change FATE or the results of that pixfmt choosing > >> > function, avcodec_find_best_pix_fmt_of_list()? > >> > >> Fate passes. I am not sure about avcodec_find_best_pix_fmt_of_list(), but > >> since pixdesc_has_alpha() in avutil/pixdesc.c already considered PAL8 as a > >> format with alpha, I don't think it changes. > > > > Oh, interesting point. So this whole discussion is moot anyway, since > > it always suggested RGBA when converting PAL8 to a non-paletted RGB > > format? > > Only if has_alpha was true when the user called > avcodec_find_best_pix_fmt_of_list() or av_find_best_pix_fmt_of_2(). > > For avfiltergraph format negotiation or for ffmpeg.c automatic codec pixel > format selection, pal8 was not considered as a format with alpha > (nb_components % 2 == 0 check was used), so they called > av_find_best_pix_fmt_of_2() with has_alpha=false, therefore alpha loss was > not penalized. A subsequent patch takes care of that. Right. That's a bit of a mess. So if anyone thinks that's a problem, the avfilter code can still be special-cased, or we could introduce a non-alpha PAL format. LGTM anyway, it's a nice cleanup.
diff --git a/doc/APIchanges b/doc/APIchanges index 4f6ac2a031..d9b457e080 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_FLAG_ALPHA to AV_PIX_FMT_PAL8. + -------- 8< --------- FFmpeg 4.0 was cut here -------- 8< --------- 2018-04-03 - d6fc031caf - lavu 56.13.100 - pixdesc.h diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c index 8ed52751c1..a8be7b66e6 100644 --- a/libavutil/pixdesc.c +++ b/libavutil/pixdesc.c @@ -288,7 +288,7 @@ static const AVPixFmtDescriptor av_pix_fmt_descriptors[AV_PIX_FMT_NB] = { .comp = { { 0, 1, 0, 0, 8, 0, 7, 1 }, }, - .flags = AV_PIX_FMT_FLAG_PAL, + .flags = AV_PIX_FMT_FLAG_PAL | AV_PIX_FMT_FLAG_ALPHA, }, [AV_PIX_FMT_YUVJ420P] = { .name = "yuvj420p", @@ -2432,7 +2432,6 @@ void ff_check_pixfmt_descriptors(void){ av_assert0(d->log2_chroma_h <= 3); av_assert0(d->nb_components <= 4); av_assert0(d->name && d->name[0]); - av_assert0((d->nb_components==4 || d->nb_components==2) == !!(d->flags & AV_PIX_FMT_FLAG_ALPHA)); av_assert2(av_get_pix_fmt(d->name) == i); for (j=0; j<FF_ARRAY_ELEMS(d->comp); j++) { diff --git a/libavutil/pixdesc.h b/libavutil/pixdesc.h index 1ab372782a..4f9c5a271f 100644 --- a/libavutil/pixdesc.h +++ b/libavutil/pixdesc.h @@ -167,12 +167,8 @@ typedef struct AVPixFmtDescriptor { /** * The pixel format has an alpha channel. This is set on all formats that - * support alpha in some way. The exception is AV_PIX_FMT_PAL8, which can - * carry alpha as part of the palette. Details are explained in the - * AVPixelFormat enum, and are also encoded in the corresponding - * AVPixFmtDescriptor. - * - * The alpha is always straight, never pre-multiplied. + * support alpha in some way, including AV_PIX_FMT_PAL8. The alpha is always + * straight, never pre-multiplied. * * If a codec or a filter does not support alpha, it should set all alpha to * opaque, or use the equivalent pixel formats without alpha component, e.g. diff --git a/libavutil/tests/pixdesc.c b/libavutil/tests/pixdesc.c index 7fbfeea96c..34e2bea932 100644 --- a/libavutil/tests/pixdesc.c +++ b/libavutil/tests/pixdesc.c @@ -37,10 +37,6 @@ int main(void){ skip = 0; } av_log(NULL, AV_LOG_INFO, "pix fmt %s avg_bpp:%d colortype:%d\n", desc->name, av_get_padded_bits_per_pixel(desc), get_color_type(desc)); - if ((!(desc->flags & AV_PIX_FMT_FLAG_ALPHA)) != (desc->nb_components != 2 && desc->nb_components != 4)) { - av_log(NULL, AV_LOG_ERROR, "Alpha flag mismatch\n"); - err = 1; - } } return err; } diff --git a/libavutil/version.h b/libavutil/version.h index 387421775f..23567000a3 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 56 -#define LIBAVUTIL_VERSION_MINOR 15 +#define LIBAVUTIL_VERSION_MINOR 16 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
Signed-off-by: Marton Balint <cus@passwd.hu> --- doc/APIchanges | 3 +++ libavutil/pixdesc.c | 3 +-- libavutil/pixdesc.h | 8 ++------ libavutil/tests/pixdesc.c | 4 ---- libavutil/version.h | 2 +- 5 files changed, 7 insertions(+), 13 deletions(-)