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 ImageEncoder for hdr where color type is Rgba32F #2013

Conversation

johannesvollmer
Copy link
Contributor

@johannesvollmer johannesvollmer commented Sep 21, 2023

  • impl encoder for hdr where rgba32f for now
  • add new output format

this unlocks:

  • ImageBuffer.write_with_encoder, which unlocks DynamicImage.write_with_encoder
  • free_functions::write_buffer_impl, which unlocks all the sweet stuff (DynamicImage.write_to, ImageBuffer.write_to)

@johannesvollmer
Copy link
Contributor Author

johannesvollmer commented Sep 21, 2023

please give me some hints as to what kind of tests I can add to make sure this works as expected :)
(if any)

@johannesvollmer
Copy link
Contributor Author

can we add a reference test that compares images written with HDR to images written with EXR? How does that work in the current test suite?

@fintelia
Copy link
Contributor

fintelia commented Feb 19, 2024

The relevant file is tests/reference_images.rs. I'm not super familiar with how it works, but shouldn't be too hard to figure out from there

@fintelia fintelia changed the base branch from master to next-version-0.25 February 19, 2024 06:43
@fintelia fintelia changed the base branch from next-version-0.25 to master February 19, 2024 06:43
@fintelia
Copy link
Contributor

fintelia commented Apr 6, 2024

If you're interested in reviving this PR, I think it is in good shape. Just needs a rebase and ideally also updates to ImageFormat::{can_write, writing_enabled}

@zhouhang95
Copy link

If you're interested in reviving this PR, I think it is in good shape. Just needs a rebase and ideally also updates to ImageFormat::{can_write, writing_enabled}

Sorry, I'm not good at github workflow. I don't konw how to change this pr.

{
let cp = to_rgbe8(pix);
let cp = flattened_rgbe_pixels.next().unwrap(); // we know it's here because the length is checked earlier
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be included in the chain of zip calls above? Roughly the same as the old version, but passing flattened_rgbe_pixels instead of scanline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be possible, maybe this was done to minimize the diff for an easier review. unfortunately i don't have time at the moment :/

@fintelia
Copy link
Contributor

fintelia commented Apr 7, 2024

@zhouhang95 GitHub only lets the original PR author or a repository maintainer update a PR

@johannesvollmer
Copy link
Contributor Author

johannesvollmer commented Apr 8, 2024

but it should be possible to checkout the branch of this pr and then open a new pr with your own branch :)
branch: https://github.com/johannesvollmer/image/tree/add-hdr-rgb32f-encoder-trait
more info: github checking out pull request locally

@ripytide
Copy link
Member

ripytide commented Jun 2, 2024

This can be closed now as it was merged in #2246

@fintelia
Copy link
Contributor

fintelia commented Jun 2, 2024

Thanks everyone!

@fintelia fintelia closed this Jun 2, 2024
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