Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for EGL_EXT_gl_colorspace_bt2020_pq #18

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Add support for EGL_EXT_gl_colorspace_bt2020_pq #18

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1a36e40
define EGL_EXT_gl_colorspace_bt2020_*
tmm1 Jun 29, 2020
684d67b
implement EGL_GL_COLORSPACE_BT2020_PQ_EXT for Metal
tmm1 Jun 29, 2020
f0fe6f8
expose MGLDrawableColorFormatRGBA16 and MGLDrawableColorSpaceBT2020PQ
tmm1 Jun 29, 2020
c5334b4
add to MGLKView also
tmm1 Jun 29, 2020
f5d8be7
switch back to 8bit window format
tmm1 Jun 29, 2020
4f44fe1
add missing validation checks for EGL_GL_COLORSPACE_BT2020_PQ_EXT
tmm1 Jun 29, 2020
c9b9642
remove MGLDrawableColorSpace
tmm1 Jun 30, 2020
b9a86f9
add MGLDrawableColorFormatRGBA16BT2020PQ
tmm1 Jun 30, 2020
3503d77
move metal layer colorspace into SurfaceMtl
tmm1 Jun 30, 2020
247bf59
fix mMetalLayerColorSpace lifecycle
tmm1 Jun 30, 2020
075dbc4
support 16-bit configs
tmm1 Jun 30, 2020
7a8cd44
remove unused header
tmm1 Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ios/xcode/MGLKit/MGLKView.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
@property(nonatomic) MGLDrawableStencilFormat drawableStencilFormat; // Default is StencilNone
@property(nonatomic)
MGLDrawableMultisample drawableMultisample; // Default is MGLDrawableMultisampleNone
@property(nonatomic) MGLDrawableColorSpace drawableColorSpace; // Default is ColorSpaceUnspecified

// Return the size of the OpenGL default framebuffer.
@property(readonly) CGSize drawableSize;
Expand Down
5 changes: 5 additions & 0 deletions ios/xcode/MGLKit/MGLKView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ - (void)setDrawableMultisample:(MGLDrawableMultisample)drawableMultisample
self.glLayer.drawableMultisample = _drawableMultisample = drawableMultisample;
}

- (void)setDrawableColorSpace:(MGLDrawableColorSpace)drawableColorSpace
{
self.glLayer.drawableColorSpace = _drawableColorSpace = drawableColorSpace;
}

- (void)display
{
[self drawRect:self.bounds];
Expand Down
10 changes: 10 additions & 0 deletions ios/xcode/MGLKit/MGLLayer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ typedef enum MGLDrawableColorFormat : int
MGLDrawableColorFormatRGBA8888 = 32,
MGLDrawableColorFormatSRGBA8888 = -32,
MGLDrawableColorFormatRGB565 = 16,
MGLDrawableColorFormatRGBA16 = 64,
tmm1 marked this conversation as resolved.
Show resolved Hide resolved
} MGLDrawableColorFormat;

typedef enum MGLDrawableStencilFormat : int
Expand All @@ -44,6 +45,14 @@ typedef enum MGLDrawableMultisample : int
MGLDrawableMultisample4X = 4,
} MGLDrawableMultisample;

typedef enum MGLDrawableColorSpace : int
{
MGLDrawableColorSpaceUnspecified = 0,
MGLDrawableColorSpaceLinear = 1,
MGLDrawableColorSpaceSRGB = 2,
MGLDrawableColorSpaceBT2020PQ = 2020,
} MGLDrawableColorSpace;

@interface MGLLayer : CALayer

// Return the size of the OpenGL framebuffer.
Expand All @@ -57,6 +66,7 @@ typedef enum MGLDrawableMultisample : int
@property(nonatomic) MGLDrawableStencilFormat drawableStencilFormat; // Default is StencilNone
@property(nonatomic)
MGLDrawableMultisample drawableMultisample; // Default is MGLDrawableMultisampleNone
@property(nonatomic) MGLDrawableColorSpace drawableColorSpace; // Default is ColorSpaceUnspecified

