diff mbox series

[FFmpeg-devel] fftools/ffplay: fix YUV conversion mode

Message ID 20220621194617.76561-1-ffmpeg@haasn.xyz
State Accepted
Commit a526f0cc3ac938364afe8ce62c4bc47d3b9bb2af
Headers show
Series [FFmpeg-devel] fftools/ffplay: fix YUV conversion mode | expand

Checks

Context Check Description
yinshiyou/configure_loongarch64 warning Failed to apply patch
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished

Commit Message

Niklas Haas June 21, 2022, 7:46 p.m. UTC 
From: Niklas Haas <git@haasn.dev>

GL and Metal cache the state at time of texture creation. GLES2 and
Direct3D 11 use the state at time of the render copy call.

So the only way we can get the correct behavior consistently is by
making sure the state is set for both the upload *and* the draw call.
This probably isn't our bug to fix (upstream should make itself behave
consistently and also document its functions), but as it stands,
`ffplay` is misrendering BT.709 as BT.601 on my stock Linux system, and
that leaves a bad taste in my mouth.

Signed-off-by: Niklas Haas <git@haasn.dev>
---
 fftools/ffplay.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Jan Ekström June 27, 2022, 7:03 a.m. UTC | #1 
On Tue, Jun 21, 2022 at 10:46 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:
>
> From: Niklas Haas <git@haasn.dev>
>
> GL and Metal cache the state at time of texture creation. GLES2 and
> Direct3D 11 use the state at time of the render copy call.
>
> So the only way we can get the correct behavior consistently is by
> making sure the state is set for both the upload *and* the draw call.
> This probably isn't our bug to fix (upstream should make itself behave
> consistently and also document its functions), but as it stands,
> `ffplay` is misrendering BT.709 as BT.601 on my stock Linux system, and
> that leaves a bad taste in my mouth.
>
> Signed-off-by: Niklas Haas <git@haasn.dev>
> ---

Moves the color space setting earlier, as well as clears the state in
case of an additional failure exit case with upload.

LGTM.

Jan
Niklas Haas June 27, 2022, 10:20 a.m. UTC | #2 
On Mon, 27 Jun 2022 10:03:55 +0300 Jan Ekström <jeebjp@gmail.com> wrote:
> On Tue, Jun 21, 2022 at 10:46 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:
> >
> > From: Niklas Haas <git@haasn.dev>
> >
> > GL and Metal cache the state at time of texture creation. GLES2 and
> > Direct3D 11 use the state at time of the render copy call.
> >
> > So the only way we can get the correct behavior consistently is by
> > making sure the state is set for both the upload *and* the draw call.
> > This probably isn't our bug to fix (upstream should make itself behave
> > consistently and also document its functions), but as it stands,
> > `ffplay` is misrendering BT.709 as BT.601 on my stock Linux system, and
> > that leaves a bad taste in my mouth.
> >
> > Signed-off-by: Niklas Haas <git@haasn.dev>
> > ---
> 
> Moves the color space setting earlier, as well as clears the state in
> case of an additional failure exit case with upload.
> 
> LGTM.
> 
> Jan

Merged.
diff mbox series

Patch

diff --git a/fftools/ffplay.c b/fftools/ffplay.c
index 040afa0189..9242047f5c 100644
--- a/fftools/ffplay.c
+++ b/fftools/ffplay.c
@@ -1011,15 +1011,17 @@  static void video_image_display(VideoState *is)
     }
 
     calculate_display_rect(&rect, is->xleft, is->ytop, is->width, is->height, vp->width, vp->height, vp->sar);
+    set_sdl_yuv_conversion_mode(vp->frame);
 
     if (!vp->uploaded) {
-        if (upload_texture(&is->vid_texture, vp->frame, &is->img_convert_ctx) < 0)
+        if (upload_texture(&is->vid_texture, vp->frame, &is->img_convert_ctx) < 0) {
+            set_sdl_yuv_conversion_mode(NULL);
             return;
+        }
         vp->uploaded = 1;
         vp->flip_v = vp->frame->linesize[0] < 0;
     }
 
-    set_sdl_yuv_conversion_mode(vp->frame);
     SDL_RenderCopyEx(renderer, is->vid_texture, NULL, &rect, 0, NULL, vp->flip_v ? SDL_FLIP_VERTICAL : 0);
     set_sdl_yuv_conversion_mode(NULL);
     if (sp) {