Skip to content

Conversation

@lilith
Copy link
Contributor

@lilith lilith commented Dec 29, 2025

Summary

Extra channels (alpha, depth, etc.) can have different bit depths than the main color channels. For example, an image might be 8-bit RGB with a 16-bit alpha channel.

Previously, the render pipeline used the image's global metadata.bit_depth for converting all extra channels from modular to f32. This caused incorrect conversion for extra channels with different bit depths.

This PR:

  • Adds a bit_depth() getter to ExtraChannelInfo
  • Uses each extra channel's own bit depth in ConvertModularToF32Stage

Test plan

🤖 Generated with Claude Code

@google-cla
Copy link

google-cla bot commented Dec 29, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@lilith lilith force-pushed the pr/fix-extra-channel-bitdepth branch 2 times, most recently from 104e206 to 8bf5953 Compare December 29, 2025 21:21
@lilith
Copy link
Contributor Author

lilith commented Dec 29, 2025

⚠️ Test Limitation

No true failing integration test. A proper regression test would require a JXL file where the image bit depth differs from an extra channel bit depth. Without such a test asset, we can only test the components in isolation.

The added tests demonstrate the math of the bug (using wrong scale produces 257x error), but do not exercise the specific buggy line in frame/render.rs end-to-end.


Changes in latest push:

  • Added test_modular_to_f32_bit_depth_matters showing 257x error from wrong scale
  • Added modular_to_f32_8bit_consistency and modular_to_f32_16bit_consistency stage tests
  • Removed Claude co-author attribution

for i in 3..num_channels {
pipeline =
pipeline.add_inout_stage(ConvertModularToF32Stage::new(i, metadata.bit_depth))?;
// Use each extra channel's own bit depth, not the image's metadata bit depth
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this comment.

Extra channels (alpha, depth, etc.) can have different bit depths than
the main color channels. For example, an image might be 8-bit RGB with
a 16-bit alpha channel.

Previously, the render pipeline used the image's global `metadata.bit_depth`
for converting all extra channels from modular to f32. This caused incorrect
conversion for extra channels with different bit depths - values would be
~257x wrong when an 8-bit channel was decoded with 16-bit scale.

This PR:
- Adds a `bit_depth()` getter to `ExtraChannelInfo`
- Uses each extra channel's own bit depth in `ConvertModularToF32Stage`
- Adds tests demonstrating the impact of using wrong bit depth
@lilith lilith force-pushed the pr/fix-extra-channel-bitdepth branch from 8bf5953 to d354042 Compare January 2, 2026 04:56
}

/// Test ConvertModularToF32Stage consistency with different bit depths.
#[test]
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I see the point of all the other tests, except these last two.
Do you mind removing them?

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.

2 participants