[FFmpeg-devel,v2] avfilter/vf_scale: translate and verify swscale internal range flag

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.

Additionally, after calling sws_setColorspaceDetails double-check
the configured internal flag for the color range. Warn if this is
different to the requested value.

Finally, utilize the translated configured output value into the
output AVFrame's color_range.
---
 libavfilter/vf_scale.c | 51 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

On Wed, Sep 16, 2020 at 11:18:48PM +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.
> 
> Additionally, after calling sws_setColorspaceDetails double-check
> the configured internal flag for the color range. Warn if this is
> different to the requested value.
> 
> Finally, utilize the translated configured output value into the
> output AVFrame's color_range.
> ---
>  libavfilter/vf_scale.c | 51 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> index 58eee96744..592e4a344e 100644
> --- a/libavfilter/vf_scale.c
> +++ b/libavfilter/vf_scale.c
> @@ -750,11 +750,30 @@ scale:
>          || in_range != AVCOL_RANGE_UNSPECIFIED
>          || scale->out_range != AVCOL_RANGE_UNSPECIFIED) {
>          int in_full, out_full, brightness, contrast, saturation;
> +        int configured_in_full_range_flag, configured_out_full_range_flag;
>          const int *inv_table, *table;
> +        const AVPixFmtDescriptor *in_desc  = av_pix_fmt_desc_get(in->format);
> +        const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(out->format);

> +        if (!in_desc || !out_desc) {
> +            av_log(ctx, AV_LOG_ERROR,
> +                   "Failed to get one or more of the pixel format descriptors "
> +                   "for formats - in: %d (%s), out: %d (%s)!\n",
> +                   in->format, in_desc ? "OK" : "bad",
> +                   out->format, out_desc ? "OK": "bad");
> +            av_frame_free(&in);
> +            av_frame_free(frame_out);
> +            return AVERROR_INVALIDDATA;
> +        }

can this be true ?


>  
>          sws_getColorspaceDetails(scale->sws, (int **)&inv_table, &in_full,
>                                   (int **)&table, &out_full,
>                                   &brightness, &contrast, &saturation);

> +        // translate the swscale internal range flags to hold true
> +        // for RGB

The range field of AVFrame is documented as
"MPEG vs JPEG YUV range."

so it is not defined for RGB and cannot be "wrong" for RGB
maybe iam nitpicking here but if you want to use the range for RGB
the API docs need to be changed first.

<super nitpick>I mean you could set it to 1 for RGB thats ok without a API
docs change. But writing in a comment that one way is correct for RGB is not
compatible with the current documentation</super nitpick>

thx

[...]

On Thu, Sep 17, 2020 at 11:31 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Wed, Sep 16, 2020 at 11:18:48PM +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.
> >
> > Additionally, after calling sws_setColorspaceDetails double-check
> > the configured internal flag for the color range. Warn if this is
> > different to the requested value.
> >
> > Finally, utilize the translated configured output value into the
> > output AVFrame's color_range.
> > ---
> >  libavfilter/vf_scale.c | 51 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> > index 58eee96744..592e4a344e 100644
> > --- a/libavfilter/vf_scale.c
> > +++ b/libavfilter/vf_scale.c
> > @@ -750,11 +750,30 @@ scale:
> >          || in_range != AVCOL_RANGE_UNSPECIFIED
> >          || scale->out_range != AVCOL_RANGE_UNSPECIFIED) {
> >          int in_full, out_full, brightness, contrast, saturation;
> > +        int configured_in_full_range_flag, configured_out_full_range_flag;
> >          const int *inv_table, *table;
> > +        const AVPixFmtDescriptor *in_desc  = av_pix_fmt_desc_get(in->format);
> > +        const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(out->format);
>
> > +        if (!in_desc || !out_desc) {
> > +            av_log(ctx, AV_LOG_ERROR,
> > +                   "Failed to get one or more of the pixel format descriptors "
> > +                   "for formats - in: %d (%s), out: %d (%s)!\n",
> > +                   in->format, in_desc ? "OK" : "bad",
> > +                   out->format, out_desc ? "OK": "bad");
> > +            av_frame_free(&in);
> > +            av_frame_free(frame_out);
> > +            return AVERROR_INVALIDDATA;
> > +        }
>
> can this be true ?
>

Maybe it cannot, but it's a pointer. If the value somehow ends up
being one for which there is no descriptor.

It all boils to, unless I know there cannot technically be a nullptr
received, I check. Because this way the check is in one place, instead
of doing `xx_desc ? xx_desc->thing : failover` everywhere.

I can change this to an assert as mentioned by Nicolas, of course.

>
> >
> >          sws_getColorspaceDetails(scale->sws, (int **)&inv_table, &in_full,
> >                                   (int **)&table, &out_full,
> >                                   &brightness, &contrast, &saturation);
>
> > +        // translate the swscale internal range flags to hold true
> > +        // for RGB
>
> The range field of AVFrame is documented as
> "MPEG vs JPEG YUV range."
>

Yes, but I think quite a few people at this point just read it "full
range" VS "limited range" without any connotations towards YCbCr
specifically. Personally, I consider the enumeration names as just old
left-overs from the time when the MPEG/JPEG or TV/PC naming sounded
like a good idea and that nobody wanted to start the change of adding
new defines and enum values etc, doing deprecation etc as that is
quite a lot of work just to get the naming of enums nicer. Case in
point - for example pngdec sets the range value for the RGB frames it
returns.

Of course unless you specifically want to support limited range RGB,
RGB should not only be implied being full range by default but also
always set to be full range everywhere.

> so it is not defined for RGB and cannot be "wrong" for RGB
> maybe iam nitpicking here but if you want to use the range for RGB
> the API docs need to be changed first.
>

The code was ALREADY doing that, and I'm just converting the value so
that you don't end up with RGB + limited range AVFrames?!

> <super nitpick>I mean you could set it to 1 for RGB thats ok without a API
> docs change. But writing in a comment that one way is correct for RGB is not
> compatible with the current documentation</super nitpick>
>

So you want another patch that changes it to just say "limited/narrow
range" and "full range"? Like seriously, I just wanted to fix a
problem that got found because I finally started plugging some pipes
together!

Jan

On Thu, Sep 17, 2020 at 11:44:39PM +0300, Jan Ekström wrote:
> On Thu, Sep 17, 2020 at 11:31 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Wed, Sep 16, 2020 at 11:18:48PM +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.
> > >
> > > Additionally, after calling sws_setColorspaceDetails double-check
> > > the configured internal flag for the color range. Warn if this is
> > > different to the requested value.
> > >
> > > Finally, utilize the translated configured output value into the
> > > output AVFrame's color_range.
> > > ---
> > >  libavfilter/vf_scale.c | 51 +++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 50 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> > > index 58eee96744..592e4a344e 100644
> > > --- a/libavfilter/vf_scale.c
> > > +++ b/libavfilter/vf_scale.c
> > > @@ -750,11 +750,30 @@ scale:
> > >          || in_range != AVCOL_RANGE_UNSPECIFIED
> > >          || scale->out_range != AVCOL_RANGE_UNSPECIFIED) {
> > >          int in_full, out_full, brightness, contrast, saturation;
> > > +        int configured_in_full_range_flag, configured_out_full_range_flag;
> > >          const int *inv_table, *table;
> > > +        const AVPixFmtDescriptor *in_desc  = av_pix_fmt_desc_get(in->format);
> > > +        const AVPixFmtDescriptor *out_desc = av_pix_fmt_desc_get(out->format);
> >
> > > +        if (!in_desc || !out_desc) {
> > > +            av_log(ctx, AV_LOG_ERROR,
> > > +                   "Failed to get one or more of the pixel format descriptors "
> > > +                   "for formats - in: %d (%s), out: %d (%s)!\n",
> > > +                   in->format, in_desc ? "OK" : "bad",
> > > +                   out->format, out_desc ? "OK": "bad");
> > > +            av_frame_free(&in);
> > > +            av_frame_free(frame_out);
> > > +            return AVERROR_INVALIDDATA;
> > > +        }
> >
> > can this be true ?
> >
> 
> Maybe it cannot, but it's a pointer. If the value somehow ends up
> being one for which there is no descriptor.
> 
> It all boils to, unless I know there cannot technically be a nullptr
> received, I check. Because this way the check is in one place, instead
> of doing `xx_desc ? xx_desc->thing : failover` everywhere.
> 

> I can change this to an assert as mentioned by Nicolas, of course.

please do

if theres a NULL check, both the human reader as well as things like coverity
assume it can be NULL. And both get confused if thats not actually possible


> 
> >
> > >
> > >          sws_getColorspaceDetails(scale->sws, (int **)&inv_table, &in_full,
> > >                                   (int **)&table, &out_full,
> > >                                   &brightness, &contrast, &saturation);
> >
> > > +        // translate the swscale internal range flags to hold true
> > > +        // for RGB
> >
> > The range field of AVFrame is documented as
> > "MPEG vs JPEG YUV range."
> >
> 
> Yes, but I think quite a few people at this point just read it "full
> range" VS "limited range" without any connotations towards YCbCr
> specifically. Personally, I consider the enumeration names as just old
> left-overs from the time when the MPEG/JPEG or TV/PC naming sounded
> like a good idea and that nobody wanted to start the change of adding
> new defines and enum values etc, doing deprecation etc as that is
> quite a lot of work just to get the naming of enums nicer. Case in
> point - for example pngdec sets the range value for the RGB frames it
> returns.
> 
> Of course unless you specifically want to support limited range RGB,
> RGB should not only be implied being full range by default but also
> always set to be full range everywhere.
> 
> > so it is not defined for RGB and cannot be "wrong" for RGB
> > maybe iam nitpicking here but if you want to use the range for RGB
> > the API docs need to be changed first.
> >
> 
> The code was ALREADY doing that, and I'm just converting the value so
> that you don't end up with RGB + limited range AVFrames?!
> 
> > <super nitpick>I mean you could set it to 1 for RGB thats ok without a API
> > docs change. But writing in a comment that one way is correct for RGB is not
> > compatible with the current documentation</super nitpick>
> >
> 
> So you want another patch that changes it to just say "limited/narrow
> range" and "full range"? Like seriously, I just wanted to fix a
> problem that got found because I finally started plugging some pipes
> together!

If you want to change the range enum and field so it also applies to
RGB and not just YUV. That should be done in a consistent and complete
way and i imagine will not be just "one more thing"

otherwise if the API isnt changed, this variable is not describing RGB
I dont think theres a problem if you set it to a specific value for RGB
in that case but you cannot call it a bug or wrong if its not set that
way. Basically code which expects it to be a specific value for RGB is
where the bug is.

also the full YUV range is 2 and limited range is 1. 0 is unspecified
in our enum.
so i wonder if RGB shouldnt be AVCOL_RANGE_UNSPECIFIED instead of AVCOL_RANGE_JPEG
which i think is what this patch would do.
That is if the range API isnt extended to include RGB.

All that said, i think extending the range API to cover RGB would be a good
idea. But its surely more work


thx

[...]