Add support for EGL_EXT_gl_colorspace_bt2020_pq #18
Conversation
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
For a new config to be accepted (such as RGBA 16 bits), it must be added to the supported list inside DisplayMtl::generateConfigs()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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:
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.
There was a problem hiding this comment.
Thanks! I will reduce scope to BT2020 variant only as you suggested.
| mColorFormat.intendedFormatId = mColorFormat.actualFormatId = | ||
| angle::FormatID::R16G16B16A16_FLOAT; | ||
| mColorFormat.metalFormat = MTLPixelFormatRGBA16Float; | ||
| mColorFormat.metalColorspace = CGColorSpaceCreateWithName(kCGColorSpaceITUR_2020_PQ); |
There was a problem hiding this comment.
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.
| config.greenSize = 8; | ||
| config.blueSize = 8; | ||
| config.alphaSize = 8; | ||
| config.redSize = size; |
There was a problem hiding this comment.
Based on your explanation I understand now this is not correct. Instead I should advertise only one RGBA16 format, and set allowed depth/stencil to 0 for that format.
What should I use for renderTargetFormat and depthStencilFormat for this new config?
| @@ -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; | |||
There was a problem hiding this comment.
Thanks! I will reduce scope to BT2020 variant only as you suggested.
|
BTW, I have been testing these changes and they basically work but somehow the colors on the screen were not correct compared to the reference AVPlayer. After some investigation I discovered there is a bug in the translation of the fragment shader: (translation comes from GetTranslatedShaderSourceANGLE) Is this bug part of ANGLE or in SPIRV-cross? |
|
That is translated shader source done by ANGLE. If you want to get translated metal shader done by spirv-cross, you can use I don't think the incorrect color is done by shader translation. Why do you think the above fragment shader translation is wrong? I only see the floating point number rounding errors (very small i.e 0.01 to 0.0099999998). The constructor calls are basically the same between original source and translated one. |
|
I'm not sure how is the bt2020 colorspace used to be honest? When you render a video frame to a texture, it is in linear space, isn't it? when the later step renders this texture onto the view layer in BT2020 colorspace, is there any special conversion step needs to be done? |
|
Another way to debug this type of mismatch rendering result is using |
|
My understanding is not perfect, but here is how I believe it works: The video image is in YUV format. Each plane is uploaded into separate 16bit texture. Then a shader is used to convert back to 10bit, combine the textures into one (via color matrix to convert YUV to RGB), and finally apply any color mapping transformations before sending to FBO. In the case of BT2020 PQ, no color map transformation is done (since the display handles it in transfer function, based on PQ equations). My guess was floating point precision issue was causing changes to the luma and chroma planes affecting final colors slightly. But maybe you're right the effect is minimal and there is still some other problem.
|
This is very helpful for debugging, thank you! I will try it tomorrow and see if the metal shader source looks how I expect. |
|
To check quickly, I added a breakpoint in Then I removed these lines from the original GLSL shader, and the colors are still not correct. So you were right that this is a red herring. I also tried MoltenVK with libmpv (using vulkan mode instead of GL), and the colors are exactly the same as with MetalANGLE. So maybe it is an OS issue. I plan to test on more OS/device combinations to narrow down the bug. |
|
I’m curious anw, why multiply by 100 then divide by 100 (multiply by 0.01). Doesn’t it produce the (nearly) exact same old value? Maybe I misunderstood something. |
|
The shader is generated programmatically. In this case there's nothing in-between, because no color mapping is being applied. |
|
I just noticed that |
|
|
This appears not to work correctly on Apple TV due to tvOS bugs. I have filed a report with Apple:
|
|
ok I see, thanks. |
|
It seems to me that it is simply unsupported at this time outside macOS. But given that the first tvOS 14 Beta makes some things render now, I am hopeful they are working on improving support. If you look at the macOS docs, there are several extra methods provided to allow color-correct rendering. See:
Hopefully these will be copied over to tvOS and iOS soon:
|
|
As far as a repro, probably closest available right now is @qiudaomao's MoltenVK example in https://github.com/qiudaomao/MPVColorIssue One thing I found interesting: with the MoltenVK example, it renders colors correctly inside the Apple TV simulator on macOS. But when I tried the same thing with MetalANGLE and this branch, I only got a black screen in the Apple TV simulator. |
WIP. Wired up the EGL_EXT_gl_colorspace_bt2020 extension, but have not tested it yet.
I'm not so sure about the public MGLKit API to expose this. I may decide to use
eglCreateWindowSurfacedirectly in my app and remove theMGLDrawableColorSpace/MGLDrawableColorFormatchanges here.