Re: [FFmpeg-devel] [PATCH v2] avfilter/vf_scale: translate and verify swscale internal range flag
On Sun, Sep 20, 2020 at 6:03 PM Michael Niedermayer
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 **)_table, _full,
> > (int **), _full,
> > , , );
> > +// 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 **)_table,
> > + _in_full_range_flag,
> > + (int **),
> > + _out_full_range_flag,
> > + , , );
> > +
> > +// 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
Re: [FFmpeg-devel] [PATCH v2] avfilter/vf_scale: translate and verify swscale internal range flag
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 **)_table, _full,
> (int **), _full,
> , , );
> +// 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 **)_table,
> + _in_full_range_flag,
> + (int **),
> + _out_full_range_flag,
> + , , );
> +
> +// 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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
[FFmpeg-devel] [PATCH v2] avfilter/vf_scale: translate and verify swscale internal range flag
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 **)_table, _full,
(int **), _full,
, , );
+// 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 **)_table,
+ _in_full_range_flag,
+ (int **),
+ _out_full_range_flag,
+ , , );
+
+// 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(>sample_aspect_ratio.num, >sample_aspect_ratio.den,
--
2.26.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avfilter/vf_scale: translate and verify swscale internal range flag
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
> 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();
> > > +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 **)_table,
> > > _full,
> > > (int **), _full,
> > > , , );
> >
> > > +// 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?!
>
> > 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
> >
>
> 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
Re: [FFmpeg-devel] [PATCH v2] avfilter/vf_scale: translate and verify swscale internal range flag
On Thu, Sep 17, 2020 at 11:31 PM Michael Niedermayer
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();
> > +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 **)_table, _full,
> > (int **), _full,
> > , , );
>
> > +// 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?!
> 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
>
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avfilter/vf_scale: translate and verify swscale internal range flag
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();
> +av_frame_free(frame_out);
> +return AVERROR_INVALIDDATA;
> +}
can this be true ?
>
> sws_getColorspaceDetails(scale->sws, (int **)_table, _full,
> (int **), _full,
> , , );
> +// 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.
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
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The day soldiers stop bringing you their problems is the day you have stopped
leading them. They have either lost confidence that you can help or concluded
you do not care. Either case is a failure of leadership. - Colin Powell
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v2] avfilter/vf_scale: translate and verify swscale internal range flag
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();
+av_frame_free(frame_out);
+return AVERROR_INVALIDDATA;
+}
sws_getColorspaceDetails(scale->sws, (int **)_table, _full,
(int **), _full,
, , );
+// 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 **)_table,
+ _in_full_range_flag,
+ (int **),
+ _out_full_range_flag,
+ , , );
+
+// 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(>sample_aspect_ratio.num, >sample_aspect_ratio.den,
--
2.26.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
