diff mbox

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

Message ID 20180419193221.21712-3-cus@passwd.hu
State New
Headers show

Commit Message

Marton Balint April 19, 2018, 7:32 p.m. UTC 
Signed-off-by: Marton Balint <cus@passwd.hu>
---
 doc/APIchanges      |  3 +++
 libavutil/pixdesc.h | 11 +++++++++++
 libavutil/version.h |  2 +-
 3 files changed, 15 insertions(+), 1 deletion(-)

Comments

wm4 April 19, 2018, 7:39 p.m. UTC | #1 
On Thu, 19 Apr 2018 21:32:20 +0200
Marton Balint <cus@passwd.hu> wrote:

> Signed-off-by: Marton Balint <cus@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 also object to making it static inline, which not only can cause
problems in various cases, but also would make it part of the ABI (you
couldn't change the implementation without modifying the ABI,
essentially).

> +
>  #endif /* AVUTIL_PIXDESC_H */
> 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, \
Hendrik Leppkes April 19, 2018, 7:44 p.m. UTC | #2 
ex

On Thu, Apr 19, 2018 at 9:39 PM, wm4 <nfxjfg@googlemail.com> wrote:
> On Thu, 19 Apr 2018 21:32:20 +0200
> Marton Balint <cus@passwd.hu> wrote:
>
>> Signed-off-by: Marton Balint <cus@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?

(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)

- Hendrik
wm4 April 19, 2018, 7:56 p.m. UTC | #3 
On Thu, 19 Apr 2018 21:44:36 +0200
Hendrik Leppkes <h.leppkes@gmail.com> wrote:

>  ex
> 
> On Thu, Apr 19, 2018 at 9:39 PM, wm4 <nfxjfg@googlemail.com> wrote:
> > On Thu, 19 Apr 2018 21:32:20 +0200
> > Marton Balint <cus@passwd.hu> wrote:
> >  
> >> Signed-off-by: Marton Balint <cus@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.)
diff mbox

Patch

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);
+}
+
 #endif /* AVUTIL_PIXDESC_H */
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, \