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

[Michael Niedermayer]

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 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.

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200918/e4ebeda3/attachment.sig>