Message ID | 20210607080223.180870-1-val.zapod.vz@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [FFmpeg-devel] fftools/ffplay: 240M matrix is not the same as BT.601 | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
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
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 */ 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 }
Signed-off-by: Valerii Zapodovnikov <val.zapod.vz@gmail.com> --- fftools/ffplay.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)