diff mbox series

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

Message ID 20200920103656.24728-1-jeebjp@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avfilter/vf_scale: translate and verify swscale internal range flag
Related show

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. 20, 2020, 10:36 a.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, 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(-)

Comments

Michael Niedermayer Sept. 20, 2020, 3:02 p.m. UTC | #1
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 ?


thx

[...]
Jan Ekström Sept. 20, 2020, 3:26 p.m. UTC | #2
On Sun, Sep 20, 2020 at 6:03 PM Michael Niedermayer
<michael@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
diff mbox series

Patch

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");
+        }
+
+        out->color_range = configured_out_full_range_flag ?
+                           AVCOL_RANGE_JPEG : AVCOL_RANGE_MPEG;
     }
 
     av_reduce(&out->sample_aspect_ratio.num, &out->sample_aspect_ratio.den,