[vlc-devel] [PATCH] macosx: fix inaccurate RGB color display

Thomas Guillem thomas at gllm.fr
Thu Jul 27 04:51:33 CEST 2017


Great job!

You could split this commit into 3 commits and...

On Thu, Jul 27, 2017, at 01:17, Victorien Le Couviour--Tuffet wrote:
> Handling the primaries and color space results in inaccurate RGB color
> display.
> BT601, BT709 and BT2020 outputs get accurate without handling.
> Tested on several full red samples of different sizes (different
> primaries).
> 
> Fixes #17383.
> ---
> remove conflict with 44ae5e72db59afc31996e35c11022f3dd5d2d153
> 
>  modules/video_output/caopengllayer.m         | 70 ----------------------
>  modules/video_output/macosx.m                | 87
>  ----------------------------
>  modules/video_output/opengl/converter_cvpx.c |  6 +-
>  3 files changed, 3 insertions(+), 160 deletions(-)
> 
> diff --git a/modules/video_output/caopengllayer.m
> b/modules/video_output/caopengllayer.m
> index 2a3cca5712..b1ddcc21a7 100644
> --- a/modules/video_output/caopengllayer.m
> +++ b/modules/video_output/caopengllayer.m
> @@ -43,15 +43,8 @@
>  
>  #include "opengl/vout_helper.h"
>  
> -#define OSX_EL_CAPITAN_AND_HIGHER (NSAppKitVersionNumber >= 1404)
>  #define OSX_SIERRA_AND_HIGHER (NSAppKitVersionNumber >= 1485)
>  
> -#if MAC_OS_X_VERSION_MIN_ALLOWED <= MAC_OS_X_VERSION_10_11
> -const CFStringRef kCGColorSpaceDCIP3 = CFSTR("kCGColorSpaceDCIP3");
> -const CFStringRef kCGColorSpaceITUR_709 =
> CFSTR("kCGColorSpaceITUR_709");
> -const CFStringRef kCGColorSpaceITUR_2020 =
> CFSTR("kCGColorSpaceITUR_2020");
> -#endif
> -
>  /*****************************************************************************
>   * Vout interface
>   *****************************************************************************/
> @@ -99,8 +92,6 @@ struct vout_display_sys_t {
>      vout_window_t *embed;
>      VLCCAOpenGLLayer *cgLayer;
>  
> -    CGColorSpaceRef cgColorSpace;
> -
>      vlc_gl_t *gl;
>      vout_display_opengl_t *vgl;
>  
> @@ -215,64 +206,6 @@ static int Open (vlc_object_t *p_this)
>          vd->display = PictureDisplay;
>          vd->control = Control;
>  
> -        /* handle color space if supported by the OS */
> -        if ([sys->cgLayer respondsToSelector:@selector(setColorspace:)])
> {
> -
> -            /* support for BT.709 and BT.2020 color spaces was
> introduced with OS X 10.11
> -             * on older OS versions, we can't show correct colors, so we
> fallback on linear RGB */
> -            if (OSX_EL_CAPITAN_AND_HIGHER) {
> -                switch (fmt.primaries) {
> -                    case COLOR_PRIMARIES_BT601_525:
> -                    case COLOR_PRIMARIES_BT601_625:
> -                    {
> -                        msg_Dbg(vd, "Using BT.601 color space");
> -                        sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceSRGB);
> -                        break;
> -                    }
> -                    case COLOR_PRIMARIES_BT709:
> -                    {
> -                        msg_Dbg(vd, "Using BT.709 color space");
> -                        sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceITUR_709);
> -                        break;
> -                    }
> -                    case COLOR_PRIMARIES_BT2020:
> -                    {
> -                        msg_Dbg(vd, "Using BT.2020 color space");
> -                        sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceITUR_2020);
> -                        break;
> -                    }
> -                    case COLOR_PRIMARIES_DCI_P3:
> -                    {
> -                        msg_Dbg(vd, "Using DCI P3 color space");
> -                        sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceDCIP3);
> -                        break;
> -                    }
> -                    default:
> -                    {
> -                        msg_Dbg(vd, "Guessing color space based on video
> dimensions (%ix%i)", fmt.i_visible_width, fmt.i_visible_height);
> -                        if (fmt.i_visible_height >= 2000 ||
> fmt.i_visible_width >= 3800) {
> -                            msg_Dbg(vd, "Using BT.2020 color space");
> -                            sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceITUR_2020);
> -                        } else if (fmt.i_height > 576) {
> -                            msg_Dbg(vd, "Using BT.709 color space");
> -                            sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceITUR_709);
> -                        } else {
> -                            msg_Dbg(vd, "SD content, using linear RGB
> color space");
> -                            sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceSRGB);
> -                        }
> -                        break;
> -                    }
> -                }
> -            } else {
> -                msg_Dbg(vd, "OS does not support BT.709 or BT.2020 color
> spaces, output may vary");
> -                sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceGenericRGBLinear);
> -            }
> -
> -            [sys->cgLayer setColorspace: sys->cgColorSpace];
> -        } else {
> -            msg_Dbg(vd, "OS does not support custom color spaces, output
> may be undefined");
> -        }
> -
>          if (OSX_SIERRA_AND_HIGHER) {
>              /* request our screen's HDR mode (introduced in OS X 10.11,
>              but correctly supported in 10.12 only) */
>              if ([sys->cgLayer
>              respondsToSelector:@selector(setWantsExtendedDynamicRangeContent:)])
>              {
> @@ -333,9 +266,6 @@ static void Close (vlc_object_t *p_this)
>      if ([sys->cgLayer glContext])
>          CGLReleaseContext([sys->cgLayer glContext]);
>  
> -    if (sys->cgColorSpace != nil)
> -        CGColorSpaceRelease(sys->cgColorSpace);
> -
>      free(sys);
>  }
>  
> diff --git a/modules/video_output/macosx.m
> b/modules/video_output/macosx.m
> index 3fa35c592d..2770c44305 100644
> --- a/modules/video_output/macosx.m
> +++ b/modules/video_output/macosx.m
> @@ -49,14 +49,6 @@
>  #include <vlc_dialog.h>
>  #include "opengl/vout_helper.h"
>  
> -#define OSX_EL_CAPITAN (NSAppKitVersionNumber >= 1404)
> -
> -#if MAC_OS_X_VERSION_MIN_ALLOWED <= MAC_OS_X_VERSION_10_11
> -const CFStringRef kCGColorSpaceDCIP3 = CFSTR("kCGColorSpaceDCIP3");
> -const CFStringRef kCGColorSpaceITUR_709 =
> CFSTR("kCGColorSpaceITUR_709");
> -const CFStringRef kCGColorSpaceITUR_2020 =
> CFSTR("kCGColorSpaceITUR_2020");
> -#endif
> -
>  /**
>   * Forward declarations
>   */
> @@ -112,9 +104,6 @@ struct vout_display_sys_t
>      VLCOpenGLVideoView *glView;
>      id<VLCOpenGLVideoViewEmbedding> container;
>  
> -    CGColorSpaceRef cgColorSpace;
> -    NSColorSpace *nsColorSpace;
> -
>      vout_window_t *embed;
>      vlc_gl_t *gl;
>      vout_display_opengl_t *vgl;
> @@ -176,57 +165,6 @@ static int Open (vlc_object_t *this)
>           * main thread, after we are done using it. */
>          sys->container = [container retain];
>  
> -        /* support for BT.709 and BT.2020 color spaces was introduced
> with OS X 10.11
> -         * on older OS versions, we can't show correct colors, so we
> fallback on linear RGB */
> -        if (OSX_EL_CAPITAN) {
> -            switch (vd->fmt.primaries) {
> -                case COLOR_PRIMARIES_BT601_525:
> -                case COLOR_PRIMARIES_BT601_625:
> -                {
> -                    msg_Dbg(vd, "Using BT.601 color space");
> -                    sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceSRGB);
> -                    break;
> -                }
> -                case COLOR_PRIMARIES_BT709:
> -                {
> -                    msg_Dbg(vd, "Using BT.709 color space");
> -                    sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceITUR_709);
> -                    break;
> -                }
> -                case COLOR_PRIMARIES_BT2020:
> -                {
> -                    msg_Dbg(vd, "Using BT.2020 color space");
> -                    sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceITUR_2020);
> -                    break;
> -                }
> -                case COLOR_PRIMARIES_DCI_P3:
> -                {
> -                    msg_Dbg(vd, "Using DCI P3 color space");
> -                    sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceDCIP3);
> -                    break;
> -                }
> -                default:
> -                {
> -                    msg_Dbg(vd, "Guessing color space based on video
> dimensions (%ix%i)", vd->fmt.i_visible_width, vd->fmt.i_visible_height);
> -                    if (vd->fmt.i_visible_height >= 2000 ||
> vd->fmt.i_visible_width >= 3800) {
> -                        msg_Dbg(vd, "Using BT.2020 color space");
> -                        sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceITUR_2020);
> -                    } else if (vd->fmt.i_height > 576) {
> -                        msg_Dbg(vd, "Using BT.709 color space");
> -                        sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceITUR_709);
> -                    } else {
> -                        msg_Dbg(vd, "SD content, using linear RGB color
> space");
> -                        sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceSRGB);
> -                    }
> -                    break;
> -                }
> -            }
> -        } else {
> -            msg_Dbg(vd, "OS does not support BT.709 or BT.2020 color
> spaces, output may vary");
> -            sys->cgColorSpace =
> CGColorSpaceCreateWithName(kCGColorSpaceSRGB);
> -        }
> -        sys->nsColorSpace = [[NSColorSpace alloc]
> initWithCGColorSpace:sys->cgColorSpace];
> -
>          /* Get our main view*/
>          [VLCOpenGLVideoView
>          performSelectorOnMainThread:@selector(getNewView:)
>                                               withObject:[NSValue
>                                               valueWithPointer:&sys->glView]
> @@ -360,12 +298,6 @@ void Close (vlc_object_t *this)
>  
>          [sys->glView release];
>  
> -        if (sys->cgColorSpace != nil)
> -            CGColorSpaceRelease(sys->cgColorSpace);
> -
> -        if (sys->nsColorSpace != nil)
> -            [sys->nsColorSpace release];
> -
>          if (sys->embed)
>              vout_display_DeleteWindow (vd, sys->embed);
>          free (sys);
> @@ -904,23 +836,4 @@ static void OpenglSwap (vlc_gl_t *gl)
>      return YES;
>  }
>  
> -- (void)viewWillMoveToWindow:(nullable NSWindow *)newWindow
> -{
> -    [super viewWillMoveToWindow:newWindow];
> -
> -    if (newWindow == nil)
> -        return;
> -
> -    @synchronized (self) {
> -        @try {
> -            if (vd) [newWindow setColorSpace:vd->sys->nsColorSpace];
> -        }
> -        @catch (NSException *exception) {
> -            msg_Warn(vd, "Setting the window color space failed due to
> an Obj-C exception (%s, %s", [exception.name UTF8String],
> [exception.reason UTF8String]);
> -        }
> -
> -    }
> -
> -}
> -
>  @end
> diff --git a/modules/video_output/opengl/converter_cvpx.c
> b/modules/video_output/opengl/converter_cvpx.c
> index 7864d3f25b..e388551b2e 100644
> --- a/modules/video_output/opengl/converter_cvpx.c
> +++ b/modules/video_output/opengl/converter_cvpx.c
> @@ -196,7 +196,7 @@ opengl_tex_converter_cvpx_init(opengl_tex_converter_t
> *tc)
>          case VLC_CODEC_CVPX_UYVY:
>              fragment_shader =
>                  opengl_fragment_shader_init(tc, tex_target,
>                  VLC_CODEC_UYVY,
> -                                            tc->fmt.space);
> +                                            COLOR_SPACE_UNDEF);

Explain in a comment that this is handled by videotoolbox.

>              tc->texs[0].internal = GL_RGB;
>              tc->texs[0].format = GL_RGB_422_APPLE;
>              tc->texs[0].type = GL_UNSIGNED_SHORT_8_8_APPLE;
> @@ -205,13 +205,13 @@
> opengl_tex_converter_cvpx_init(opengl_tex_converter_t *tc)
>          {
>              fragment_shader =
>                  opengl_fragment_shader_init(tc, tex_target,
>                  VLC_CODEC_NV12,
> -                                            tc->fmt.space);
> +                                            COLOR_SPACE_UNDEF);
>              break;
>          }
>          case VLC_CODEC_CVPX_I420:
>              fragment_shader =
>                  opengl_fragment_shader_init(tc, tex_target,
>                  VLC_CODEC_I420,
> -                                            tc->fmt.space);
> +                                            COLOR_SPACE_UNDEF);
>              break;
>          case VLC_CODEC_CVPX_BGRA:
>              fragment_shader =
> -- 
> 2.13.1
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel


More information about the vlc-devel mailing list