KhronosGroup / KTX-Specification

KTX file format source
Other
70 stars 12 forks source link

Tighten Vulkan / GL enums #8

Closed lexaknyazev closed 5 years ago

lexaknyazev commented 6 years ago

The KTX2 headers includes 3 OpenGL-originated fields describing data format (glType, glFormat, and glInternalFormat) and one from Vulkan (vkFormat). The presence of all four of them drastically increases the amount of possible combinations and may lead to various implementation issues unless strictly defined by the KTX2 spec.

Here're the most outstanding issues, as I see them.

vkFormat alone is usually enough to fully describe data

For example: VK_FORMAT_R8G8_UINT

It's not clear what implementations should do if they encounter mismatching GL values with known Vulkan format.

Desktop OpenGL allows in-driver data conversion on texture upload

So glInternalFormat don't have to match other fields to be consumed by OpenGL API. Behavior may vary by vendor and driver version.

Vulkan has a notion of "scaled" formats which has no direct equivalent in OpenGL ES

For example: VK_FORMAT_R8G8_USCALED. There's no valid glFormat / glInternalFormat combination to cover this. Since such formats are not mandatory in Vulkan, they could be disallowed in KTX2.

Vulkan supports sRGB with RED and RG formats

See VK_FORMAT_R8_SRGB and VK_FORMAT_R8G8_SRGB. There's no specific glInternalFormat enums for them in the core OpenGL. Desktop OpenGL may be able to accept GL_SRGB8 if conversion works. For embedded, there're EXT_texture_sRGB_R8 and EXT_texture_sRGB_RG8 extensions. Which of these should KTX2 allow?

BGRA format requires different enums on OpenGL and OpenGL ES

Vulkan has all flavors of it: signed/unsigned/integer/normalized/sRGB. OpenGL supports GL_BGRA for glFormat but there's no corresponding internal format. On the other hand, OpenGL ES may accept GL_BGRA8_EXT as glInternalFormat with APPLE_texture_format_BGRA8888 extension. BGR without alpha has even worse API support.

Some compressed formats from OpenGL don't have matching vkFormat.

In case someone uses some of these formats, vkFormat could be extended with little effort.


With all that said, it seems that we have two options for achieving predictable behavior across different platforms.

  1. List every valid combination of these four fields in the spec and implement all checks in the reference loader. Conforming implementations would need to perform such validation even when not using OpenGL (ES) to keep ecosystem consistent, i.e., refuse to load KTX2 file with invalid OpenGL fields even while running on Vulkan.
  2. Keep only vkFormat, while deriving OpenGL values at runtime (so reference implementation will be almost the same as in option one). Clients running on Metal or Direct3D will be able to completely omit OpenGL-related validation from their codebase since it's much easier to get MTLPixelFormat or DXGI_FORMAT from VkFormat rather than from three OpenGL values.

/cc @MarkCallow @dewilkinson @pjcozzi

MarkCallow commented 6 years ago

A few points in no particular order.

It is not my intent to force loaders to attempt to load any format on any implementation of anything. GL loaders should refuse to load a texture if glInternalFormat is GL_FORMAT_UNKNOWN (or whatever we end up with for this) and Vulkan loaders should refuse to load a texture if vkFormat is VK_FORMAT_UNKNOWN. I have written "should" here. Obvious spec language would be "must not" but I do not want to rule out using KTX files to store multi-sample textures for which there are no formats in either API. So I will most like use "may not".

If both glInternalFormat and vkFormat are set to known values, the values must match otherwise the file is invalid. I don't think there is any language today that states this. I need suggestions for how to specify what "match" means.

For GL loaders glInternalFormat is the important field. glType and glFormat are there to make it easier for loaders. We can add a table like Table 8.2 in OpenGL ES that specifies valid combinations of format, type and internalformat.

Re the Vulkan "scaled" formats most Vulkan implementations do not support using these formats as textures so I have no problem disallowing them in KTX2. There is an issue in the spec. regarding these.