// Default value is NO. Setting to YES will keep the framebuffer data after presenting.
// Doing so will reduce performance and increase memory usage.
Expand Down
13 changes: 13 additions & 0 deletions ios/xcode/MGLKit/MGLLayer.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ - (void)constructor
_drawableDepthFormat = MGLDrawableDepthFormatNone;
_drawableStencilFormat = MGLDrawableStencilFormatNone;
_drawableMultisample = MGLDrawableMultisampleNone;
_drawableColorSpace = MGLDrawableColorSpaceUnspecified;

_display = [MGLDisplay defaultDisplay];

Expand Down Expand Up @@ -710,6 +711,18 @@ - (void)ensureSurfaceCreated
red = green = blue = alpha = 8;
colorSpace = EGL_GL_COLORSPACE_SRGB_KHR;
break;
case MGLDrawableColorFormatRGBA16:
red = green = blue = alpha = 16;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 16 causes Failed to call eglChooseConfig() exception. I think it is probably unnecessary since we are setting the metal pixel format and color space correctly.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a new config to be accepted (such as RGBA 16 bits), it must be added to the supported list inside DisplayMtl::generateConfigs()

Copy link
Contributor Author

@tmm1 tmm1 Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay, I see. That function is a bit confusing to me, but I will try to figure out how to add the 16bit formats in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to make this work. But I still don't understand what effect this has, as opposed to fbo format and texture formats used.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently SurfaceMtl assumes users always create a window surface with 8 bits format (this is actually guaranteed by DisplayMtl::generateConfigs()). Hence in this code, there wasn't any color channels' configured bits size's verification, thus it always chooses MTLPixelFormatBGRA8Unorm metal format for default colorspace:

metalangle/src/libANGLE/renderer/metal/SurfaceMtl.mm

Line 219 in 78017f0

mColorFormat.metalFormat = MTLPixelFormatBGRA8Unorm;

If you add a new 16 bits config with default colorspace, the code should examine the color channels's configured bits size inside egl::SurfaceState parameter and choose appropriate metal format such as MTLPixelFormatBGRA8Unorm or MTLPixelFormatRGBA16Float similar to how depth & stencil bits configuration is handled:

metalangle/src/libANGLE/renderer/metal/SurfaceMtl.mm

Line 228 in 78017f0

depthBits = state.config->depthSize;

metalangle/src/libANGLE/renderer/metal/SurfaceMtl.mm

Line 232 in 78017f0

if (depthBits && stencilBits)

However, I just realized you have no use for RGBA16 with default color space, maybe we can just omit MGLDrawableColorFormatRGBA16 and only keep its BT.2020 variant. Thus further hassles would be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will reduce scope to BT2020 variant only as you suggested.

switch (_drawableColorSpace)
{
case MGLDrawableColorSpaceBT2020PQ:
colorSpace = EGL_GL_COLORSPACE_BT2020_PQ_EXT;
tmm1 marked this conversation as resolved.
Show resolved Hide resolved
break;
default:
colorSpace = EGL_GL_COLORSPACE_LINEAR_KHR;
break;
}
break;
default:
UNREACHABLE();
break;
Expand Down
2 changes: 2 additions & 0 deletions src/libANGLE/Caps.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1222,6 +1222,8 @@ std::vector<std::string> DisplayExtensions::getStrings() const
InsertExtensionString("EGL_EXT_gl_colorspace_display_p3", glColorspaceDisplayP3, &extensionStrings);
InsertExtensionString("EGL_EXT_gl_colorspace_display_p3_linear", glColorspaceDisplayP3Linear, &extensionStrings);
InsertExtensionString("EGL_EXT_gl_colorspace_display_p3_passthrough", glColorspaceDisplayP3Passthrough, &extensionStrings);
InsertExtensionString("EGL_EXT_gl_colorspace_bt2020_linear", glColorspaceBT2020, &extensionStrings);
InsertExtensionString("EGL_EXT_gl_colorspace_bt2020_pq", glColorspaceBT2020PQ, &extensionStrings);
InsertExtensionString("EGL_KHR_gl_texture_2D_image", glTexture2DImage, &extensionStrings);
InsertExtensionString("EGL_KHR_gl_texture_cubemap_image", glTextureCubemapImage, &extensionStrings);
InsertExtensionString("EGL_KHR_gl_texture_3D_image", glTexture3DImage, &extensionStrings);
Expand Down
6 changes: 6 additions & 0 deletions src/libANGLE/Caps.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,6 +961,12 @@ struct DisplayExtensions

