Message ID | 20200916201848.262474-1-jeebjp@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [FFmpeg-devel,v2] avfilter/vf_scale: translate and verify swscale internal range flag | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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 [...]
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; + } 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 + in_full = in_desc->flags & AV_PIX_FMT_FLAG_RGB ? + 1 : in_full; + out_full = out_desc->flags & AV_PIX_FMT_FLAG_RGB ? + 1 : out_full; if (scale->in_color_matrix) inv_table = parse_yuv_type(scale->in_color_matrix, in->colorspace); @@ -782,7 +801,37 @@ 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 to hold true + // for RGB as well. + configured_in_full_range_flag = in_desc->flags & AV_PIX_FMT_FLAG_RGB ? + 1 : configured_in_full_range_flag; + configured_out_full_range_flag = out_desc->flags & AV_PIX_FMT_FLAG_RGB ? + 1 : 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,