diff mbox series

[FFmpeg-devel] avfilter/vf_scale: set RGB to always be full range

Message ID 20200915211643.259961-1-jeebjp@gmail.com
State New
Headers show
Series [FFmpeg-devel] avfilter/vf_scale: set RGB to always be full range | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jan Ekström Sept. 15, 2020, 9:16 p.m. UTC 
This value - while it looks like the actual range of the content -
is nothing but the internal value of swscale.

Thus, if we have RGB content, force the value to 1. Swscale will
ignore it, but at least the value of the output AVFrame will now
properly be "full range" instead of "limited range", as it is right
now.
---
 libavfilter/vf_scale.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Nicolas George Sept. 16, 2020, 1:57 p.m. UTC | #1 
Jan Ekström (12020-09-16):
> This value - while it looks like the actual range of the content -
> is nothing but the internal value of swscale.
> 
> Thus, if we have RGB content, force the value to 1. Swscale will
> ignore it, but at least the value of the output AVFrame will now
> properly be "full range" instead of "limited range", as it is right
> now.
> ---
>  libavfilter/vf_scale.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> index 58eee96744..12df27edf4 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
> @@ -751,6 +751,15 @@ scale:
>          || scale->out_range != AVCOL_RANGE_UNSPECIFIED) {
>          int in_full, out_full, brightness, contrast, saturation;
>          const int *inv_table, *table;

> +        const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(out->format);
> +        if (!out_desc) {
> +            av_log(ctx, AV_LOG_ERROR,
> +                   "Failed to get the pixel format descriptor for format %d!\n",
> +                   out->format);
> +            av_frame_free(&in);
> +            av_frame_free(frame_out);
> +            return AVERROR_INVALIDDATA;
> +        }

Since the format is negotiated, I think it can be considered a severe
internal bug that out_desc is not found, i.e. checked by an assert.

>  
>          sws_getColorspaceDetails(scale->sws, (int **)&inv_table, &in_full,
>                                   (int **)&table, &out_full,
> @@ -768,7 +777,15 @@ scale:
>          else if (in_range != AVCOL_RANGE_UNSPECIFIED)
>              in_full  = (in_range == AVCOL_RANGE_JPEG);
>          if (scale->out_range != AVCOL_RANGE_UNSPECIFIED)
> +            // note: this can be silently overridden by
> +            //       sws_setColorspaceDetails for non-YCbCr formats
>              out_full = (scale->out_range == AVCOL_RANGE_JPEG);
> +        else if (out_desc->flags & AV_PIX_FMT_FLAG_RGB)
> +            // the range values received from swscale are its internal
> +            // identifiers, and will not be nonzero for any sort of RGB.
> +            // thus, if we do not set it to nonzero ourselves, this will
> +            // be incorrect.
> +            out_full = 1;
>  
>          sws_setColorspaceDetails(scale->sws, inv_table, in_full,
>                                   table, out_full,

With you explanation, I think it is ok, but I do not know the topic
enough to be sure.

Regards,
Michael Niedermayer Sept. 16, 2020, 8:38 p.m. UTC | #2 
On Wed, Sep 16, 2020 at 12:16:43AM +0300, Jan Ekström wrote:
> This value - while it looks like the actual range of the content -
> is nothing but the internal value of swscale.
> 
> Thus, if we have RGB content, force the value to 1. Swscale will
> ignore it, but at least the value of the output AVFrame will now
> properly be "full range" instead of "limited range", as it is right
> now.
> ---
>  libavfilter/vf_scale.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> index 58eee96744..12df27edf4 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
[...]
> @@ -768,7 +777,15 @@ scale:
>          else if (in_range != AVCOL_RANGE_UNSPECIFIED)
>              in_full  = (in_range == AVCOL_RANGE_JPEG);
>          if (scale->out_range != AVCOL_RANGE_UNSPECIFIED)
> +            // note: this can be silently overridden by
> +            //       sws_setColorspaceDetails for non-YCbCr formats
>              out_full = (scale->out_range == AVCOL_RANGE_JPEG);
> +        else if (out_desc->flags & AV_PIX_FMT_FLAG_RGB)
> +            // the range values received from swscale are its internal
> +            // identifiers, and will not be nonzero for any sort of RGB.
> +            // thus, if we do not set it to nonzero ourselves, this will
> +            // be incorrect.
> +            out_full = 1;

Does anything document the meaning of range or AVCOL_RANGE for RGB ?
The enum is documented as "MPEG vs JPEG YUV range."

What is limited range RGB ? what would the ranges of R,G,B be and where
is that specified ?

That said, changing it to full range for RGB as done by this patch is
probably ok if it helps some code (for example allows simplifications)

thx

[...]
Jan Ekström Sept. 16, 2020, 8:43 p.m. UTC | #3 
On Wed, Sep 16, 2020 at 11:38 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Wed, Sep 16, 2020 at 12:16:43AM +0300, Jan Ekström wrote:
> > This value - while it looks like the actual range of the content -
> > is nothing but the internal value of swscale.
> >
> > Thus, if we have RGB content, force the value to 1. Swscale will
> > ignore it, but at least the value of the output AVFrame will now
> > properly be "full range" instead of "limited range", as it is right
> > now.
> > ---
> >  libavfilter/vf_scale.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> > index 58eee96744..12df27edf4 100644
> > --- a/libavfilter/vf_scale.c
> > +++ b/libavfilter/vf_scale.c
> [...]
> > @@ -768,7 +777,15 @@ scale:
> >          else if (in_range != AVCOL_RANGE_UNSPECIFIED)
> >              in_full  = (in_range == AVCOL_RANGE_JPEG);
> >          if (scale->out_range != AVCOL_RANGE_UNSPECIFIED)
> > +            // note: this can be silently overridden by
> > +            //       sws_setColorspaceDetails for non-YCbCr formats
> >              out_full = (scale->out_range == AVCOL_RANGE_JPEG);
> > +        else if (out_desc->flags & AV_PIX_FMT_FLAG_RGB)
> > +            // the range values received from swscale are its internal
> > +            // identifiers, and will not be nonzero for any sort of RGB.
> > +            // thus, if we do not set it to nonzero ourselves, this will
> > +            // be incorrect.
> > +            out_full = 1;
>
> Does anything document the meaning of range or AVCOL_RANGE for RGB ?
> The enum is documented as "MPEG vs JPEG YUV range."
>
> What is limited range RGB ? what would the ranges of R,G,B be and where
> is that specified ?
>
> That said, changing it to full range for RGB as done by this patch is
> probably ok if it helps some code (for example allows simplifications)
>

I am not here to say that we would have to support limited range RGB,
which I mostly only see mentioned as the PC graphics output for
devices such as video projectors.

The problem is just that vf_scale took that internal swscale flag and
attempted to interpret that as full|limited range flag as-is.

Anyways, posted a v2 which actually verifies the requested flags
against the actually configured ones, while keeping the translation
between swscale internal value and "is this stuff going to be full
range or not".

Possibly simpler to follow/read than this one :) , even though clearly
more complex.

Jan
Kevin Wheatley Sept. 17, 2020, 10:10 a.m. UTC | #4 
On Wed, Sep 16, 2020 at 9:43 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Wed, Sep 16, 2020 at 11:38 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> > Does anything document the meaning of range or AVCOL_RANGE for RGB ?
> > The enum is documented as "MPEG vs JPEG YUV range."
> >
> > What is limited range RGB ? what would the ranges of R,G,B be and where
> > is that specified ?

EBU only describes full range for 'file' based images

https://tech.ebu.ch/docs/r/r103.pdf

but ITU https://www.itu.int/dms_pubrec/itu-r/rec/bt/R-REC-BT.2100-2-201807-I!!PDF-E.pdf

does describe narrow range RGB on p9

Kevin
Michael Niedermayer Sept. 17, 2020, 8:21 p.m. UTC | #5 
On Thu, Sep 17, 2020 at 11:10:29AM +0100, Kevin Wheatley wrote:
> On Wed, Sep 16, 2020 at 9:43 PM Jan Ekström <jeebjp@gmail.com> wrote:
> >
> > On Wed, Sep 16, 2020 at 11:38 PM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > > Does anything document the meaning of range or AVCOL_RANGE for RGB ?
> > > The enum is documented as "MPEG vs JPEG YUV range."
> > >
> > > What is limited range RGB ? what would the ranges of R,G,B be and where
> > > is that specified ?
> 
> EBU only describes full range for 'file' based images
> 
> https://tech.ebu.ch/docs/r/r103.pdf
> 
> but ITU https://www.itu.int/dms_pubrec/itu-r/rec/bt/R-REC-BT.2100-2-201807-I!!PDF-E.pdf

thanks for the links!

[...]
diff mbox series

Patch

diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index 58eee96744..12df27edf4 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -751,6 +751,15 @@  scale:
         || scale->out_range != AVCOL_RANGE_UNSPECIFIED) {
         int in_full, out_full, brightness, contrast, saturation;
         const int *inv_table, *table;
+        const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(out->format);
+        if (!out_desc) {
+            av_log(ctx, AV_LOG_ERROR,
+                   "Failed to get the pixel format descriptor for format %d!\n",
+                   out->format);
+            av_frame_free(&in);
+            av_frame_free(frame_out);
+            return AVERROR_INVALIDDATA;
+        }
 
         sws_getColorspaceDetails(scale->sws, (int **)&inv_table, &in_full,
                                  (int **)&table, &out_full,
@@ -768,7 +777,15 @@  scale:
         else if (in_range != AVCOL_RANGE_UNSPECIFIED)
             in_full  = (in_range == AVCOL_RANGE_JPEG);
         if (scale->out_range != AVCOL_RANGE_UNSPECIFIED)
+            // note: this can be silently overridden by
+            //       sws_setColorspaceDetails for non-YCbCr formats
             out_full = (scale->out_range == AVCOL_RANGE_JPEG);
+        else if (out_desc->flags & AV_PIX_FMT_FLAG_RGB)
+            // the range values received from swscale are its internal
+            // identifiers, and will not be nonzero for any sort of RGB.
+            // thus, if we do not set it to nonzero ourselves, this will
+            // be incorrect.
+            out_full = 1;
 
         sws_setColorspaceDetails(scale->sws, inv_table, in_full,
                                  table, out_full,