| Message ID | 20180424190501.23908-4-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 24, 2018 at 09:05:00PM +0200, Marton Balint wrote: > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > doc/APIchanges | 3 +++ > libavcodec/tests/imgconvert.c | 4 ---- > libavutil/pixdesc.c | 3 +-- > libavutil/pixdesc.h | 8 ++------ > libavutil/tests/pixdesc.c | 4 ---- > libavutil/version.h | 2 +- > 6 files changed, 7 insertions(+), 17 deletions(-) this with the rest of the patchset seems not to break anything so no objections from me i was wondering though if when a 2nd PAL8 is introduced which will be with alpha PAL8 and PAL8A seemed a natural choice name wise iam mentioning this as if these 2 would be used then the addition of alpha to PAL8 would have to be undone. so it would make sense to first decide if the new format will be with or without alpha thx [...]
On Wed, 25 Apr 2018, Michael Niedermayer wrote: > On Tue, Apr 24, 2018 at 09:05:00PM +0200, Marton Balint wrote: >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> doc/APIchanges | 3 +++ >> libavcodec/tests/imgconvert.c | 4 ---- >> libavutil/pixdesc.c | 3 +-- >> libavutil/pixdesc.h | 8 ++------ >> libavutil/tests/pixdesc.c | 4 ---- >> libavutil/version.h | 2 +- >> 6 files changed, 7 insertions(+), 17 deletions(-) > > this with the rest of the patchset seems not to break anything > so no objections from me Thanks, will apply soon. > > i was wondering though if when a 2nd PAL8 is introduced which will > be with alpha > > PAL8 and PAL8A seemed a natural choice name wise > iam mentioning this as if these 2 would be used then the addition of alpha > to PAL8 would have to be undone. > so it would make sense to first decide if the new format will be with or > without alpha Keeping in mind compatibility, I think it is better if the new format gets to be the one without alpha (PAL8_NO_ALPHA or whatever), even if that not fits fully in the existing naming convention. Regards, Marton
2018-04-30 14:42 GMT+02:00, Marton Balint <cus@passwd.hu>: > > On Wed, 25 Apr 2018, Michael Niedermayer wrote: > >> On Tue, Apr 24, 2018 at 09:05:00PM +0200, Marton Balint wrote: >>> Signed-off-by: Marton Balint <cus@passwd.hu> >>> --- >>> doc/APIchanges | 3 +++ >>> libavcodec/tests/imgconvert.c | 4 ---- >>> libavutil/pixdesc.c | 3 +-- >>> libavutil/pixdesc.h | 8 ++------ >>> libavutil/tests/pixdesc.c | 4 ---- >>> libavutil/version.h | 2 +- >>> 6 files changed, 7 insertions(+), 17 deletions(-) >> >> this with the rest of the patchset seems not to break anything >> so no objections from me > > Thanks, will apply soon. Please do. >> i was wondering though if when a 2nd PAL8 is introduced which will >> be with alpha >> >> PAL8 and PAL8A seemed a natural choice name wise >> iam mentioning this as if these 2 would be used then the addition of alpha >> to PAL8 would have to be undone. >> so it would make sense to first decide if the new format will be with or >> without alpha > > Keeping in mind compatibility, I think it is better if the new format gets > to be the one without alpha (PAL8_NO_ALPHA or whatever), even if that not > fits fully in the existing naming convention. I wanted to agree but applications have to learn about the new format in any case since with your suggestion, we would have to change most pal decoders. Carl Eugen
On Mon, 30 Apr 2018, Carl Eugen Hoyos wrote: > 2018-04-30 14:42 GMT+02:00, Marton Balint <cus@passwd.hu>: >> >> On Wed, 25 Apr 2018, Michael Niedermayer wrote: >> >>> On Tue, Apr 24, 2018 at 09:05:00PM +0200, Marton Balint wrote: >>>> Signed-off-by: Marton Balint <cus@passwd.hu> >>>> --- >>>> doc/APIchanges | 3 +++ >>>> libavcodec/tests/imgconvert.c | 4 ---- >>>> libavutil/pixdesc.c | 3 +-- >>>> libavutil/pixdesc.h | 8 ++------ >>>> libavutil/tests/pixdesc.c | 4 ---- >>>> libavutil/version.h | 2 +- >>>> 6 files changed, 7 insertions(+), 17 deletions(-) >>> >>> this with the rest of the patchset seems not to break anything >>> so no objections from me >> >> Thanks, will apply soon. > > Please do. Applied the series. > >>> i was wondering though if when a 2nd PAL8 is introduced which will >>> be with alpha >>> >>> PAL8 and PAL8A seemed a natural choice name wise >>> iam mentioning this as if these 2 would be used then the addition of alpha >>> to PAL8 would have to be undone. >>> so it would make sense to first decide if the new format will be with or >>> without alpha >> >> Keeping in mind compatibility, I think it is better if the new format gets >> to be the one without alpha (PAL8_NO_ALPHA or whatever), even if that not >> fits fully in the existing naming convention. > > I wanted to agree but applications have to learn about the new > format in any case since with your suggestion, we would have > to change most pal decoders. Yeah, changing the pixel format of a codec will always be a suprise for API users unless they are using a filter chain or swscale to get a fixed pixel format in which case the change should be transparent enough. Regards, Marton
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/libavcodec/tests/imgconvert.c b/libavcodec/tests/imgconvert.c index c598d461d3..aefc324bf5 100644 --- a/libavcodec/tests/imgconvert.c +++ b/libavcodec/tests/imgconvert.c @@ -39,10 +39,6 @@ int main(void){ skip = 0; } av_log(NULL, AV_LOG_INFO, "pix fmt %s yuv_plan:%d avg_bpp:%d\n", desc->name, is_yuv_planar(desc), av_get_padded_bits_per_pixel(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/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 +++ libavcodec/tests/imgconvert.c | 4 ---- libavutil/pixdesc.c | 3 +-- libavutil/pixdesc.h | 8 ++------ libavutil/tests/pixdesc.c | 4 ---- libavutil/version.h | 2 +- 6 files changed, 7 insertions(+), 17 deletions(-)