-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix hdf5 tile data after animation stops #1371
Conversation
Not sure if this should be code reviewed or tested first since it involves copying data, which we try to avoid. |
In this case I think copying is unavoidable -- the compression function should not be modifying data in the tile cache! I do remember adding a tile pool to the tile cache to avoid constantly creating and destroying vector objects, since this was causing quite a significant slowdown. I wonder if a tile pool would be appropriate here as well (I think that we should test the performance impact of this change first). If yes, then we could either create a new tile pool to be used by this function, or add a |
Actually, now I see that the current change is in Perhaps adding a new tile pool in |
@confluence I added TileCache::GetCopy. If I added a TilePool to Frame, wouldn't it have the same problem as the TileCache pool? |
Currently
I'm not sure what problem you mean here. I think I've confused the issue by conflating multiple suggestions -- for clarity, I'm talking about two separate things:
|
@pford I was suggesting to use only a tile pool in the frame for all the tiles, not the tile cache -- we only added the full-resolution tile cache to HDF5 files because in the HDF5 schema the main dataset is chunked, which makes it efficient to read a chunk (which is 2x2 tiles) at a time. For FITS files and other formats, reading a chunk at a time is not efficient, which is why we still use the full channel cache for those formats. I don't think it makes sense to copy tiles read from the full channel cache into the tile cache (this is currently bypassing the tile cache's tile pool, and I believe that downsampled tiles are also being saved -- this cache is only written to handle full-resolution tiles). I am suggesting only that the frame gets its own tile pool object (separate to the tile cache's tile pool), and uses that pool to recycle the tile pointers (instead of creating new pointers with newly allocated data). This wouldn't be caching any tile data; just keeping a buffer of reusable objects allocated in memory. They would be overwritten with new data every time they are used (so it would be fine to use them for all tiles in So the frame would have a So it would be just this small change, plus the copying in the tile cache case. |
I added a TilePool to Frame with the capacity set to the number of omp threads, since Session fills the tiles in parallel using omp. The TilePool is created the first time tiles are requested, since tiles are not used when Frame holds the downsampled image for PV preview. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the place the pool is created, I think this is good now. Using the number of threads to calculate the pool capacity makes sense. Perhaps it should be the number of threads plus one or two, to avoid race conditions in which a new tile is needed before any tiles have been returned to the pool?
|
@kswang1029 in addition to testing the bug fix, please check the performance impact for animation (of both HDF5 files and other file types). I expect this to slightly decrease performance for HDF5 files but also slightly increase performance across the board, and I'd like to find out what the overall impact is (and to make sure that something unexpected isn't happening). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pford this looks good. @confluence As for performance test, please refer to the following table. I test the performance with a custom e2e test which has a 30s animation playback and then I count the stopped channel index and hence an average fps. I do not see obvious performance improvement/degradation.
@kswang1029 @pford in that case I'm happy to merge this. |
the effect (if any) may be more prominent using "large" cube but I do not have the resources to test with, unfortunately. |
Description
What is implemented or fixed? Mention the linked issue(s), if any.
Fixes odd small pixel tiles seen in hdf5 image after animation stops #1368
How does this PR solve the issue? Give a brief summary.
By default, image quality is decreased during animation for better speed. For each channel, tile data for HDF5 images is requested from the tile cache, encoded so that NaN values may be restored, and then compressed. However, the encoding step changes the cached tile data (passed with a shared_ptr) by replacing a NaN values in a block with the average of finite values in that block. When the cached tile data is requested again after animation stops, for image data with a higher compression quality, the image data is distorted since some values are no longer NaN. To fix this, the tile data is assigned to a new vector for encoding and compression, leaving the cached data unchanged.
Are there any companion PRs (frontend, protobuf)?
No
Is there anything else that testers should know (e.g. exactly how to reproduce the issue)?
Zoom in an HDF5 image to an area with NaN values, then start and stop animation. There should be no defect in the image after the animation is stopped.
Checklist
no changelog update neededcorresponding fix added/new e2e test createdprotobuf updated to the latest dev commit/ no protobuf update neededprotobuf version bumped/ protobuf version not bumped