[FFmpeg-devel] fftools/ffplay: 240M matrix is not the same as BT.601

Signed-off-by: Valerii Zapodovnikov <val.zapod.vz@gmail.com>
---
 fftools/ffplay.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

On Mon, 7 Jun 2021, Valerii Zapodovnikov wrote:

> Signed-off-by: Valerii Zapodovnikov <val.zapod.vz@gmail.com>
> ---
> fftools/ffplay.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> index 0be1d90bf9..53bd9362fa 100644
> --- a/fftools/ffplay.c
> +++ b/fftools/ffplay.c
> @@ -963,12 +963,12 @@ static void set_sdl_yuv_conversion_mode(AVFrame *frame)
>     if (frame && (frame->format == AV_PIX_FMT_YUV420P || frame->format == AV_PIX_FMT_YUYV422 || frame->format == AV_PIX_FMT_UYVY422)) {
>         if (frame->color_range == AVCOL_RANGE_JPEG)
>             mode = SDL_YUV_CONVERSION_JPEG;
> -        else if (frame->colorspace == AVCOL_SPC_BT709)
> +        else if (frame->colorspace == AVCOL_SPC_BT709) /* FIXME: sometimes it selects this even for BT.601 matrix, see issue 8862 */

I still dont understand, what can be fixed in ffplay and how. This FIXME 
helps very little regarding what needs to be done, what is missing and 
when. Either remove it or make it more exact.

Thanks,
Marton

>             mode = SDL_YUV_CONVERSION_BT709;
> -        else if (frame->colorspace == AVCOL_SPC_BT470BG || frame->colorspace == AVCOL_SPC_SMPTE170M || frame->colorspace == AVCOL_SPC_SMPTE240M)
> +        else if (frame->colorspace == AVCOL_SPC_BT470BG || frame->colorspace == AVCOL_SPC_SMPTE170M)
>             mode = SDL_YUV_CONVERSION_BT601;
>     }
> -    SDL_SetYUVConversionMode(mode);
> +    SDL_SetYUVConversionMode(mode); /* FIXME: no support for linear transfer */
> #endif
> }
>
> -- 
> 2.30.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-request@ffmpeg.org with subject "unsubscribe".
>

I cannot clarify it further, the issue is there on trac.

On Mon, 7 Jun 2021, Valerii Zapodovnikov wrote:

> I cannot clarify it further, the issue is there on trac.

And that ticket is bogus, because comparison is made between 4:2:0 and 
4:4:4. And when the reporter says the colors are muddy, he means the pixel 
format diffrerence, not the color space difference. So there is nothing to 
fix regarding the color space, because that is OK, and the different tone 
of green around the edges are only because of the 4:2:0, not because 
the wrong colorspace is used.

Regards,
Marton

You are **very** wrong. This is YCbCr 101: 420 has all the same colors as
444 does. Just if one pixel is fixated the entagled pixels have less than
all possible colors. This is also not corners issues, it is reproducable on
one color all over the plane. Again, the workaround is to use  ffplay -vf
scale=in_color_matrix=bt601,format=gbrp. Why you cannot try it? It is not
THAT hard.

On Tue, 8 Jun 2021, Valerii Zapodovnikov wrote:

> You are **very** wrong. This is YCbCr 101: 420 has all the same colors as
> 444 does. Just if one pixel is fixated the entagled pixels have less than
> all possible colors. This is also not corners issues, it is reproducable on
> one color all over the plane.

Then do reproduce with that and attach the sample having the same metadata 
(colorspace/primaries/transfer) to the ticket, with mpv and ffplay 
screenshots with the rendered output.

I am asking this, because based on the tests I have made I still believe 
there is no color space issue, but feel free to prove me wrong.

Thanks,
Marton

Can you merge my mxfdec patch? Thank you. Maybe all my oldest patches too,
except XYZ patch to libopenjpeg, that is WIP, since openjpeg did not even
merge yet or did a release to support that wrapper option.

Listen, it is commonly known that ffplay is broken, Carl agrees. MPV is not
broken and is perfect according to reference calculator here.
https://res18h39.netlify.app/color

 I did send you a sample in my "v1" patch (this is not v2, since v1 had
wrong title, so v2 would be wrong thing to do, I just superseeded patch on
patchwork). But for your comfort here it is again:
https://trac.ffmpeg.org/attachment/ticket/9167/2.mp4 (also read #9167).

Just *use* the sample above and ffplay it without any flags and ffplay
with -vf scale=in_color_matrix=bt601,format=gbrp.

I do not really care about ffplay, I care about Chromium (since I work on
it) and now VLC. You can carefully read those two issues that use the
sample above too.

https://code.videolan.org/videolan/vlc/-/issues/25651

https://code.videolan.org/videolan/vlc/-/issues/25676

As you can see from those issues I know a lot about YCbCr and read the
first documents from back when ITU-R was CCI(R). I even know that BT.709
was first defined in CCIR Rec. XA/11 1986-90f, a.k.a. IWP 11/6-2108
(Canada). That is not something you can fimd on the Internet, only in ITU-R
archives (wikipedia is me, I wrote it there). So, please, stop with this.

I'll just chime in and say:

FIXME comments aren't that helpful. It would be more helpful to av_log when
you detect you've hit an unsupported situation.

