Simplify ImageCache #1551
Merged
Simplify ImageCache #1551
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jleibs
force-pushed
the
jleibs/image_decode_cache
branch
from
8e59ca2
to
60ba461
Compare
jleibs
force-pushed
the
jleibs/simplify_image_cache
branch
from
bd538cd
to
2aed7c1
Compare
jleibs
force-pushed
the
jleibs/simplify_image_cache
branch
from
2aed7c1
to
b339041
Compare
jleibs
force-pushed
the
jleibs/simplify_image_cache
branch
from
b339041
to
3033cd5
Compare
emilk
approved these changes
Mar 13, 2023
| self.key.hash(&mut s); | ||
| let texture_key = std::hash::Hasher::finish(&s); | ||
|
|
||
| let debug_name = format!("tensor {:?}", self.tensor.shape()); |
There was a problem hiding this comment.
it would be nice to use the entity path as the debug name
There was a problem hiding this comment.
Agreed, except we don't currently have easy access to that information and I'm not inclined to plumb it through right now. This is something that would be really nice to generally add to all Component structs.
|
|
||
| // TODO(emilk): support histograms of non-RGB images too | ||
| if let image::DynamicImage::ImageRgb8(rgb_image) = dynamic_img { | ||
| if let image::DynamicImage::ImageRgba8(rgb_image) = dynamic_img { |
There was a problem hiding this comment.
intentional to switch from rgb to rgba? Won't this break histogram for RGB images (e.g. jpegs)?
There was a problem hiding this comment.
The DynamicImage is now derived from the cached RGBA ColorImage. I'd like to re-implement the histogram directly on top of the raw tensor so it always represents raw values, but going to save that for a future PR.
Wumpf
approved these changes
Mar 13, 2023
jleibs
force-pushed
the
jleibs/simplify_image_cache
branch
from
3033cd5
to
1dd2c3d
Compare
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Simplify the ImageCache to only be responsible for converting Tensor -> ColorImage. This includes color-mapping, application of annotation context, and conversion to srgb.
The re_renderer TextureCaching is now moved to the TextureManager.
As the color-mapping / data conversion pieces are moved to the GPU, this cache can ultimately go away and we should be able to just converge on streaming Tensor -> Texture.
This gets rid of usage of DynamicImage everywhere except for the Histogram.
The retained image is still kept around in this cache because moving to re_renderer for all images is a bigger chunk of work.
Checklist