That leaves the BGRA format issue. Note that the values of GL_BGRA_EXT in the apple extension and GL_BGRA in glcorearb.h are the same so from the p.o.v. of KTX2 they're identical. It looks like OpenGL converts those to whatever internalformat is specified. OpenGL ES < 3 converts them to GL_RGBA. I need to look further into what happens on OpenGL ES >= 3. One way to solve this is to have in the valid combination table format = GL_BGRA & internalformat = GL_BGRA8_EXT. Loaders can make best effort to load into versions of OpenGL that don't support GL_RGBA8_EXT and failing with unsupported format is acceptable. Or we could just forbid use of BGRA with OpenGL.

lexaknyazev commented 6 years ago

It is not my intent to force loaders to attempt to load any format on any implementation of anything. GL loaders should refuse to load a texture if glInternalFormat is GL_FORMAT_UNKNOWN (or whatever we end up with for this) and Vulkan loaders should refuse to load a texture if vkFormat is VK_FORMAT_UNKNOWN.

Looking at the current API usage trends (like ANGLE and VkPortability), it seems that the format should be as platform-agnostic as possible yet avoiding edge cases that require runtime conversions.

For GL loaders glInternalFormat is the important field.

Does it mean that KTX2 follows OpenGL ES notion of glInternalFormat?

glType and glFormat are there to make it easier for loaders.

Sorry, I don't follow. Loader will need to validate three fields (glInternalFormat, glFormat, and glType) before pushing them to OpenGL (since we cannot always trust drivers) instead of deriving glFormat and glType from pre-made correct lookup table.

MarkCallow commented 6 years ago

Does it mean that KTX2 follows OpenGL ES notion of glInternalFormat?

Yes it should. We don't want to encourage data conversions so glFormat and glType should match glinternalFormat.

Sorry, I don't follow. Loader will need to validate three fields

Yes you are right. I was thinking too narrowly of just the GL loader. It can pass the fields to OpenGL {,ES} and let the API do the validation. But as you have pointed out, it is not appropriate to rely on OpenGL {,ES} because of the variability between them and across implementations.

I'm coming round to your idea of having just the vkFormat field. I was worried about the size of the switch statement needed to map to equivalent GL formats, which is why I left the GL fields in, but the validation code is probably as large or larger.

To ensure consistency we could add a table to the spec. giving the required mapping, if there is any ambiguity, maybe in an appendix as it will be a long table.

MarkCallow commented 6 years ago

If we drop the GL fields, what should we do about glTypeSize. This gives the size of data type and is used when byte swapping is necessary. Do we keep it, renamed to just typeSize or do we have readers divine it from the value of vkFormat?

Part of me rebels against the idea of thousands of applications having these large tables vs. a handful of KTX2 writers. That is why the GL fields are still there. If we ditch typeSize readers will continue to need a big switch statement to divine the typeSize even if not supporting OpenGL.

jherico commented 6 years ago

I would definitely prefer the option is to have a canonical mapping from a vkFormat to the required gl enums for loading using GL or other legacy APIs vs having multiple fields in. KTX is somewhat notorious for having lots of bad, buggy implementations, and having multiple fields which represent the same information but in different way, and which can become unsychronized with each other is one source of those bugs. Library and tool authors naturally ask themselves why a given piece of information is represented in multiple ways and sometimes come to the conclusion that there is something unexpected they're supposed to do with one of the representations.

If a large switch statement is required to divine all the appropriate GL enums required, that's because of the failings of the GL API, and shouldn't be propagated to KTX2.

MarkCallow commented 6 years ago

@jherico, thanks for the comment. Even readers not supporting OpenGL will need a large switch statement or table to map from a VkFormat enum to the size of its type, if we remove typeSize from KTX2.

For readers supporting GL loading, the same table with added data can be used to map vkFormat to the proper GL format, internalformat and type. The size of the table is purely down to the number of formats in Vulkan.

lexaknyazev commented 6 years ago

glTypeSize description is a bit confusing to me. There're several types of texture formats:

If endianness conversion is expected to affect only the first group, the "switch case" with vkFormat is very small because VkFormat enum values are pre-sorted by type.

Size 2:

Size 4:

Size 8:

All ranges are inclusive.

Also, the spec should have some language about whether depth/stencil formats are supported (this may affect ranges above).

lexaknyazev commented 6 years ago

First-glance implementation seems to be quite compact (definitely not the biggest part of loader):

