DDS with implicit first mipmap size fails to import

[LunaticInAHat]

Tested versions

Reproducible in v4.3.dev.custom_build [07677f0f5]

System information

Godot v4.3.dev (07677f0f5) - Manjaro Linux #1 SMP PREEMPT_DYNAMIC Tue Nov 28 20:37:45 UTC 2023 - X11 - Vulkan (Forward+) - dedicated AMD Radeon RX 6700 XT (RADV NAVI22) () - AMD Ryzen 9 5900X 12-Core Processor (24 Threads)

Issue description

Compressed DDS images fail to import, if they do not have the DDSD_LINEARSIZE flag set in their header. This flag is not required to be set in compressed DDS images, per Microsoft's format specification, but Godot's importer aborts loading the images if it is not.

This flag (and the associated dwPitchOrLinearSize) field are not essential to the image-loading process; the sizes of all the mipmaps are already correctly calculated by Godot's loader code. If it simply continued to load the image despite the flag being absent, and the linear size being advertised as 0, I believe it would end up loading the correct data from the DDS.

Steps to reproduce

Open the MRP, observe errors in the console about the image tmp.dds failing to import.

Minimal reproduction project (MRP)

reproducer.zip

[AThousandShips]

So to clarify you do not provide a pitch?

[LunaticInAHat]

Pitch is not meaningful for compressed images (in the context of the DDS header). For compressed image formats, that field is interpreted as being the linear size of the top-level image. Here is the field's description, from the format specification I linked to:

dwPitchOrLinearSize Type: DWORD The pitch or number of bytes per scan line in an uncompressed texture; the total number of bytes in the top level texture for a compressed texture. For information about how to compute the pitch, see the DDS File Layout section of the Programming Guide for DDS.

For compressed images, "pitch" does not have its usual meaning, because the pixel data is not stored in scanlines; it is compressed in blocks of 4x4 pixels. The size of the compressed data belonging to each mipmap is expected to be calculated by the loader, at runtime:

_The D3DX library (for example, D3DX11.lib) and other similar libraries unreliably or inconsistently provide the pitch value in the dwPitchOrLinearSize member of the DDS_HEADER structure. Therefore, when you read and write to DDS files, we recommend that you compute the pitch in one of the following ways..._

Godot's loader appears to calculate the correct size of compressed data for each mipmap, but nevertheless errors out if the flag is not set, or if dwPitchOrLinearSize does not match the size of the base image -- but as you can see from the Microsoft documentation quoted above, the field cannot always be expected to have valid data in it.

[AThousandShips]

Except:

Required when pitch is provided for a compressed texture.

So if pitch is provided the flag also must be used, as per the specification

Godot seems to always expect pitch and for it to match, so by that logic the flag must also be set, as per the specification

[LunaticInAHat]

Right, but pitch isn't being provided. The dwPitchOrLinearSize field has 0 in it. This is a legal thing to do. I didn't create this image; this is one of literally thousands of DDS images that I have found which will not load in Godot, because whichever tool created them declined to populate that field (and, accordingly, left the flag cleared).

[AThousandShips]

Aw I said, Godot expects this to be provided, so the issue isn't the flag but the limited formats supported 🙂

[LunaticInAHat]

If you wanted to phrase it as "Godot only supports a subset of compressed DDS images, and specifically the subset wherein the linear size of the top level image is provided", then sure, that's probably a valid way of viewing the current situation.

But I wasn't able to find any documented "known limitations" for DDS support. The documentation just says:

Godot can import the following image formats:
 - DirectDraw Surface (.dds) - If mipmaps are present in the texture, they will be loaded directly. This can be used to achieve effects using custom mipmaps.

So, from my perspective, this is a bug. I have a DDS image whose author opted to do a thing that the DDS spec allows (omit the linear size value), and Godot can't import it.

I think it should be fairly straightforward to just alter the checks such that dwPitchOrLinearSize == 0 is allowed, if the DDSD_LINEARSIZE flag is not set.

[AThousandShips]

It was more a matter of being specific, because if your file provides a pitch of 0 then the lack of this flag is not what throws the error, but the pitch != size check

I didn't say this wasn't a bug 🙂, but with reports like this specificity is everything, if the error report is:

Compressed DDS images fail to import, if they do not have the DDSD_LINEARSIZE flag set in their header.

But the issue is actually that the files must provide a pitch, then that misses the point and makes it harder to investigate 🙂

Note also that this particular restriction has been around in the format since at least 7 years and no issues have been raised about it before, so it doesn't seem to be an issue with most exporters of DDS textures

[LunaticInAHat]

Fair enough. In that case, to be 100% precise, both checks are going to fail. I'm sure you've already looked, but for completeness of the discussion here, this is what the loader does:

// (modules/dds/texture_loader_dds.cpp, lines 223-225)
uint32_t size = MAX(info.divisor, w) / info.divisor * MAX(info.divisor, h) / info.divisor * info.block_size;
ERR_FAIL_COND_V(size != pitch, Ref<Resource>());
ERR_FAIL_COND_V(!(flags & DDSD_LINEARSIZE), Ref<Resource>());

It correctly calculates the size of the top-level image, then it checks that the dwPitchOrLinearSize field (known here as pitch) matches, then it checks that DDSD_LINEARSIZE is set. Both of those checks will fail, for the images I am trying to work with.

My belief is that the correct logic probably looks more like:

if (flags & DDSD_LINEARSIZE) {
    ERR_FAIL_COND_V(size != pitch, Ref<Resource>());
} else {
    ERR_FAIL_COND_V(pitch != 0, Ref<Resource>());
}

And yes, I agree that this appears to have been a particularity of whatever exporter created these images. I'm not trying to portray this as a "wow, Godot is such junk; it can't even load this DDS!" thing, or trying to play it off like somehow the Godot team has done a half-assed job, or anything like that. The Godot implementation made a fairly reasonable assumption that has worked well up until now. But now we've found a case where its assumption has been violated, and the specification doesn't support the assumption. That means it's a bug, and one I would like to see fixed (and am willing to take a stab at fixing, if there is a consensus behind my interpretation of what the correct behavior ought to be)

[AThousandShips]

Both of those checks will fail

Only one will fail, either the first one will fail and exit, or it will pass and the second may fail or not, that's why the specificity is important 🙂, that's why I asked about if you provided the pitch in the first place, as the original post didn't make this clear

The pitch here does make sense to require, as it prevents invalid input, but it'd be worth it to improve, though again it hasn't been a problem that's been reported before (it was maybe reported once 6 years ago but the report wasn't detailed enough to verify), so it needs to be weighed against the risks

But you're very welcome to attempt to improve it, your suggestion does look good though I'm no authority (though the error should be clearer as it now doesn't make it clear in what way the pitch is incorrect)