[FFmpeg-devel,4/5] avutil/pixdesc: add AV_PIX_FMT_FLAG_ALPHA to AV_PIX_FMT_PAL8

Submitted by Marton Balint on April 24, 2018, 7:05 p.m.

Details

Message ID 20180424190501.23908-4-cus@passwd.hu
State New
Headers show

Commit Message

Marton Balint April 24, 2018, 7:05 p.m.
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(-)

Comments

Michael Niedermayer April 25, 2018, 5:44 p.m.
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

[...]
Marton Balint April 30, 2018, 12:42 p.m.
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
Carl Eugen Hoyos April 30, 2018, 6:04 p.m.
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
Marton Balint April 30, 2018, 8:44 p.m.
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

Patch hide | download patch | download mbox

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, \