int getTypeSizeFromVkFormat(int vkFormat) {
    if (vkFormat >= 70 && vkFormat <= 96 && 
        vkFormat != 76 && vkFormat != 83 && vkFormat != 90) {
        return 2;
    }

    if (vkFormat >= 98 && vkFormat <= 108 && 
        vkFormat != 100 && vkFormat != 103 && vkFormat != 106) {
        return 4;
    }

    if (vkFormat >= 110 && vkFormat <= 120 && 
        vkFormat != 112 && vkFormat != 115 && vkFormat != 118) {
        return 8;
    }

    return 1;
}
MarkCallow commented 6 years ago

There're several types of texture formats:

  • integers of 8/16/32/64 bit per channel (I guess these correspond to 1/2/4/8)

Yes.

  • floats of 16/32/64 bit per channel (should glTypeSize be 1?)

No. Floating point values typically need endianness conversion as well so glTypesize should be set accordingly to 2, 4 or 8.

  • packed formats like 565 or 5551, i.e., 8/16/32 bits per pixel (should glTypeSize be 1?)

No. glTypesize is set according to the size of the type the components are packed in to. For 565 & 5551 that is a uint16_t so typesize is 2.

  • block-compressed formats with 64 or 128 bits per block (the spec says that glTypeSize should be 1)

These are not byte oriented so no endiannness conversion is needed. glTypeSize is set to 1 to indicate that.

  • recently added video-plane formats (some of them are packed, some are aligned)

See above re packed. Not sure what you mean by "aligned". Probably we need to consult Andrew on these.

Please make suggestions how I can improve the explanation of glTypeSizein KTX 1 to avoid he confusion you felt. If we remove glTypeSize from KTX2 we will still need a section on endianness conversion that explains what readers must do in the event of a mismatch with the writer.

lexaknyazev commented 6 years ago

Thanks for the clarification re float and packed formats.

Formats added in Vulkan 1.1 are quite unique and may need some additional treatment re planes layout from KTX (unless we want to disallow them). A few examples from the Vulkan 1.1 spec (there're 34 of them with different layouts):

image

image


Updated code that covers all Vulkan 1.0 formats (integers, floats, packed, compressed, depth, stencil)

int getTypeSizeFromVkFormat(int vkFormat) {
    if ((vkFormat >= 2 && vkFormat <= 8) || // pack 16
        (vkFormat >= 70 && vkFormat <= 97) || // regular 16
        vkFormat == 124) { // D16_UNORM
        return 2;
    }

    if (vkFormat == 128) return 3; // D16_UNORM_S8_UINT

    if ((vkFormat >= 51 && vkFormat <= 69) || // pack 32
        (vkFormat >= 98 && vkFormat <= 109) || // regular 32
        vkFormat == 122 || // B10G11R11_UFLOAT_PACK32
        vkFormat == 123 || // E5B9G9R9_UFLOAT_PACK32
        vkFormat == 125 || // X8_D24_UNORM_PACK32
        vkFormat == 126 || // D32_SFLOAT
        vkFormat == 129) { // D24_UNORM_S8_UINT
        return 4;
    }

    if (vkFormat == 130) return 5; // D32_SFLOAT_S8_UINT

    if (vkFormat >= 110 && vkFormat <= 121) { // regular 64
        return 8;
    }

    return 1;
}
jherico commented 6 years ago

@lexaknyazev You need to account for edge conditions and formats added by extensions. Presumably that would be easiest to do by changing return 1 to if (vkFormat > VK_FORMAT_UNDEFINED && vkFormat <= VK_FORMAT_ASTC_12x12_SRGB_BLOCK) { return 1; } else { return -1; }. That way a subsequent extension that adds a new format doesn't automatically get 1 as a type size.

lexaknyazev commented 6 years ago

Sure. I'm just evaluating how much code is needed. Updated snippet with your suggestion and my first-impression understanding (which might be incorrect) of new Vulkan 1.1 formats (some range checks may be optimized by using bitmasks, let's focus on high-level question first):

