Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Codecs] BC4 support #1495

Closed
wants to merge 2 commits into from
Closed

Conversation

localthomas
Copy link

@localthomas localthomas commented Jun 15, 2021

This pull request adds support for the BC4 (ATI1) block compression encoding and decoding to the dxt module. Since the BC4 is similar to the BC3 (DXT5), the existing encoding and decoding of BC3 implementation is reused. More information about BC3 and BC4 can be found in Texture Compression Techniques (T. Paltashev and I. Perminov; 2014) and Texture Block Compression in Direct3D 11 (Microsoft).

The output format is a single channel and therefore the color type L8 was chosen. Additionaly, the FourCC code ATI1 that corresponds to this compression in the DDS format is included.

As this is my first pull request for this project, feel free to start a discussion. Some thoughts I had during development:

  • There are no tests in the dxt module. I added mine in dxt, but is there a better spot for this?
  • There is now more code duplication (e.g. decode_dxt1_row, decode_dxt3_row, decode_dxt5_row and decode_bc4_row). Should I refactor parts of the module to reduce such code duplication?
  • I think the BC1 (DXT1), BC2 (DXT3) and BC3 (DXT5) implementations have no tests. Should these be added before any refactoring?
  • The DXTVariant enum is not non-exhaustive. I added BC4 here for now, but there is propably a better solution?
  • I would like to submit BC5 support in the near future as well. Is there anything I should keep in mind?

I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to chose either at their option.

@localthomas
Copy link
Author

Additional information in #1271 and #1272.
If moving the code to a separate crate is desired, I would be keen to help with that.

@fintelia
Copy link
Contributor

Right now our GPU compressed texture handling is really not all that great. For decoding we don't support any of the newer formats like BC6, BC7 or ASTC, And for encoding, our code is slow, low quality, and doesn't support any of the newer formats.

I think it is definitely worth having a conversation about how we should be handling GPU texture formats in image-rs. Splitting this out into its own crate seems like an improvement so it isn't clutter within the main image crate. But also, are you or anyone else interested in porting one of the good GPU encoding libraries (which are all in C/C++ at the moment) or writing/porting decoders for the more modern formats?

@localthomas
Copy link
Author

Splitting this out into its own crate seems like an improvement so it isn't clutter within the main image crate.

Is there any established process for doing that?
I looked at the image-tiff crate and think there are two steps involved (correct me if I'm wrong):

  1. pulling the module into its own crate and only keep the internal encoding and decoding functionality
  2. the current module dxt (or any renamed module) would only implement the ImageDecoder and ImageEncoder traits for the external crate

Would the feature dxt be removed, as its not actually a file format? Only the module dds would require the implementations of step 2?

But also, are you or anyone else interested in porting one of the good GPU encoding libraries (which are all in C/C++ at the moment) or writing/porting decoders for the more modern formats?

Could you give examples of good GPU encoding libraries? The only one I spoted is the GPUOpen Compressonators BCN common kernel. Is there a policy about licensing issues?

I would be willing to take part in the following steps:

  • splitting dxt into a separate crate (i.e. steps from above)
  • adding tests for BC1, BC2, BC3 en/decoding
    • potentially improving the code quality after adding those tests
  • adding BC4 and BC5 support

That means I am hesitant to say wether or not I would provide any more format implementations in the future.

@fintelia
Copy link
Contributor

The steps you list sound good to me.

Would the feature dxt be removed, as its not actually a file format? Only the module dds would require the implementations of step 2?

I'd be in favor of deprecating and eventually removing dxt, once we have an alternative crate we can point people to who still need the functionality (though to be honest I'm not sure anyone is using it). @HeroicKatora what do you think?

@HeroicKatora
Copy link
Member

👍 on deprecation from codecs when a crate is ready but I'd be slightly more cautious on removal. It would be great if we could change the implementation without directly touching the interface. My rationale is that while ImageDecoder is a rather awkward manner of exposing DXT decoding we don't have a mechanism for converting any block-based formats either. I would keep it around until we have a better interface ready that addresses the need directly.

@fintelia
Copy link
Contributor

Actually, looking at crates.io there's already a couple other DXT crates. There's tbc which supports pure Rust encoding (but not decoding) of BC1, BC3, and BC4 using similar algorithms to what we do. And there's squish-rs which is a Rust port of the C++ library with the same name supporting BC1-3 encoding/decoding. It hasn't been updated in a while but implements considerably higher quality algorithms than ours.

@localthomas
Copy link
Author

localthomas commented Jun 19, 2021

And there's squish-rs which is a Rust port of the C++ library with the same name supporting BC1-3 encoding/decoding.

This crate looks quite reasonable (and the last update is not a year old), although there might be issues. It also seems to use RGBA byte buffers only, which is somewhat laborious for BC4s and BC5s one and two channels respectively.

What would be the preferred way? The current implementation pulled into a separate crate or using a third party crate for that job?

For the latter part, I can think of these three points for starting a discussion:

  • Positive: higher quality encoding (and possibly decoding)
  • Positive: less code for maintenance in the image-rs organization
  • Neutral: involves another maintainer
  • Negative: the adaptor in this crate becomes more complex, as the concepts do not transfer directly (could be mitigated in the future)

@jansol
Copy link
Contributor

jansol commented Jun 23, 2021

Squish-rs maintainer here. Support for the other BC variants (including BC6H and BC7) is something I've been meaning to add for a while now. As you can probably see from the commit activity I have also been working on improving the API. I am open to suggestions for the direction of the API design if there are wishes regarding that on the image-rs side.

As for the linked issue, I probably made some small mistake while transcribing the libsquish C++ code, I'll have to go over that and see if I can spot a difference.

On an unrelated note, I'd also like to see KTX support in image-rs. It's a similar format to DDS but with an open specification from Khronos (the organization maintaining OpenGL and Vulkan specifications). There is a ktx crate but it needs some streamlining (using rust enums with repr(u32) instead of plain u32 everywhere) and does not have write support yet, although there appears to be an open PR regarding that.

@jansol
Copy link
Contributor

jansol commented Aug 30, 2021

FWIW I've just pushed BC4 and BC5 support to squish-rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants