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

Implement encoding for more types #14

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from

Conversation

LPGhatguy
Copy link
Contributor

@LPGhatguy LPGhatguy commented Sep 6, 2022

This PR is based on #11. This PR enables users to write ktx2 images using the crate without having to manually encode core data structures. It still requires a lot of code even with this PR, so I'd like to follow it up with another PR that includes a higher-level API without sacrificing performance or simplicity.

I extended pseudo_enum to enable configurable integer sizes. I also hid the inner value in favor of exposing a value method as it makes consuming code a bit cleaner. This was necessary because many of these types are narrower than 32 bits.

I made the LENGTH associated constant public on a couple of types and implemented as_bytes for them. For DataFormatDescriptorHeader::as_bytes, I added an argument of the DFD block size because it's not part of the type. I think it maybe should be part of the type.

I changed some members that have invariants about being non-zero from their primitive types to NonZero* types. In addition, I changed the relevant cases doing arithmetic on those values to handle incorrect cases to prevent panics or silent incorrect values.

I put the one existing test into a mod test block as is convention and started adding round-trip tests for encoding and decoding. This helped uncover a couple bugs as I was working on the implementation. I think that this should use property-based testing if we can, but I have no experience with it in Rust.

Here is a GitHub gist containing the code to write a single layer RGBA8 sRGB image using this crate after this PR: https://gist.github.com/LPGhatguy/a42ccf5bed9c7a099d2ee7a169d03c80

This shows that there is still a lot of nontrivial code required to encode a KTX2 image. I am interested in exploring APIs to make this much easier and I think it's important to do that in this crate.

Checklist

  • cargo clippy reports no issues
  • cargo doc reports no issues
  • cargo deny issues have been fixed or added to deny.toml
  • cargo test shows all tests passing
  • human-readable change descriptions added to the changelog under the "Unreleased" heading.
    • If the change does not affect the user (or is a process change), preface the change with "Internal:"
    • Add credit to yourself for each change: Added new functionality @githubname.

Description

Related Issues

src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@LPGhatguy LPGhatguy marked this pull request as ready for review September 7, 2022 01:47
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
pub texel_block_dimensions: [u32; 4], //: 8 x 4;
pub bytes_planes: [u32; 8], //: 8 x 8;
pub flags: DataFormatFlags, //: 8;
pub texel_block_dimensions: [NonZeroU8; 4], //: 8 x 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is NonZeroU8 here useful enough to justify the ergonomic overhead compared to a plain u8?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worthwhile. The value must be non-zero or we can't encode the struct, and I think it's helpful to keep as_bytes non-fallible.

src/lib.rs Outdated Show resolved Hide resolved
pub bit_length: u32, //: 8;
pub channel_type: u32, //: 4;
pub bit_offset: u16, //: 16;
pub bit_length: NonZeroU8, //: 8;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same ergonomic question as above.

src/lib.rs Outdated Show resolved Hide resolved
@Ralith
Copy link
Contributor

Ralith commented Sep 12, 2022

I think that this should use property-based testing if we can, but I have no experience with it in Rust.

I've gotten some good results from some initial attempts to apply proptest to quinn's delay queue.

@LPGhatguy
Copy link
Contributor Author

Review feedback should be addressed.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still ambivalent about the NonZero stuff, but not inclined to block on it.

@LPGhatguy
Copy link
Contributor Author

Hey! Not blocked on this, but anything else I can do to help move this towards merging?

@expenses
Copy link
Contributor

Hi! #21 is going to make encoding the DFDs in combination with this a bit easier. I'm pretty happy with the changes here and it'd make things a lot easier to have these merged before #21.

@Uriopass Uriopass mentioned this pull request Aug 11, 2023
3 tasks
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