-
Notifications
You must be signed in to change notification settings - Fork 70
[WIP] PR #3 nvimagecodec v0.7.0 #983
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
base: main
Are you sure you want to change the base?
Conversation
cpp/cmake/deps/nvimgcodec.cmake
Outdated
| if (NOT TARGET deps::nvimgcodec) | ||
| # Option to automatically install nvImageCodec via conda | ||
| option(AUTO_INSTALL_NVIMGCODEC "Automatically install nvImageCodec via conda" ON) | ||
| set(NVIMGCODEC_VERSION "0.6.0" CACHE STRING "nvImageCodec version to install") |
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.
Shouldn't this say "0.7.0"? Maybe not right now, but when 0.7 is released
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.
| - ninja | ||
| - numpy>=1.23.4,<3.0a0 | ||
| - numpydoc>=1.7 | ||
| - nvimgcodec>=0.6.0 |
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.
Shouldn't this say 0.7.0?
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.
| - ipython | ||
| - lazy-loader>=0.4 | ||
| - libnvimgcodec-dev=0.6.0 | ||
| - libnvimgcodec==0.6.0 |
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.
Are these two libs conda specific? I never heard about them (but I also don't use conda)
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.
Yes, those were conda-forge specific packages (libnvimgcodec-dev for headers, libnvimgcodec for runtime library). I've removed them since nvImageCodec 0.7.0 isn't published to conda-forge yet. Based on my understanding,for pip users, the equivalent is nvidia-nvimgcodec-cu12 or nvidia-nvimgcodec-cu13
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.
Yes, you are right about pip. There is also nvidia-nvimgcodec-tegra-cu12 for tegra (but no cuda 13 as there was unification of sbsa and tegra)
| nvimgcodecDecoder_t decoder; | ||
| if (target_is_cpu && manager.has_cpu_decoder()) | ||
| { | ||
| decoder = manager.get_cpu_decoder(); |
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.
cpu decoder doesn't support out of bounds ROI decoding. Maybe you should add checks for this case? Otherwise decode will fail later on
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.
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.
@cdinea check for roi is fine. But changes about destroying codeStream are not. I added a comment here: #983 (comment)
| #ifdef DEBUG | ||
| // Map kind to human-readable name for debugging | ||
| // nvImageCodec 0.7.0 nvimgcodecMetadataKind_t enum values: | ||
| // 0 = UNKNOWN, 1 = TIFF_TAG, 2 = ICC_PROFILE, 3 = EXIF, 4 = GEO |
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.
I think this comment can be removed now, if you use enum values
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.
| &metadata_count | ||
| ); | ||
|
|
||
| if (status != NVIMGCODEC_STATUS_SUCCESS || metadata.buffer_size == 0) |
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.
This is unexpected - maybe you should signal this error to the user?
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.
| else | ||
| { | ||
| // Binary data - store as hex or size info | ||
| tag_value = fmt::format("[{} bytes]", metadata.buffer_size); |
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.
Maybe you should limit size as in SHORT and LONG case?
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.
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.
I don't know, I didn't work that close with tiff. What is the reason to limit metadata in the first place?
| break; | ||
|
|
||
| case NVIMGCODEC_METADATA_VALUE_TYPE_LONG8: | ||
| if (metadata.value_count == 1 && metadata.buffer_size >= sizeof(uint64_t)) |
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.
What if there is more that one LONG8 value?
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.
arrays of LONG8 values are handled in the else if (metadata.value_count > 1) branch . When there are multiple values, they're stored as a std::vector<uint64_t>. I also added buffer size validation (std::min) to ensure we don't read beyond the buffer, matching how SHORT and LONG arrays are handled in this committ
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.
Oh, I must have missed the else case, sorry about that
| break; | ||
|
|
||
| case NVIMGCODEC_METADATA_VALUE_TYPE_RATIONAL: | ||
| if (metadata.buffer_size >= 8) |
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.
Can there be more than 1 rational value?
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.
yes, TIFF tags can have multiple RATIONAL values. I've added array support - when value_count > 1, the values are stored as comma-separated strings (e.g., '1/2, 3/4'). Same fix applied to SRATIONAL as well in thiss commit
| } | ||
| else | ||
| { | ||
| tag_value = fmt::format("[{} bytes, type={}]", metadata.buffer_size, |
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.
Should you limit max size as in other cases?
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.
thank you for the feedback @mkepa-nv - I believe I already addressed this as the default case now limits binary data to 4KB (MAX_BINARY_SIZE = 4096) and logs truncation in DEBUG builds
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.
Yes, but for SHORT array you keep all elements - so why you limit here?
| // NOTE: Sub-code streams obtained via nvimgcodecCodeStreamGetSubCodeStream() are VIEWS | ||
| // into the parent stream and share internal data. They should NOT be explicitly destroyed. | ||
| // The parent stream destruction handles cleanup of all sub-streams. | ||
| // Using nvimgcodecCodeStreamDestroy() on sub-streams causes "free(): invalid pointer". |
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.
This shouldn't happen, maybe there is a bug in some other place? You should be able to call nvimgcodecCodeStreamDestroy on parent or sub code stream in any order and all should be fine. Also each codeStream have some non shared state, so you need to call destroy for each of them. Destroying parent codeStream doesn't clean up sub code stream
I played around with creating and destroying CodeStreams locally and it all worked as expected. Could you investigate further what is causing the issue? And if this is confirmed to be on nvimgcodec side, could you create a minimal reproducer of the issue?
| ~NvImageCodecManager() | ||
| { | ||
| if (cpu_decoder_) | ||
| // Use try-catch to prevent segfaults during static destruction |
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.
@cdinea I don't think this work as you expect. Segfaults are not exceptions, so try catch will not work for them. Are you sure this work?
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.
| // on a code stream whose parent instance is already destroyed, we get | ||
| // "free(): invalid pointer". | ||
| // | ||
| // Solution: Let nvimgcodec handle cleanup. When nvimgcodecInstanceDestroy() is called |
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.
@cdinea This is not true. nvimgcodecInstanceDestroy() doesn't cleanup resources allocated by other objects (like CodeStream or Decoder). Did we write somewhere in docs that it does? Where did you find this info?
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.
thank you @mkepa-nv -that comment was based on an incorrect assumption. I've since learned (through testing and your feedback) that nvimgcodecInstanceDestroy() does not automatically clean up code streams or decoders- this incorrect comment has been removed in this commit,currently we destroy all resources explicitly:
- sub-code streams destroyed in TiffFileParser::~TiffFileParser()
- decoders destroyed in NvImageCodecTiffParserManager::~NvImageCodecTiffParserManager()
- instance destroyed last
| } | ||
|
|
||
| case NVIMGCODEC_METADATA_VALUE_TYPE_SHORT: | ||
| if (metadata.value_count == 1 && metadata.buffer_size >= sizeof(uint16_t)) |
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.
This patter is repeated a lot. I would extract it into function templated on type. Also you don't need to check for buffer_size, count should be enough
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.
| { | ||
| uint16_t val = *reinterpret_cast<const uint16_t*>(buffer.data()); | ||
| tag_value = val; | ||
| value_stored = true; |
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.
What is the definition of TiffTagValue? Maybe you can add std::monostate to it, which would represent empty state
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.
thank you for the suggestion @mkepa-nv .Re:TiffTagValue definition,it's a std::variant that stores typed TIFF tag values. Previously it was:
using TiffTagValue = std::variant<
std::string, // ASCII strings
int8_t, uint8_t, // SBYTE/BYTE
int16_t, uint16_t, // SSHORT/SHORT
int32_t, uint32_t, // SLONG/LONG
int64_t, uint64_t, // SLONG8/LONG8 (BigTIFF)
float, double, // FLOAT/DOUBLE
std::vector<uint8_t>, // Binary/arrays
std::vector<uint16_t>, std::vector<uint32_t>, std::vector<uint64_t>
I've added std::monostate as you suggested in this commit
using TiffTagValue = std::variant<
std::monostate, // Empty/unset state - NEW!
std::string,
// ... rest of types
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.
@cdinea If you added monostate you can get rid of value_stored, here is my idea: #983 (comment)
| // Array of doubles - store as string representation | ||
| const double* vals = reinterpret_cast<const double*>(buffer.data()); | ||
| std::string result; | ||
| for (uint32_t i = 0; i < metadata.value_count && i < 10; ++i) |
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.
I am a bit confused. Why do we store float array as string, while we can store int array of single float value directly?
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.
thank you for the feedback @mkepa-nv - you're right - this was inconsistent. Float and double arrays were being stored as comma-separated strings while integer arrays were properly stored as typed vectorss. I've fixed it to store float/double arrays as std::vector and std::vector instead, matching the behavior of integer arrays. This provides consistent typed storage across all TIFF data types. Fixed in this commit
| { | ||
| // Array of Rationals - store as comma-separated string | ||
| size_t rational_size = 8; // 2 × uint32_t | ||
| size_t count = std::min(static_cast<size_t>(metadata.value_count), |
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.
What about metadata.value_count? Does it contain wrong value?
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.
| { | ||
| // Array of 64-bit values - validate count against buffer size | ||
| const uint64_t* vals = reinterpret_cast<const uint64_t*>(buffer.data()); | ||
| size_t count = std::min(static_cast<size_t>(metadata.value_count), |
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.
Why are you not using metada.value_count?
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.
this has already been addressed in the current version. The LONG8/IFD8 arrays now use the extract_value_array<uint64_t>() template which trusts metadata.value_count directly
| } | ||
| else | ||
| { | ||
| // Store raw bytes - limit size to prevent storing huge blobs |
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.
What if use would need the whole buffer? And right now the limits it somehow arbitrary when it comes to types - e.g. SHORT tags are not limited
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.
| } | ||
|
|
||
| // Store ImageDescription if available from vendor metadata | ||
| if (!ifd_info.image_description.empty()) |
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.
Didn't you remove code that sets it based on vendor metadata in a recent commit?
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.
| try | ||
| { | ||
| // Get compression value from typed variant | ||
| uint16_t compression_value = tiff_tag_value_to_uint16(compression_it->second); |
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.
Compression tag should be SHORT right? So maybe it is better to just check if that is the case and throw exception in the unlikely case that it is not? And we can get rid of this whole tiff_tag_value_to_uint16 function
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.
|
|
||
| // Convert value based on type and store as typed variant | ||
| TiffTagValue tag_value; | ||
| bool value_stored = false; |
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.
value_stored is not needed. The variant will be initialized into first type from its list - so std::monotstate. We can just check at the end if type is still monostate with either:
tag_value.index() == 0std::holds_alternative<std::monotstate>(tag_value))
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.
| uint16_t compression_value = tiff_tag_value_to_uint16(compression_it->second); | ||
| // COMPRESSION tag is always SHORT (uint16_t) per TIFF spec | ||
| // If it's not, that indicates a bug in nvImageCodec or our parsing | ||
| uint16_t compression_value = std::get<uint16_t>(compression_it->second); |
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.
Maybe it would be better to directly check if type is correct via https://en.cppreference.com/w/cpp/utility/variant/holds_alternative.html instead of relying on exceptions? But I don't have strong opinions on that, we can leave catch too
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.
|
/ok to test 2716b26 |
…make Co-authored-by: Kyle Edwards <[email protected]>
Co-authored-by: Kyle Edwards <[email protected]>
Co-authored-by: Kyle Edwards <[email protected]>
7b402de to
43aa310
Compare
Update cuslide2 plugin for nvImageCodec v0.7.0
Description
This PR updates the
cucim.kit.cuslide2plugin to support nvImageCodec v0.7.0 from internal release, which includes new features for direct TIFF tag retrieval and API changes.Key Changes
1. CMake Configuration (
cmake/deps/nvimgcodec.cmake)0.6.0to0.7.0NVIMGCODEC_DIRandNVIMGCODEC_ROOTvariables for local installations2. API Compatibility Fix (
nvimgcodec_decoder.cpp)buffer_sizeassignment fromnvimgcodecImageInfo_tbuffer_sizemember was removed in v0.7.0; buffer size is now inferred fromplane_info3. TIFF Tag Support (
nvimgcodec_tiff_parser.cpp)NVIMGCODEC_METADATA_KIND_TIFF_TAGSUBFILETYPE(254) - for associated image classificationCOMPRESSION(259) - compression method detectionIMAGEDESCRIPTION(270) - vendor metadataSUBIFD(330) - sub-IFD referencesJPEGTABLES(347) - JPEG quantization/huffman tables4. Dependencies (
dependencies.yaml)libnvimgcodecandnvimgcodecversions to0.7.0Related
buffer_sizefromnvimgcodecImageInfo_t