int getTypeSizeFromVkFormat(int vkFormat) {
    if ((vkFormat >= 2 && vkFormat <= 8) || // pack 16
        (vkFormat >= 70 && vkFormat <= 97) || // regular 16
        vkFormat == 124 || // D16_UNORM
        vkFormat == 1000156007 || // R10X6_UNORM_PACK16
        vkFormat == 1000156017 || // R12X4_UNORM_PACK16
        (vkFormat >= 1000156027 && vkFormat <= 1000156033)) { // multi-planar & 2x1 16 bpc
        return 2;
    }

    if (vkFormat == 128) return 3; // D16_UNORM_S8_UINT

    if ((vkFormat >= 51 && vkFormat <= 69) || // pack 32
        (vkFormat >= 98 && vkFormat <= 109) || // regular 32
        vkFormat == 122 || // B10G11R11_UFLOAT_PACK32
        vkFormat == 123 || // E5B9G9R9_UFLOAT_PACK32
        vkFormat == 125 || // X8_D24_UNORM_PACK32
        vkFormat == 126 || // D32_SFLOAT
        vkFormat == 129 || // D24_UNORM_S8_UINT
        vkFormat == 1000156000 || // G8B8G8R8_422_UNORM
        vkFormat == 1000156001 || // B8G8R8G8_422_UNORM
        vkFormat == 1000156008 || // R10X6G10X6_UNORM_2PACK16
        vkFormat == 1000156018) { // R12X4G12X4_UNORM_2PACK16
        return 4;
    }

    if (vkFormat == 130) return 5; // D32_SFLOAT_S8_UINT

    if ((vkFormat >= 1000156012 && vkFormat <= 1000156016) || // multi-planar 10 bpc
        (vkFormat >= 1000156022 && vkFormat <= 1000156026)) { // multi-planar 12 bpc
        return 6;
    }

    if ((vkFormat >= 110 && vkFormat <= 121) || // regular 64
        (vkFormat >= 1000156009 && vkFormat <= 1000156011) || // RGBA10 & 2x1 10 bpc
        (vkFormat >= 1000156019 && vkFormat <= 1000156021)) { // RGBA12 & 2x1 12 bpc
        return 8;
    }

    if ((vkFormat > 0 && vkFormat <= 184) || // all other known core formats
        (vkFormat >= 1000054000 && vkFormat <= 1000054007) || // PVRTC
        (vkFormat >= 1000156002 && vkFormat <= 1000156006)) { // multi-planar 8 bpc
        return 1;
    }

    return -1;
}

This is arguably more complex than the initial version above, so I'm interested what do people think re deriving typeSize from vkFormat on load vs reading both values from the file. Part of me screams "never trust the input" so having two fields won't keep KTX2 loader from validation which would require roughly the same procedure as deriving typeSize at runtime.

MarkCallow commented 6 years ago

Thanks for the code @lexaknyazev. Is it possible to use the enum names rather than values?

For the 2 example Vulkan 1.1 formats you added, for G8B8G8R8_422_UNORM the spec states it is 32-bit so typesize would be 4, as you have in your code. G8_B8_R8 is just bytes so typesize would be 1.

One reason for preferring having typesize in the file is because it is more future-proof. A Vulkan loader for KTX does not need to be aware of newly added types. It simply does the endian conversion indicated by typesize then uses the VkFormat value for the upload without needing any awareness of what it actually means.

MarkCallow commented 6 years ago

We can probably get the type size from the data format descriptor. I'll check with Andrew.

lexaknyazev commented 6 years ago

Is it possible to use the enum names rather than values?

Sure. I used values just to showcase ranges and spare some screen space. An actual program would likely use names.

We can probably get the type size from the data format descriptor.

Isn't DFD optional?

Future-proof and input-proof implementation would be like:

// read vkFormat and typeSize from file

int knownTypeSize = getTypeSizeFromVkFormat(vkFormat);

if (typeSize == knownTypeSize) {
  // OK
} else if (knownTypeSize == -1) {
  // assume file is correct, use typeSize
} else {
  // file is surely invalid
}
MarkCallow commented 6 years ago

DFD is not optional.

I have just published a new draft spec with the status reverted to not ready for implementation while we resolve this issue.

MarkCallow commented 6 years ago

Getting the typesize from a DFD is not straightforward because a DFD has no concept of primitive types. So we'll either need to keep the field or provide example code like we've been discussing above.

My asking Andrew about this has led to a lengthy discussion between us about endianness support and making vkFormat and the DFD play well together. I still do not have a clear idea how to resolve these issues. I will open other issues about these topics.

MarkCallow commented 6 years ago