// EGL_EXT_gl_colorspace_display_p3_passthrough
bool glColorspaceDisplayP3Passthrough = false;

// EGL_EXT_gl_colorspace_bt2020_linear
bool glColorspaceBT2020 = false;

// EGL_EXT_gl_colorspace_bt2020_pq
bool glColorspaceBT2020PQ = false;
};

struct DeviceExtensions
Expand Down
4 changes: 4 additions & 0 deletions src/libANGLE/renderer/gl/egl/DisplayEGL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,10 @@ void DisplayEGL::generateExtensions(egl::DisplayExtensions *outExtensions) const
mEGL->hasExtension("EGL_EXT_gl_colorspace_scrgb_linear");
outExtensions->glColorspaceDisplayP3Passthrough =
mEGL->hasExtension("EGL_EXT_gl_colorspace_display_p3_passthrough");
outExtensions->glColorspaceBT2020 =
mEGL->hasExtension("EGL_EXT_gl_colorspace_bt2020_linear");
outExtensions->glColorspaceBT2020PQ =
mEGL->hasExtension("EGL_EXT_gl_colorspace_bt2020_pq");
}

outExtensions->imageNativeBuffer = mEGL->hasExtension("EGL_ANDROID_image_native_buffer");
Expand Down
1 change: 1 addition & 0 deletions src/libANGLE/renderer/metal/DisplayMtl.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ bool IsMetalDisplayAvailable()
outExtensions->fenceSync = true;
outExtensions->waitSync = true;
outExtensions->glColorspace = true;
outExtensions->glColorspaceBT2020PQ = true;
}

void DisplayMtl::generateCaps(egl::Caps *outCaps) const {}
Expand Down
8 changes: 8 additions & 0 deletions src/libANGLE/renderer/metal/SurfaceMtl.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,13 @@ void StopFrameCapture()
{
mColorFormat = display->getPixelFormat(angle::FormatID::B8G8R8A8_UNORM_SRGB);
}
else if (attribs.get(EGL_GL_COLORSPACE, EGL_GL_COLORSPACE_LINEAR) == EGL_GL_COLORSPACE_BT2020_PQ_EXT)
{
mColorFormat.intendedFormatId = mColorFormat.actualFormatId =
angle::FormatID::R16G16B16A16_FLOAT;
mColorFormat.metalFormat = MTLPixelFormatRGBA16Float;
mColorFormat.metalColorspace = CGColorSpaceCreateWithName(kCGColorSpaceITUR_2020_PQ);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using mColorFormat to hold the CGColorRef between the SurfaceMtl() and SurfaceMtl::initialize() methods. Should I add another variable in the class instead?

Also here I'm forcing RGBA16 just when the EGL_GL_COLORSPACE is set, which doesn't seem exactly right but I wasn't sure what is better.

}
else
{
// https://developer.apple.com/metal/Metal-Feature-Set-Tables.pdf says that BGRA8Unorm is
Expand Down Expand Up @@ -302,6 +309,7 @@ void StopFrameCapture()

mMetalLayer.get().device = metalDevice;
mMetalLayer.get().pixelFormat = mColorFormat.metalFormat;
mMetalLayer.get().colorspace = mColorFormat.metalColorspace;
mMetalLayer.get().framebufferOnly = NO; // Support blitting and glReadPixels

#if TARGET_OS_OSX || TARGET_OS_MACCATALYST
Expand Down
2 changes: 2 additions & 0 deletions src/libANGLE/renderer/metal/mtl_format_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#define LIBANGLE_RENDERER_METAL_MTL_FORMAT_UTILS_H_

#import <Metal/Metal.h>
#import <CoreGraphics/CoreGraphics.h>

#include <unordered_map>

Expand Down Expand Up @@ -81,6 +82,7 @@ struct Format : public FormatBase
bool needConversion(angle::FormatID srcFormatId) const;

MTLPixelFormat metalFormat = MTLPixelFormatInvalid;
CGColorSpaceRef metalColorspace = nil;
tmm1 marked this conversation as resolved.
Show resolved Hide resolved

LoadFunctionMap textureLoadFunctions = nullptr;
InitializeTextureDataFunction initFunction = nullptr;
Expand Down