As for SMPTE 240M vs BT.601 Y'CbCr matrices: yes, they're different. But
SDL doesn't support SMPTE 240M. It only supports:

SDL_YUV_CONVERSION_JPEG,  /**< Full range JPEG */
SDL_YUV_CONVERSION_BT601, /**< BT.601 (the default) */
SDL_YUV_CONVERSION_BT709, /**< BT.709 */
(and automatic, but that's just BT.601 + BT.709)

It's probably better to fall back to using BT.709 matrix for SMPTE 240M
(with a warning log) since BT.709 is closer to SMPTE 240M than BT.601 is.

вт, 8 июн. 2021 г., 5:28 Michael Bradshaw <mjbshaw-at-google.com@ffmpeg.org
>:

> I'll just chime in and say:
>
> FIXME comments aren't that helpful. It would be more helpful to av_log when
> you detect you've hit an unsupported situation.
>

How should I know what are unsupported situations? I mean sure, sYCC
(BT.709 primaries with BT.601 matrix and sRGB transfer) is such a case, but
I am not sure what else and it is impossible to detect it (also BT.601
matrix with classic BT.601/709/2020 transfer and SMPTE C primaries, like on
trac)). Because were it possible, I would have JUST FIXED IT.

Also, that will still be not enough as primaries must be also color
managed, except ffplay does not support color managment. BT.601 matrix vs
BT.709 matrix vs BT.2020-ncl matrix is not all, I hope you understand that.
With the case BT.2020-ncl it is not it that make it that bad for BT.2100
(HDR). It is PQ transfer function. Not even primaries.

As for SMPTE 240M vs BT.601 Y'CbCr matrices: yes, they're different. But
> SDL doesn't support SMPTE 240M. It only supports:
>

Yes, I know that. Can you imagine?? Oh, my.


> SDL_YUV_CONVERSION_JPEG,  /**< Full range JPEG */
> SDL_YUV_CONVERSION_BT601, /**< BT.601 (the default) */
> SDL_YUV_CONVERSION_BT709, /**< BT.709 */
> (and automatic, but that's just BT.601 + BT.709)
>
> It's probably better to fall back to using BT.709 matrix for SMPTE 240M
> (with a warning log) since BT.709 is closer to SMPTE 240M than BT.601 is.
>

No. This uses sRGB transfer. sRGB transfer is not close BT.709 OETF (or its
BT.1886 EOTF) because sRGB is an EOTF in by itself. Please do not say it
again.

Wait a second. SDL_SetYUVConversionMode does not support 444? You are
setting it to mode = SDL_YUV_CONVERSION_AUTOMATIC if it is 444, which I
suppose works on just the size of frame? Then this cannot be fixed, I
suppose? There is a workround above, as I said.

On Tue, 8 Jun 2021, Valerii Zapodovnikov wrote:

> Wait a second. SDL_SetYUVConversionMode does not support 444? You are
> setting it to mode = SDL_YUV_CONVERSION_AUTOMATIC if it is 444, which I
> suppose works on just the size of frame?

SDL itself does not support YUV444 pixel format, by the time the frame is 
uploaded into an SDL texture, it is already got converted into one of the 
pixel formats the SDL renderer supports via acceleration. And that is 
usually RGB, YUV420 and for some renderers YUYV422/UYVY422.

Regards,
Marton

On Tue, 8 Jun 2021, Valerii Zapodovnikov wrote:

> Can you merge my mxfdec patch? Thank you.

I'd give at least a week for the mxf maintainer to respond before 
pushing.

> Listen, it is commonly known that ffplay is broken, Carl agrees. MPV is not
> broken and is perfect according to reference calculator here.
> https://res18h39.netlify.app/color
>
> I did send you a sample in my "v1" patch (this is not v2, since v1 had
> wrong title, so v2 would be wrong thing to do, I just superseeded patch on
> patchwork). But for your comfort here it is again:

Patchwork statuses are rarely monitored, so IMHO it is better to use v2 in 
the mailing list even for cases like this.

> https://trac.ffmpeg.org/attachment/ticket/9167/2.mp4 (also read #9167).
>
> Just *use* the sample above and ffplay it without any flags and ffplay
> with -vf scale=in_color_matrix=bt601,format=gbrp.

Both attempts gets me 238,77,44. If you get something else, then I guess 
it is SDL version / renderer / OS / HW dependant? With MPV, it is 
237,77,44 for me.

> I do not really care about ffplay,

Ok.

> I care about Chromium (since I work on
> it) and now VLC. You can carefully read those two issues that use the
> sample above too.
>
> https://code.videolan.org/videolan/vlc/-/issues/25651
>
> https://code.videolan.org/videolan/vlc/-/issues/25676
>
> As you can see from those issues I know a lot about YCbCr and read the
> first documents from back when ITU-R was CCI(R). I even know that BT.709
> was first defined in CCIR Rec. XA/11 1986-90f, a.k.a. IWP 11/6-2108
> (Canada). That is not something you can fimd on the Internet, only in ITU-R
> archives (wikipedia is me, I wrote it there). So, please, stop with this.

I do respect your knowledge regarding colorspaces, but I cannot reproduce 
the issue in 8862, therefore the FIXME makes no sense to me. So applied 
the patch without it.

Regards,
Marton