There are 2 things holding up resolution of this issue:

  1. I want KTX2 to be usable with Vulkan and OpenGL formats that may not exist yet, i.e. may be introduced by extensions. Having both gl and vkFormat values enables this. Removing the gl values means a mapping to GL is required in the loader and that mapping will be unaware of newly introduced Vulkan formats. I do not yet see an obvious way to strictly define combinations while allowing use of new formats from extensions.

  2. We need to resolve whether to keep a typesize field. This is related to no. 1 above, as it is needed to avoid hardwired mappings, and to Issue #14. If we figure out a resolution to 1, I can of course keep the typesize field until #14 is also resolved.

lexaknyazev commented 6 years ago

On (1).

So far, the only formats missing from Vulkan are palette and legacy compressed formats (ASTC 3D will obviously be in Vulkan). I doubt that we will ever see new GPU formats introduced exclusively in OpenGL future versions or extensions.

And even if that's the case, depending on the underlying platform, an application may still need to know about such extension's existence to properly create and initialize GL context; so some kind of in-app hard-coded switch-case seems to be inevitable for today-unknown formats.

Also, values of row_of_blocks, block_of_pixels, and format-specific-number-of-bytes are format-dependent, so a generic KTX loader may be unable to unpack unknown format.

MarkCallow commented 6 years ago

As discussed in today's TSG meeting, we we will fix this by

  1. Removing the gl header fields, except glTypeSize which will be handled as part of issue #14.
  2. Specifying that vkFormat can take any of the values from the VkFormat enumeration in the Vulkan 1.1 spec, except the SCALED types and the PACK32 types. It can also take any values added to that enum by extensions.
  3. Add metadata, ktxRequiredVkExtension (and ktxRequiredGLExtension if there is a GL equivalent) which will give the names of extensions required when loading a format added by an extension.
  4. Adding a table specifying mappings from VkFormats to GL internalformat, format & type and, as a bonus, DX formats.

@lexaknyazev to draft the table referred to in no. 4.

lexaknyazev commented 6 years ago

@MarkCallow Why PACK32 types are disallowed? Some of them are widely supported (like E5B9G9R9).

MarkCallow commented 6 years ago

It is not all PACK32 types, sorry. My shorthand was a bit too brief. Here's the explanation I've written in the spec. draft I'm working on.

The A8B8G8R8*PACK32 formats are prohibited because the end result is the same regardless of whether the data is treated as packed into 32-bits or as the equivalent R8G8B8A8 format, i.e. as an array of 4 bytes, and because the Data Format Descriptor cannot distinguish between these cases.

lexaknyazev commented 6 years ago

re ktxRequiredGLExtension:

What GL version will be considered as "baseline"? The specs make many assumptions regarding compressed formats that may or may not be true on a given platform. Since we decided to not include GL enums, I doubt that there's much value in including GL extensions names.

lexaknyazev commented 5 years ago

Here's the first draft of a full list of Vulkan formats with their OpenGL, Metal, and DX counterparts (excluding SCALED and A8B8G8R8*PACK32 variants).

Keep in mind that different APIs might have different conventions on naming enums, so some matching formats may appear to a reader as if they have different channels order. For the same reason, some packed formats that do not have corresponding native VkFormat value are not in the list.

A few random notes:

TODO:

MarkCallow commented 5 years ago

Great. Thanks. I haven't attempted to check it for correctness. It would be great if we could put this data in some form that it can be used to generate the table for the spec. and a switch statement or other code for run time mapping. Then we'd only need to update one place.

Keep in mind that different APIs might have different conventions

The spec needs to include this paragraph and several of the random notes.

Regarding OpenGL being too flexible, OpenGL ES 3.2 has table 8.2 which lists valid combinations of format, internalformat and type. The mapping should be based on that. If we were retaining the gl* fields we could have referenced that table to limit the combinations.

Yes we should raise the ASTC 3D format enums with the Vulkan WG. That should be on the TODO list.

Do we need to explicitly support alpha-only and luminance formats, i.e, define the necessary swizzles?

lexaknyazev commented 5 years ago

Do we need to explicitly support alpha-only and luminance formats, i.e, define the necessary swizzles?

Mapping is straightforward:

