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

[Jan Ekström]

On Thu, Sep 17, 2020 at 11:31 PM Michael Niedermayer
<michael at 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