Add VK_EXT_ycbcr_3plane_16bit_lsb_formats extension#2709
Conversation
|
@rmader please rebase on github main branch, which should fix the CI CTS framework issue, and address the other issues raised by CI around copyright dates and naming. |
c04f6cb to
8d96571
Compare
Thanks for the review! I think I addressed all issues - and hope the change to append |
15b2a7c to
b73a6f0
Compare
|
I added one more change for completeness - 14bit formats. While those are very uncommon AFAIK, they are supported by ffmpeg. Thus let's include them while on it, so we don't have to revisit this in a couple of years. |
b73a6f0 to
21b0612
Compare
gfxstrand
left a comment
There was a problem hiding this comment.
Sorry this took me nearly a month to get to. Between a Khronos meeting and vacation, it kind of got lost in my e-mail.
| Add the formats in question. For any drivers already supporting YCbCr formats | ||
| via shaders this should be straight forward. Values in the 0.0 - 1.0 range just | ||
| need to get multiplied by 65535.0 / 1023.0 (for 10 bit), 65535.0 / 4095.0 (for | ||
| 12 bit) or 65535.0 / 16383.0 (for 14 bit). |
There was a problem hiding this comment.
This is a tricky assertion to make. Right now, we assume that the X bits are garbage and (mostly) ignored by sampling. I say "mostly" because it's kind of okay if they're not since they're the low bits and any garbage in those bits will show up as noise which likely isn't perceptible to the human eye. With theese formats, however, your assertion that they're easy for shaders by doing a bit of multiplication assumes that the top bits are always zero. While this is fairly easy for software to guarantee when outputting such an image, it's very hard for the driver to defend against. We could make it invalid usage or unknown results if you have non-zero bits in the high bits when sampling but we'd need to be very explicit about that in the API.
There was a problem hiding this comment.
Indeed. So on the DRM side we solved that by using z instead of x: https://github.com/torvalds/linux/blob/master/include/uapi/drm/drm_fourcc.h#L403-L405
We could do the same here by turning
VK_FORMAT_X6G10_X6B10_X6R10_3PLANE_420_UNORM_3PACK16_EXT
into
VK_FORMAT_Z6G10_Z6B10_Z6R10_3PLANE_420_UNORM_3PACK16_EXT
which would hopefully make this clear. WDYT?
There was a problem hiding this comment.
We could do that. Since no one will ever be writing to these formats, it's not like we would need hardware to support that. I'd feel a little better about that than saying it's ignored. Again, I think this is where @fluppeteer will have good opinions. As might @spencer-lunarg since he did the format table in the first place.
There was a problem hiding this comment.
Since no one will ever be writing to these formats
For completeness: these formats are also the native/optimal input formats for common software encoders, meaning there is a chance that at some point we'd want native driver support for writing into shared buffers that can directly be fed into those sw-encoders. Not super likely, just to keep in mind.
|
@fluppeteer should probably also give this a read. |
| @@ -2447,6 +2460,19 @@ ifdef::VK_KHR_maintenance5,VK_BASE_VERSION_1_3[and] | |||
| ename:VK_FORMAT_G16_B16R16_2PLANE_444_UNORM | |||
| **** | |||
There was a problem hiding this comment.
The '****' marks the end of the VU block so needs to be moved after the new VU you are adding immediately below.
There was a problem hiding this comment.
So this required me to move the
.Valid Usage
****
block outside of the ifdef::VK_EXT_ycbcr_2plane_444_formats[] block. There don't seem to be many cases where the VU list could potentially be empty / every item is guarded by an ifdef, however there's one such case here: https://github.com/KhronosGroup/Vulkan-Docs/blob/main/chapters/features.adoc?plain=1#L1893-L1907
So I hope this is ok here as well.
|
I got tired with adding explanatory comments, but there are what appear to be a bunch of minor markup errors in the new VUs which I have suggested fixes for. |
|
(Sorry, accidentally clicked the comment & close button). |
6080136 to
b89c40e
Compare
b89c40e to
a419b1e
Compare
|
Github is borked ATM (presumably because it is a day ending in 'y') and won't let me comment on the diff for some reason, but features.adoc line 1845 introduced some leading white space before the ' ifdef::' which should be removed. |
a419b1e to
146edec
Compare
Whops, fixed. Also should have addressed all other issues from the last CI run (and ran the corresponding scripts locally to validate that) |
This extension adds support for 10/12bit YCbCr formats used by software decoders like ffmpeg, dav1d and libvpx. See https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/34303
146edec to
24c3e2e
Compare
|
Sorry for the noise, just some more minor fixes. |
This extension adds support for 10/12bit YCbCr formats used by software decoders like ffmpeg, dav1d and libvpx.
See also:
This branch as well as the related Mesa and GTK4 MRs have been in draft status for a while and already got a lot of testing. With the recent Gstreamer 1.28 release (shipped in distros like Fedora 44 and Ubuntu 26.04) they can be tested and used - e.g. the default Gnome Video player will make use of the new formats when playing HDR videos (for various codecs, including AV1, HEVC, H266, Pro-Res and DNxHR).
@gfxstrand I'd be super happy if you could have a look at this and, if you find time, help me get this over the line 😅 Most importantly point me to where I should elaborate more.