Message ID | 20201007182720.26619-1-jeebjp@gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,v2] swscale: separate exported and internal range flags | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
On Wed, Oct 7, 2020 at 9:27 PM Jan Ekström <jeebjp@gmail.com> wrote: > > Fixes vf_scale outputting RGB AVFrames with limited range in > case either input or output specifically sets the range. > > Keeps the external interfaces the same, but allows us to retrieve > and set nonzero value for RGB. Additionally defines the default > value of the AVOption as -1 so we can differentiate between > attempting to force limited range and requesting the default. > --- > libswscale/options.c | 4 +- > libswscale/swscale_internal.h | 3 + > libswscale/utils.c | 160 +++++++++++++++++++++++----------- > 3 files changed, 114 insertions(+), 53 deletions(-) > Ping.
On Wed, Oct 07, 2020 at 09:27:20PM +0300, Jan Ekström wrote: > Fixes vf_scale outputting RGB AVFrames with limited range in > case either input or output specifically sets the range. > > Keeps the external interfaces the same, but allows us to retrieve > and set nonzero value for RGB. Additionally defines the default > value of the AVOption as -1 so we can differentiate between > attempting to force limited range and requesting the default. > --- > libswscale/options.c | 4 +- > libswscale/swscale_internal.h | 3 + > libswscale/utils.c | 160 +++++++++++++++++++++++----------- > 3 files changed, 114 insertions(+), 53 deletions(-) This changes the output for: ./ffmpeg -i ~/tickets/4926/5611842b1f09f2_20295212.png -vf scale=34:44 file.tga same with png output That is the red looks a bit different, the filesize also changes file should be here: https://trac.ffmpeg.org/attachment/ticket/4926/5611842b1f09f2_20295212.png I dont know if the one after the patch is more or less correct, but why does your patch change this ? is that change intentional ? thx [...]
On Fri, Oct 9, 2020 at 10:02 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Wed, Oct 07, 2020 at 09:27:20PM +0300, Jan Ekström wrote: > > Fixes vf_scale outputting RGB AVFrames with limited range in > > case either input or output specifically sets the range. > > > > Keeps the external interfaces the same, but allows us to retrieve > > and set nonzero value for RGB. Additionally defines the default > > value of the AVOption as -1 so we can differentiate between > > attempting to force limited range and requesting the default. > > --- > > libswscale/options.c | 4 +- > > libswscale/swscale_internal.h | 3 + > > libswscale/utils.c | 160 +++++++++++++++++++++++----------- > > 3 files changed, 114 insertions(+), 53 deletions(-) > > This changes the output for: > ./ffmpeg -i ~/tickets/4926/5611842b1f09f2_20295212.png -vf scale=34:44 file.tga > same with png output > > That is the red looks a bit different, the filesize also changes > file should be here: > https://trac.ffmpeg.org/attachment/ticket/4926/5611842b1f09f2_20295212.png > > I dont know if the one after the patch is more or less correct, but why > does your patch change this ? > is that change intentional ? > Had a look at the stuff that happens by adding a bunch of debug logging: before: [Parsed_scale_0 @ 0x611000001080] w:34 h:33 flags:'bicubic' interl:0 [graph 0 input from stream 0:0 @ 0x6110000011c0] w:2200 h:2827 pixfmt:rgba tb:1/25 fr:25/1 sar:0/1 [swscaler @ 0x62f00000e400] sws_init_context: Initial configured range values: src: 0, dst: 0 [swscaler @ 0x62f00000e400] sws_init_context: color ranges: src (rgba): 0 dst (rgba): 0 [swscaler @ 0x62f00000e400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (rgba): 0 (given arguments: src: 0, dst: 0) [swscaler @ 0x62f00001c400] sws_init_context: Initial configured range values: src: 0, dst: 0 [swscaler @ 0x62f00001c400] sws_init_context: color ranges: src (rgba): 0 dst (yuva420p): 0 [swscaler @ 0x62f00001c400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (yuva420p): 0 (given arguments: src: 0, dst: 0) [swscaler @ 0x62f00002a400] sws_init_context: Initial configured range values: src: 0, dst: 0 [swscaler @ 0x62f00002a400] sws_init_context: color ranges: src (yuva420p): 0 dst (rgba): 0 [swscaler @ 0x62f00002a400] sws_setColorspaceDetails: color ranges: src (yuva420p): 0, dst (rgba): 0 (given arguments: src: 0, dst: 0) [Parsed_scale_0 @ 0x611000001080] w:2200 h:2827 fmt:rgba sar:0/1 -> w:34 h:33 fmt:rgba sar:0/1 flags:0x4 [Parsed_scale_0 @ 0x611000001080] scale_frame: received range info: 0, dst: 0 [Parsed_scale_0 @ 0x611000001080] scale_frame: filter chain logic range info: 1, dst: 0 [swscaler @ 0x62f00000e400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (rgba): 0 (given arguments: src: 1, dst: 0) [swscaler @ 0x62f00001c400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (yuva420p): 0 (given arguments: src: 0, dst: 0) before with 'scale=w=34:h=33:out_range=full' filter chain: [Parsed_scale_0 @ 0x611000001080] w:34 h:33 flags:'bicubic' interl:0 [graph 0 input from stream 0:0 @ 0x6110000011c0] w:2200 h:2827 pixfmt:rgba tb:1/25 fr:25/1 sar:0/1 [swscaler @ 0x62f00000e400] sws_init_context: Initial configured range values: src: 0, dst: 1 [swscaler @ 0x62f00000e400] sws_init_context: color ranges: src (rgba): 0 dst (rgba): 1 [swscaler @ 0x62f00000e400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (rgba): 0 (given arguments: src: 0, dst: 1) [swscaler @ 0x62f00001c400] sws_init_context: Initial configured range values: src: 0, dst: 0 [swscaler @ 0x62f00001c400] sws_init_context: color ranges: src (rgba): 0 dst (yuva420p): 0 [swscaler @ 0x62f00001c400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (yuva420p): 0 (given arguments: src: 0, dst: 0) [swscaler @ 0x62f00002a400] sws_init_context: Initial configured range values: src: 0, dst: 0 [swscaler @ 0x62f00002a400] sws_init_context: color ranges: src (yuva420p): 0 dst (rgba): 0 [swscaler @ 0x62f00002a400] sws_setColorspaceDetails: color ranges: src (yuva420p): 0, dst (rgba): 0 (given arguments: src: 0, dst: 0) [Parsed_scale_0 @ 0x611000001080] w:2200 h:2827 fmt:rgba sar:0/1 -> w:34 h:33 fmt:rgba sar:0/1 flags:0x4 [Parsed_scale_0 @ 0x611000001080] scale_frame: received range info: 0, dst: 0 [Parsed_scale_0 @ 0x611000001080] scale_frame: filter chain logic range info: 1, dst: 1 [swscaler @ 0x62f00000e400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (rgba): 0 (given arguments: src: 1, dst: 1) [swscaler @ 0x62f00001c400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (yuva420p): 0 (given arguments: src: 0, dst: 0) after: [Parsed_scale_0 @ 0x611000001080] w:34 h:33 flags:'bicubic' interl:0 [graph 0 input from stream 0:0 @ 0x6110000011c0] w:2200 h:2827 pixfmt:rgba tb:1/25 fr:25/1 sar:0/1 [swscaler @ 0x62f00000e400] sws_init_context: Initial configured range values: src: -1, dst: -1 [swscaler @ 0x62f00000e400] sws_init_context: color ranges: src (rgba): 0 src exported: 1, dst (rgba): 0, dst exported: 1 [swscaler @ 0x62f00000e400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (rgba): 0 (given arguments: src: 1, dst: 1) [swscaler @ 0x62f00001c400] sws_init_context: Initial configured range values: src: -1, dst: -1 [swscaler @ 0x62f00001c400] sws_init_context: color ranges: src (rgba): 0 src exported: 1, dst (yuva420p): 0, dst exported: 0 [swscaler @ 0x62f00001c400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (yuva420p): 0 (given arguments: src: 1, dst: 0) [swscaler @ 0x62f00002a400] sws_init_context: Initial configured range values: src: -1, dst: -1 [swscaler @ 0x62f00002a400] sws_init_context: color ranges: src (yuva420p): 0 src exported: 0, dst (rgba): 0, dst exported: 1 [swscaler @ 0x62f00002a400] sws_setColorspaceDetails: color ranges: src (yuva420p): 0, dst (rgba): 0 (given arguments: src: 0, dst: 1) [Parsed_scale_0 @ 0x611000001080] w:2200 h:2827 fmt:rgba sar:0/1 -> w:34 h:33 fmt:rgba sar:0/1 flags:0x4 [Parsed_scale_0 @ 0x611000001080] scale_frame: received range info: 1, dst: 1 [Parsed_scale_0 @ 0x611000001080] scale_frame: filter chain logic range info: 1, dst: 1 [swscaler @ 0x62f00000e400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (rgba): 0 (given arguments: src: 1, dst: 1) [swscaler @ 0x62f00001c400] sws_setColorspaceDetails: color ranges: src (rgba): 0, dst (yuva420p): 1 (given arguments: src: 1, dst: 1) So how I see this: 1. RGBA is apparently handled with a primary RGBA swscaler and a secondary yuva420p scaler. 2. This yuva420p scaler is called in limited range currently. 3. sws_setColorspaceDetails blindly passes the range value to sub-scalers. In this case a yuva420p one. 4. Before there was no difference between the internal and external range value, and for the base thing RGBA became 0,0. 3. Now the value being passed is the external one, which ends up being 1,1. 4. For RGBA we override the value to 0 internally, but for yuva420p full range is valid, so no adjustment is made. Thus we get a zero-standard RGBA and full range yuva420p. 5. The yuva420p to RGBA conversion is still 0 on both sides. So yes, this is one of these cases not caught by FATE tests, and I would guess the largest difference comes from now the ranges of the yuva420p sub-scalers not matching? Not sure how to improve this quickly other than just scrapping the idea of attempting to properly have two separate flags, and just adjusting the flag to full range in sws_getColorspaceDetails. Which is ugly, but will not require us to go through any more untested swscale area... Jan
diff --git a/libswscale/options.c b/libswscale/options.c index 7eb2752543..48dcfc7634 100644 --- a/libswscale/options.c +++ b/libswscale/options.c @@ -59,8 +59,8 @@ static const AVOption swscale_options[] = { { "dsth", "destination height", OFFSET(dstH), AV_OPT_TYPE_INT, { .i64 = 16 }, 1, INT_MAX, VE }, { "src_format", "source format", OFFSET(srcFormat), AV_OPT_TYPE_PIXEL_FMT,{ .i64 = DEFAULT }, 0, INT_MAX, VE }, { "dst_format", "destination format", OFFSET(dstFormat), AV_OPT_TYPE_PIXEL_FMT,{ .i64 = DEFAULT }, 0, INT_MAX, VE }, - { "src_range", "source is full range", OFFSET(srcRange), AV_OPT_TYPE_BOOL, { .i64 = DEFAULT }, 0, 1, VE }, - { "dst_range", "destination is full range", OFFSET(dstRange), AV_OPT_TYPE_BOOL, { .i64 = DEFAULT }, 0, 1, VE }, + { "src_range", "source is full range", OFFSET(exported_srcRange), AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VE }, + { "dst_range", "destination is full range", OFFSET(exported_dstRange), AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VE }, { "param0", "scaler param 0", OFFSET(param[0]), AV_OPT_TYPE_DOUBLE, { .dbl = SWS_PARAM_DEFAULT }, INT_MIN, INT_MAX, VE }, { "param1", "scaler param 1", OFFSET(param[1]), AV_OPT_TYPE_DOUBLE, { .dbl = SWS_PARAM_DEFAULT }, INT_MIN, INT_MAX, VE }, diff --git a/libswscale/swscale_internal.h b/libswscale/swscale_internal.h index d207d3beff..60ef33dd9b 100644 --- a/libswscale/swscale_internal.h +++ b/libswscale/swscale_internal.h @@ -625,6 +625,9 @@ typedef struct SwsContext { SwsDither dither; SwsAlphaBlend alphablend; + + int exported_srcRange; + int exported_dstRange; } SwsContext; //FIXME check init (where 0) diff --git a/libswscale/utils.c b/libswscale/utils.c index 9ca378bd3b..f7efb4ab5d 100644 --- a/libswscale/utils.c +++ b/libswscale/utils.c @@ -864,6 +864,74 @@ static void fill_xyztables(struct SwsContext *c) } } +static int handle_jpeg(enum AVPixelFormat *format) +{ + switch (*format) { + case AV_PIX_FMT_YUVJ420P: + *format = AV_PIX_FMT_YUV420P; + return 1; + case AV_PIX_FMT_YUVJ411P: + *format = AV_PIX_FMT_YUV411P; + return 1; + case AV_PIX_FMT_YUVJ422P: + *format = AV_PIX_FMT_YUV422P; + return 1; + case AV_PIX_FMT_YUVJ444P: + *format = AV_PIX_FMT_YUV444P; + return 1; + case AV_PIX_FMT_YUVJ440P: + *format = AV_PIX_FMT_YUV440P; + return 1; + case AV_PIX_FMT_GRAY8: + case AV_PIX_FMT_YA8: + case AV_PIX_FMT_GRAY9LE: + case AV_PIX_FMT_GRAY9BE: + case AV_PIX_FMT_GRAY10LE: + case AV_PIX_FMT_GRAY10BE: + case AV_PIX_FMT_GRAY12LE: + case AV_PIX_FMT_GRAY12BE: + case AV_PIX_FMT_GRAY14LE: + case AV_PIX_FMT_GRAY14BE: + case AV_PIX_FMT_GRAY16LE: + case AV_PIX_FMT_GRAY16BE: + case AV_PIX_FMT_YA16BE: + case AV_PIX_FMT_YA16LE: + return 1; + default: + return 0; + } +} + +static int range_override_needed(enum AVPixelFormat format) +{ + return !isYUV(format) && !isGray(format); +} + +static int check_format_range(SwsContext *c, enum AVPixelFormat *format, + int range, const char *descriptor) +{ + const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(*format); + int default_range = handle_jpeg(format); + av_assert0(desc); + + if (range_override_needed(*format) && !range) { + // first, handle the special case of limited range RGB + av_log(c, AV_LOG_WARNING, + "%s range set to limited for %s, unsupported! " + "Overriding to full range.\n", + descriptor ? descriptor : "Color", desc->name); + + return 1; + } + + + if (range != -1) + // for YCbCr and gray, we return the value if set + return range; + + return range_override_needed(*format) ? 1 : default_range; +} + int sws_setColorspaceDetails(struct SwsContext *c, const int inv_table[4], int srcRange, const int table[4], int dstRange, int brightness, int contrast, int saturation) @@ -876,10 +944,25 @@ int sws_setColorspaceDetails(struct SwsContext *c, const int inv_table[4], desc_dst = av_pix_fmt_desc_get(c->dstFormat); desc_src = av_pix_fmt_desc_get(c->srcFormat); - if(!isYUV(c->dstFormat) && !isGray(c->dstFormat)) - dstRange = 0; - if(!isYUV(c->srcFormat) && !isGray(c->srcFormat)) - srcRange = 0; + if (dstRange == -1) { + dstRange = c->dstRange; + } else { + c->exported_dstRange = check_format_range(c, &c->dstFormat, + dstRange, + "Destination"); + dstRange = range_override_needed(c->dstFormat) ? + 0 : c->exported_dstRange; + } + + if (srcRange == -1) { + srcRange = c->srcRange; + } else { + c->exported_srcRange = check_format_range(c, &c->srcFormat, + srcRange, + "Source"); + srcRange = range_override_needed(c->srcFormat) ? + 0 : c->exported_srcRange; + } if (c->srcRange != srcRange || c->dstRange != dstRange || @@ -911,7 +994,7 @@ int sws_setColorspaceDetails(struct SwsContext *c, const int inv_table[4], c->srcFormatBpp = av_get_bits_per_pixel(desc_src); if (c->cascaded_context[c->cascaded_mainindex]) - return sws_setColorspaceDetails(c->cascaded_context[c->cascaded_mainindex],inv_table, srcRange,table, dstRange, brightness, contrast, saturation); + return sws_setColorspaceDetails(c->cascaded_context[c->cascaded_mainindex],inv_table, c->exported_srcRange,table, c->exported_dstRange, brightness, contrast, saturation); if (!need_reinit) return 0; @@ -968,7 +1051,8 @@ int sws_setColorspaceDetails(struct SwsContext *c, const int inv_table[4], return ret; //we set both src and dst depending on that the RGB side will be ignored sws_setColorspaceDetails(c->cascaded_context[0], inv_table, - srcRange, table, dstRange, + c->exported_srcRange, table, + c->exported_dstRange, brightness, contrast, saturation); c->cascaded_context[1] = sws_getContext(tmp_width, tmp_height, tmp_format, @@ -977,7 +1061,8 @@ int sws_setColorspaceDetails(struct SwsContext *c, const int inv_table[4], if (!c->cascaded_context[1]) return -1; sws_setColorspaceDetails(c->cascaded_context[1], inv_table, - srcRange, table, dstRange, + c->exported_srcRange, table, + c->exported_dstRange, 0, 1 << 16, 1 << 16); return 0; } @@ -1008,8 +1093,8 @@ int sws_getColorspaceDetails(struct SwsContext *c, int **inv_table, *inv_table = c->srcColorspaceTable; *table = c->dstColorspaceTable; - *srcRange = c->srcRange; - *dstRange = c->dstRange; + *srcRange = c->exported_srcRange; + *dstRange = c->exported_dstRange; *brightness = c->brightness; *contrast = c->contrast; *saturation = c->saturation; @@ -1017,44 +1102,6 @@ int sws_getColorspaceDetails(struct SwsContext *c, int **inv_table, return 0; } -static int handle_jpeg(enum AVPixelFormat *format) -{ - switch (*format) { - case AV_PIX_FMT_YUVJ420P: - *format = AV_PIX_FMT_YUV420P; - return 1; - case AV_PIX_FMT_YUVJ411P: - *format = AV_PIX_FMT_YUV411P; - return 1; - case AV_PIX_FMT_YUVJ422P: - *format = AV_PIX_FMT_YUV422P; - return 1; - case AV_PIX_FMT_YUVJ444P: - *format = AV_PIX_FMT_YUV444P; - return 1; - case AV_PIX_FMT_YUVJ440P: - *format = AV_PIX_FMT_YUV440P; - return 1; - case AV_PIX_FMT_GRAY8: - case AV_PIX_FMT_YA8: - case AV_PIX_FMT_GRAY9LE: - case AV_PIX_FMT_GRAY9BE: - case AV_PIX_FMT_GRAY10LE: - case AV_PIX_FMT_GRAY10BE: - case AV_PIX_FMT_GRAY12LE: - case AV_PIX_FMT_GRAY12BE: - case AV_PIX_FMT_GRAY14LE: - case AV_PIX_FMT_GRAY14BE: - case AV_PIX_FMT_GRAY16LE: - case AV_PIX_FMT_GRAY16BE: - case AV_PIX_FMT_YA16BE: - case AV_PIX_FMT_YA16LE: - return 1; - default: - return 0; - } -} - static int handle_0alpha(enum AVPixelFormat *format) { switch (*format) { @@ -1200,16 +1247,27 @@ av_cold int sws_init_context(SwsContext *c, SwsFilter *srcFilter, unscaled = (srcW == dstW && srcH == dstH); - c->srcRange |= handle_jpeg(&c->srcFormat); - c->dstRange |= handle_jpeg(&c->dstFormat); + c->exported_srcRange = check_format_range(c, &c->srcFormat, + c->exported_srcRange, + "Source"); + c->exported_dstRange = check_format_range(c, &c->dstFormat, + c->exported_dstRange, + "Destination"); + + // convert exported range to internal + c->srcRange = range_override_needed(srcFormat) ? + 0 : c->exported_srcRange; + c->dstRange = range_override_needed(dstFormat) ? + 0 : c->exported_dstRange; if(srcFormat!=c->srcFormat || dstFormat!=c->dstFormat) av_log(c, AV_LOG_WARNING, "deprecated pixel format used, make sure you did set range correctly\n"); if (!c->contrast && !c->saturation && !c->dstFormatBpp) - sws_setColorspaceDetails(c, ff_yuv2rgb_coeffs[SWS_CS_DEFAULT], c->srcRange, + sws_setColorspaceDetails(c, ff_yuv2rgb_coeffs[SWS_CS_DEFAULT], + c->exported_srcRange, ff_yuv2rgb_coeffs[SWS_CS_DEFAULT], - c->dstRange, 0, 1 << 16, 1 << 16); + c->exported_dstRange, 0, 1 << 16, 1 << 16); handle_formats(c); srcFormat = c->srcFormat;