search

KhronosGroup / KTX-Software

KTX (Khronos Texture) Library and Tools
Other
854 stars 226 forks source link

ktx transcode does not remove existing KTXwriterScParams metadata #851

Closed MarkCallow closed 6 months ago

MarkCallow commented 6 months ago

Existing params have no meaning after transcode. Here is an example after transcoding with no --zlib or --zstd

KTXwriter: ktx transcode v4.0.__default__ / libktx v4.0.__default__
KTXwriterScParams: --uastc-quality 2 --uastc-rdo --zlib 8

Even worse, with --zlib or --zstd it creates an invalid ktx2 file with multiple KTXwriterScParams keys.

KTXwriter: ktx transcode v4.0.__default__ / libktx v4.0.__default__
KTXwriterScParams: --uastc-quality 2 --uastc-rdo --zlib 8
KTXwriterScParams: --zlib 8

Sigh! What a shame I only discovered this right after releasing 4.3.0.

aqnuep commented 6 months ago

I apologize for overlooking this as well. It's not an excuse, but because, unfortunately, filling the KTXwriterScParams was a post-initial-development addition, we just didn't have the right set of input files in the test suite to catch this.

I'll go through this area, create some new set of tests and get this all in order.

MarkCallow commented 6 months ago

I discovered this when working on metadata handling tests for the new deflate tool. The transcode output files are part of a set of input files for the tests. Those input files might be useful more generally. I've pushed a deflate branch to KTX-Software-CTS with the in-progress tests. See input/deflate/metadata. If you think them suitable @aqnuep, please move them to input/metadata for use by multiple tool tests. Please note that the deflate tests are very much a work in progress.

I apologize for overlooking this as well

No need for an apology.

aqnuep commented 6 months ago

In the meantime I've already generated a set of updated input files that will cover the fix, so I don't think we necessarily need anything in addition. I'll post a fix for this later today, once I'm done with testing.

aqnuep commented 6 months ago

I've just posted the CTS (https://github.com/KhronosGroup/KTX-Software-CTS/pull/16) and code (https://github.com/KhronosGroup/KTX-Software/pull/852) PRs.