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

BGR(A) image format support #7238

Merged
merged 8 commits into from
Aug 23, 2024
Merged

BGR(A) image format support #7238

merged 8 commits into from
Aug 23, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Aug 20, 2024

What

Adds a checklist item that goes through all the different format to make sure they run properly - this looks a bit overzealous, but in fact we are using the fact that wgpu has special support for 8bit bgr images in order to support them on meshes (this detail is tbf untested as of now!), so going through our formats in a test has quite some merit.
(aaalso I think it's the only place where we actually check now that all datatypes still work fine 😳 )

image
("ethnically sourced lena picture", courtesy of Morten Rieger, https://mortenhannemose.github.io/lena/)

Speaking of meshes: the mesh texture support story is now spelled out better, showing warnings and ignoring textures if they're in an unsupported format.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

Copy link

github-actions bot commented Aug 20, 2024

Deployed docs

Commit Link
254638f https://landing-hyhgk0r4g-rerun.vercel.app/docs

@Wumpf Wumpf force-pushed the andreas/bgr-support branch from fff7b02 to 6fc111c Compare August 22, 2024 08:46
@Wumpf Wumpf marked this pull request as ready for review August 22, 2024 09:30
@Wumpf Wumpf added enhancement New feature or request 📺 re_viewer affects re_viewer itself labels Aug 22, 2024
@emilk emilk self-requested a review August 22, 2024 14:30
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

(ColorModel::BGR, ChannelDatatype::U8) => {
(
pad_rgb_to_rgba(&image.buffer, u8::MAX).into(),
TextureFormat::Bgra8Unorm,
Copy link
Member

@emilk emilk Aug 22, 2024

Choose a reason for hiding this comment

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

Is using the Bgra8Unorm texture even a good idea, or is it just adding complexity?

Copy link
Member Author

Choose a reason for hiding this comment

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

from the pov of the all knowing rectangle shader it's just extra complexity, so I should add a comment for the rationale of doing this anyways!

The reason is that this allow us to use BGR(A) on meshes and other occasions where we today don't have fancy decoding etc..
This is very very subtly hinted at by the behavior of required_shader_decode which the mesh visualizer uses to determine if it should warn the user that it can't consume a given image.
(in the glorious 🌈 future 🌈 re_renderer will have an internal conversion pipeline that will allow us to do texture conversions separately and gpu accelerated on the fly for all types. But we live in the now and gotta squeeze out the 80% with what we have 😄)

Copy link
Member Author

Choose a reason for hiding this comment

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

for the code, cleaned up and more formalized :)

            // Make use of wgpu's BGR(A)8 formats.
            //
            // From the pov of our on-the-fly decoding textured rect shader this is just a strange special case
            // given that it already has to deal with other BGR(A) formats.
            //
            // However, we have other places where we don't have the luxury of having a shader that can do the decoding for us.
            // In those cases we'd like to support as many formats as possible without decoding.
            //
            // (in some hopefully not too far future, re_renderer will have an internal conversion pipeline
            // that injects on-the-fly texture conversion from source formats before the consumer of a given texture is run
            // and caches the result alongside with the source data)
            //
            // See also [`required_shader_decode`] which lists this case as a format that does not need to be decoded.

@teh-cmc teh-cmc merged commit 1cd91ff into main Aug 23, 2024
40 checks passed
@teh-cmc teh-cmc deleted the andreas/bgr-support branch August 23, 2024 09:31
emilk added a commit that referenced this pull request Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request include in changelog 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support images in BGR and BGRA
3 participants