| Message ID | 20210828221522.61050-1-jeebjp@gmail.com |
|---|---|
| State | Accepted |
| Commit | 2818b143929d86f6b67695b09bc7483da5cef434 |
| Headers | show |
| Series | [FFmpeg-devel] avfilter/vf_scale: set the RGB matrix coefficients in case of RGB | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
probably fine if fate passes
On Sun, Aug 29, 2021 at 9:21 PM Paul B Mahol <onemda@gmail.com> wrote: > > probably fine if fate passes Yea, FATE passes :) . I think this stuff not being noticed until now is due to nothing checking the metadata values returned by vf_scale (since pix_fmt and actual logic is not changed at all with these output AVFrame metadata changes): - The first fix I did was for RGB->YCbCr still being flagged as RGB (and thus encoders like the libx264 wrapper would gladly comply, leading to bugs like issue #9132 ) - This one fixes the opposite conversion where your YCbCr input has matrix coefficients configured, and the RGB output still has that value as-is from the av_frame_copy_props call (and once again, encoder complies). Jan
On Sun, Aug 29, 2021 at 10:05 PM Jan Ekström <jeebjp@gmail.com> wrote: > > On Sun, Aug 29, 2021 at 9:21 PM Paul B Mahol <onemda@gmail.com> wrote: > > > > probably fine if fate passes > > Yea, FATE passes :) . I think this stuff not being noticed until now > is due to nothing checking the metadata values returned by vf_scale > (since pix_fmt and actual logic is not changed at all with these > output AVFrame metadata changes): > - The first fix I did was for RGB->YCbCr still being flagged as RGB > (and thus encoders like the libx264 wrapper would gladly comply, > leading to bugs like issue #9132 ) > - This one fixes the opposite conversion where your YCbCr input has > matrix coefficients configured, and the RGB output still has that > value as-is from the av_frame_copy_props call (and once again, encoder > complies). If there are no further comments, I will soon apply this to fix both sides of the YCbCr<->RGB conversion output in case the input format happens to have the matrix coefficients configured (and thus copied over by av_frame_copy_props). These can then be back-ported to 4.4 since it was the first release to plug input/filtered AVFrames' metadata into output, which brought the issue of input metadata being passed through as-is up. The only question in my mind was whether to set it to AVCOL_SPC_UNSPECIFIED or AVCOL_SPC_RGB . I chose the latter one since the value literally matches what we are checking there: If output pix_fmt is RGB, set output value to RGB. Jan
On Tue, Aug 31, 2021 at 1:14 AM Jan Ekström <jeebjp@gmail.com> wrote: > > On Sun, Aug 29, 2021 at 10:05 PM Jan Ekström <jeebjp@gmail.com> wrote: > > > > On Sun, Aug 29, 2021 at 9:21 PM Paul B Mahol <onemda@gmail.com> wrote: > > > > > > probably fine if fate passes > > > > Yea, FATE passes :) . I think this stuff not being noticed until now > > is due to nothing checking the metadata values returned by vf_scale > > (since pix_fmt and actual logic is not changed at all with these > > output AVFrame metadata changes): > > - The first fix I did was for RGB->YCbCr still being flagged as RGB > > (and thus encoders like the libx264 wrapper would gladly comply, > > leading to bugs like issue #9132 ) > > - This one fixes the opposite conversion where your YCbCr input has > > matrix coefficients configured, and the RGB output still has that > > value as-is from the av_frame_copy_props call (and once again, encoder > > complies). > > If there are no further comments, I will soon apply this to fix both > sides of the YCbCr<->RGB conversion output in case the input format > happens to have the matrix coefficients configured (and thus copied > over by av_frame_copy_props). These can then be back-ported to 4.4 > since it was the first release to plug input/filtered AVFrames' > metadata into output, which brought the issue of input metadata being > passed through as-is up. > > The only question in my mind was whether to set it to > AVCOL_SPC_UNSPECIFIED or AVCOL_SPC_RGB . I chose the latter one since > the value literally matches what we are checking there: If output > pix_fmt is RGB, set output value to RGB. Applied as 2818b143929d86f6b67695b09bc7483da5cef434 . Jan
diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c index 7ca833bbb1..17668206aa 100644 --- a/libavfilter/vf_scale.c +++ b/libavfilter/vf_scale.c @@ -738,13 +738,16 @@ scale: out->width = outlink->w; out->height = outlink->h; - // Sanity check: If we've got the RGB/XYZ (identity) matrix configured, and - // the output is no longer RGB, unset the matrix. - // In theory this should be in swscale itself as the AVFrame - // based API gets in, so that not every swscale API user has - // to go through duplicating such sanity checks. - if (out->colorspace == AVCOL_SPC_RGB && - !(av_pix_fmt_desc_get(out->format)->flags & AV_PIX_FMT_FLAG_RGB)) + // Sanity checks: + // 1. If the output is RGB, set the matrix coefficients to RGB. + // 2. If the output is not RGB and we've got the RGB/XYZ (identity) + // matrix configured, unset the matrix. + // In theory these should be in swscale itself as the AVFrame + // based API gets in, so that not every swscale API user has + // to go through duplicating such sanity checks. + if (av_pix_fmt_desc_get(out->format)->flags & AV_PIX_FMT_FLAG_RGB) + out->colorspace = AVCOL_SPC_RGB; + else if (out->colorspace == AVCOL_SPC_RGB) out->colorspace = AVCOL_SPC_UNSPECIFIED; if (scale->output_is_pal)