-
Notifications
You must be signed in to change notification settings - Fork 58
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
Support raw RGB tiled TIFF #108
Support raw RGB tiled TIFF #108
Conversation
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 looks good, but is there a simple test we can add to ensure it is working as expected? Maybe read a simple raw TIFF generated in PIL via Python?
Thanks @grlee77 for the review! |
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.
Thanks, it looks like the test failures here will be resolved by #110
537c76e
to
3049233
Compare
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.
Thanks Gigon! 😄
Had a couple questions above
unsigned char* raw_buf, | ||
uint64_t offset, | ||
uint64_t size, | ||
uint8_t** dest, |
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 does this need to be **
as opposed to *
?
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.
For the same reason that cudaMalloc accept '**' (https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__MEMORY.html#group__CUDART__MEMORY_1g37d37965bfb4803b6d4e59ff26856356) -- decoders may need to allocate memory and setting the address to dest
if needed.
See
if ((*dest = (unsigned char*)tjAlloc(width * height * tjPixelSize[pixelFormat])) == nullptr) |
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.
Was mostly thinking about this in terms of the nullptr
checks below. We may want to look into using references for this kind of thing in the future, but not urgent. Can be addressed later
case COMPRESSION_NONE: | ||
case COMPRESSION_JPEG: | ||
case COMPRESSION_ADOBE_DEFLATE: | ||
case COMPRESSION_DEFLATE: | ||
return true; | ||
default: | ||
return 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.
Might be worth indenting these to make this a bit more readable
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.
Thanks! Will do.
It turns out that the style is auto-formatted by the current clang-formatter setting (.clang-format) so this is the right style as long as we update clang-format configuration.
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.
Ah ok. Maybe we can track C++ formatting cleanup as an issue
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.
Yeah, will create an issue for that.
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.
created an issue for that: #116
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.
Thanks @jakirkham for the review! Will update it soon.
unsigned char* raw_buf, | ||
uint64_t offset, | ||
uint64_t size, | ||
uint8_t** dest, |
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.
For the same reason that cudaMalloc accept '**' (https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__MEMORY.html#group__CUDART__MEMORY_1g37d37965bfb4803b6d4e59ff26856356) -- decoders may need to allocate memory and setting the address to dest
if needed.
See
if ((*dest = (unsigned char*)tjAlloc(width * height * tjPixelSize[pixelFormat])) == nullptr) |
case COMPRESSION_NONE: | ||
case COMPRESSION_JPEG: | ||
case COMPRESSION_ADOBE_DEFLATE: | ||
case COMPRESSION_DEFLATE: | ||
return true; | ||
default: | ||
return 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.
Thanks! Will do.
It turns out that the style is auto-formatted by the current clang-formatter setting (.clang-format) so this is the right style as long as we update clang-format configuration.
rerun tests (looks like one of the jobs failed due to some sort of Jenkins issue) |
7e81eb4
to
b9df835
Compare
@gpucibot merge |
Due to #17 ([BUG] Compression scheme 33003 tile decoding is not implemented), #19 (Check compression method used in the image) was merged so cuCIM has been handling only particular formats (only jpeg/deflate-compressed image).
If the input TIFF image has no-compressed RAW tile image, cuCIM showed the following error message:
This patch is to support raw RGB tiled TIFF with fast-path, re-allowing non-compressed image with slow-path (in case the input is not tiled RGB image).
This supports Project-MONAI/MONAI#2987.