Alpha8 VkFormat: VK_FORMAT_R8_UNORM KTXswizzle: 000r

Luminance8 VkFormat: VK_FORMAT_R8_UNORM KTXswizzle: rrr1

Luminance8Alpha8 VkFormat: VK_FORMAT_R8G8_UNORM KTXswizzle: rrrg

Clients may opt to detect the first case and use API-provided enums like MTLPixelFormatA8Unorm or DXGI_FORMAT_A8_UNORM.

MarkCallow commented 5 years ago

Okay. Let's do it.

MarkCallow commented 5 years ago

KTXswizzle: rrra

Is this one right? Shouldn't it be "rrrg"?

lexaknyazev commented 5 years ago

Oops, you're right. Fixed the comment above.

I'll add these definitions to the KTX spec. The table won't change, though.

MarkCallow commented 5 years ago

I'm working on this. As I write the description of KTXrequiredVulkanExtension the following question has arisen. What do we say about formats which are added in new core versions of Vulkan without there having been extensions? As far as I can tell there were no extensions for any of the formats added in Vulkan 1.1. Do we need a KTXrequiredVulkanVersion as well?

lexaknyazev commented 5 years ago

What would be expected use cases for these fields? Unlike OpenGL (that has all enums in global scope), all Vulkan formats exist within VkFormat scope, so values unknown to an application will not cause any interference. Do we expect that apps will be able to "blindly" enable extensions based on their names in a KTX file?

So far, only one image format extension exists - VK_IMG_format_pvrtc. I think it's safe to assume that apps running on PowerVR GPUs will have it enabled anyway.

MarkCallow commented 5 years ago

I can't find any actual text for VK_IMG_format_pvrtc. If it is the case that format extensions simply add formats to VkFormat and apps can use the standard query to check if the format is supported then this metadata is unnecessary.

I was under the impression that all Vulkan extensions had to be enabled before use. The metadata was to provide the name of the extension. Apps would not "blindly" enable a listed extension. They would check if it exists in the implementation and if so enable it. If not the should raise an appropriate error such as "unsupported format".

MarkCallow commented 5 years ago

There is also a possibility that an extension may add to the textureCompression* items in VkPhysicalDeviceFeatures. As for the existing items (textureCompressionETC2, textureCompressionBC and textureCompressionASTC_LDR, should we add metadata to indicate if the format needs one of these features to be enabled?

MarkCallow commented 5 years ago

Several hours after writing the above comments I realized that extensions and device features have to be enabled at device creation, i.e. long before any texture file is read. I expect that is what @lexaknyazev was referring to by "blindly enable extensions".

The metadata is therefore probably not useful. How about a recommendation that apps handling unknown textures should enable all texture formats available in the implementation at device creation and use vkGetPhysicalDeviceImageFormatProperties to determine if the format is supported at time of loading a texture.

lexaknyazev commented 5 years ago

See https://github.com/KhronosGroup/Vulkan-Docs/issues/512 re VK_IMG_format_pvrtc.

+1 for apps querying runtime about format support. Putting pipeline tools aside, what are "apps handling unknown textures"?

MarkCallow commented 5 years ago

Thanks for the pointer to your Vulkan-Docs issue. Is the information, given in the answer there, in your mapping table in Google Docs?

An example of "apps handling unknown textures" is my test program for the Vulkan loader portion of libktx. I enable all compressed texture features available in the implementation. Now that there are extensions that add formats I will also have to enable those extensions. Elsewhere in the program I have a list of texture tests with many duplicated using different texture formats. The program loads and runs those using formats that runtime queries show are supported. The tests run on iOS, linux, macOS & Windows.

Enabling all available features up front has 2 advantages: it avoids a rats nest of platform ifdefs in the code enabling formats and in the test list and I can add new tests to the list without worrying about whether its format has been enabled.

MarkCallow commented 5 years ago

Please review the PR including the latest commit to it about handling ASTC HDR & 3D in the absence of Vulkan extensions.

MarkCallow commented 5 years ago

I've added some more commits to the PR linked above. The change is now complete except for the table mapping Vulkan formats to other API formats. Please review. I want to merge this so we can make progress on the other issues.

You can also review the data from which the mapping table will be completed

MarkCallow commented 5 years ago

Closed by PR #40. Don't know why it didn't happen automatically.