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

[Jan Ekström]

On Sun, Sep 20, 2020 at 6:03 PM Michael Niedermayer
<michael at niedermayer.cc> wrote:
>
> On Sun, Sep 20, 2020 at 01:36:56PM +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, translate the value to 1 which is what
> > at least this filter expects for RGB. Swscale will ignore this value
> > when set.
> >
> > 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 as the
> > output AVFrame's color_range.
> > ---
> >  libavfilter/vf_scale.c | 44 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
> > index 58eee96744..da8ce399cf 100644
> > --- a/libavfilter/vf_scale.c
> > +++ b/libavfilter/vf_scale.c
> > @@ -647,6 +647,8 @@ static int scale_slice(AVFilterLink *link, AVFrame *out_buf, AVFrame *cur_pic, s
> >                           out,out_stride);
> >  }
> >
> > +// swscale's internal range flag is 0 for RGB, which we have to override
> > +#define NORMALIZE_SWS_RANGE(format_flags, sws_range) (((format_flags) & AV_PIX_FMT_FLAG_RGB) ? 1 : (sws_range))
> >  static int scale_frame(AVFilterLink *link, AVFrame *in, AVFrame **frame_out)
> >  {
> >      AVFilterContext *ctx = link->dst;
> > @@ -750,11 +752,19 @@ 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);
> > +        av_assert0(in_desc && out_desc);
> >
> >          sws_getColorspaceDetails(scale->sws, (int **)&inv_table, &in_full,
> >                                   (int **)&table, &out_full,
> >                                   &brightness, &contrast, &saturation);
> > +        // translate the internal range flags according to this
> > +        // filter's expectations for RGB.
> > +        in_full = NORMALIZE_SWS_RANGE(in_desc->flags, in_full);
> > +        out_full = NORMALIZE_SWS_RANGE(out_desc->flags, out_full);
> >
> >          if (scale->in_color_matrix)
> >              inv_table = parse_yuv_type(scale->in_color_matrix, in->colorspace);
> > @@ -782,7 +792,39 @@ scale:
> >                                       table, out_full,
> >                                       brightness, contrast, saturation);
> >
> > -        out->color_range = out_full ? AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
> > +        // double-check what was actually just configured,
> > +        // since swscale can silently ignore the color range
> > +        // value in sws_setColorspaceDetails.
> > +        sws_getColorspaceDetails(scale->sws, (int **)&inv_table,
> > +                                 &configured_in_full_range_flag,
> > +                                 (int **)&table,
> > +                                 &configured_out_full_range_flag,
> > +                                 &brightness, &contrast, &saturation);
> > +
> > +        // translate the actually configured internal range flags according
> > +        // to this filter's expectations for RGB.
> > +        configured_in_full_range_flag = \
> > +            NORMALIZE_SWS_RANGE(in_desc->flags,
> > +                                configured_in_full_range_flag);
> > +        configured_out_full_range_flag = \
> > +            NORMALIZE_SWS_RANGE(out_desc->flags,
> > +                                configured_out_full_range_flag);
> > +
> > +        if (in_full != configured_in_full_range_flag ||
> > +            out_full != configured_out_full_range_flag) {
> > +            av_log(ctx, AV_LOG_WARNING,
> > +                   "swscale overrode set input/output range value as it "
> > +                   "considered it an invalid configuration! "
> > +                   "(input: requested: %s, configured: %s), "
> > +                   "(output: requested: %s, configured: %s)!\n",
> > +                   in_full ? "full" : "limited",
> > +                   configured_in_full_range_flag ? "full" : "limited",
> > +                   out_full ? "full" : "limited",
> > +                   configured_out_full_range_flag ? "full" : "limited");
> > +        }
>
> If i read this correctly, please correct me if i misread this
> swscale is setup with some ranges and ignores it.
> Is there a reason why swscale should not print a warning in this case ?
> iam asking as you print this from one user of swscale.
> wouldnt this be relevant to other users too ?
>

Yes, but this is a bit different. In this case we compare the
translated value (so if we got ignored in swscale because we tried to
set 1 for full range for RGB, that by itself is not a warning).

This will only warn if you f.ex. tried to set the value of limited
range for RGB, and then as we translate swscale's internal value of 0
to 1 for RGB, you would get a warning since limited range RGB is not
actually supported.

Of course, me or someone else might have found this a bit more quickly
if sws_setColorspaceDetails would indeed log a warning when it would
override the value pushed to it. Alternatively, this could be
partially fixed if sws_getColorspaceDetails would translate the
internal value to 1 for RGB after taking it in from the internal
configuration. Then the API user wouldn't have to translate the value
for RGB to match how swscale assumes things to be.

I am just not sure how deep I want to jump into swscale with this.

Jan