🔥 ข่าวด่วน: rapidsai/cucim/pull/

gigony · 2021-11-03T20:05:07Z

Add tif_lzw.c from libtiff as it is
Copy tif_lzw.c file from libtiff for porting lzw decoder.
The code is from the following link:
https://gitlab.com/libtiff/libtiff/-/blob/8546f7ee994eacff0a563918096f16e0a6078fa2/libtiff/tif_lzw.c
, which is after v4.3.0

Support Aperio SVS with CPU LZW and jpeg2k decoder

Update Jpeg decoder to consider colorspace of the JPEG image
- We need to set a proper color space from TIFF metadata to the decoder
- Update tjDecompress2() method call with jpeg_decode_buffer() that modified tjDecompress2() implementation
- Add jpeg_color_space_ to IFD Class.
Move additional includes to their own CMake files
Add Jpeg2k decoder with OpenJpeg
- Added fast color conversion logic with pre-calculated table
Add LZW decoder with libtiff's implementation
- Take part of the code to provide only LZW decoder part
Add rows_per_strip_ and predictor_ attributes to IFD class, to support LZW
Refactor TIFF:resolve_vendor_format() to support Aperio SVS metadata
Support LZW compressed image (with multi strips) for the associated image.

Addresses #17

jakirkham

Thanks Gigon! 😄

Had a few questions below

jakirkham · 2021-11-05T05:25:39Z

    {
-        throw std::runtime_error(
-            fmt::format("This format has more than one image with Subfile Type 0 so cannot be loaded!"));
+        // Aperio SVS format


Is there something else expected in this block? If not, should we consolidate the if/else here into a single if?

Thanks for the feedback!

I expected something but not anymore. Will consolidate it.

jakirkham · 2021-11-05T05:26:39Z

    boolean warning, stopOnWarning;
 };

+enum


Should we name this enum?

The code is directly from libjpeg-turbo's c code and COMPRESS/DECOMPRESS are used in jpeg_decode_buffer() which is modified from tjDecompress2() in libjpeg-turbo.
Since those constants are used only inside libjpeg_turbo.cpp, I would like to keep them as they are.

Ok that's fine. See we already have the license in 3rd party. Should we add a note to this file about this code coming from there?

The information is in lines 18-26 of this file

/** * Code below is derived from the libjpeg-turbo's code which is under three compatible * BSD-style open source licenses * - The IJG (Independent JPEG Group) License * - The Modified (3-clause) BSD License * - The zlib License * Please see LICENSE-3rdparty.md for the detail. * - https://github.com/libjpeg-turbo/libjpeg-turbo/blob/00607ec260efa4cfe10f9b36d6e3d3590ae92d79/tjexample.c * - https://github.com/libjpeg-turbo/libjpeg-turbo/blob/2.0.6/turbojpeg.c#L1241 */

And in LICENSE-3rdparty.md,

libjpeg-turbo - This software is based in part on the work of the Independent JPEG Group. - License: libjpeg-turbo is covered by three compatible BSD-style open source licenses - The IJG (Independent JPEG Group) License - The Modified (3-clause) BSD License - The zlib License - https://github.com/libjpeg-turbo/libjpeg-turbo/blob/master/LICENSE.md - Copyright: - D. R. Commander - Viktor Szathmáry - Files: - cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp : Implementation of jpeg decoder.

Do you think additional information is needed here?

No that seems sufficient

jakirkham · 2021-11-05T05:31:22Z

+    if (flags & TJFLAG_FASTDCT)
+        instance->dinfo.dct_method = JDCT_FASTEST;
+    if (flags & TJFLAG_FASTUPSAMPLE)
+        dinfo->do_fancy_upsampling = FALSE;


We seem to use braces in some places (like above), but not here. Would it make sense to just use braces everywhere?

Same reason for above.

jakirkham · 2021-11-05T05:33:59Z

+int jpeg_decode_buffer(const void* handle,
+                       const unsigned char* jpegBuf,
+                       unsigned long jpegSize,
+                       unsigned char* dstBuf,


Should we do nullptr checks for these below?

This method (jpeg_decode_buffer()) is from the original function tjDecompress2() and did minimal change to make it work

line 249-253 are added to replace GET_DINSTANCE(handle); macro

line 282-286 are added to set color space

For other code that is borrowed from the external library, I would like to keep the source code as it is so that further changes from the original code are trackable.

Instead, added some more comments for explaining that.

jakirkham · 2021-11-05T05:34:15Z

+                       int flags,
+                       int jpegColorSpace)
+{
+    JSAMPROW* row_pointer = NULL;


Should we use nullptr here and below?

Same reason for above.

jakirkham · 2021-11-05T05:35:22Z

+    instance->jerr.warning = FALSE;
+    instance->isInstanceError = FALSE;
+
+    instance->jerr.stopOnWarning = (flags & TJFLAG_STOPONWARNING) ? TRUE : FALSE;


Should we use true & false for these?

Suggested change

instance->jerr.stopOnWarning = (flags & TJFLAG_STOPONWARNING) ? TRUE : FALSE;

instance->jerr.stopOnWarning = (flags & TJFLAG_STOPONWARNING) ? true : false;

Same reason for above.

TRUE/FALSE is defined by build-debug/_deps/deps-libjpeg-turbo-src/jmorecfg.h which is included by <jpeglib.h> in this file.

gigony

Thanks @jakirkham for the review!

The following change is made to address your comments.

diff --git a/cpp/plugins/cucim.kit.cuslide/src/cuslide/cuslide.cpp b/cpp/plugins/cucim.kit.cuslide/src/cuslide/cuslide.cpp
index 4adc1c1..5e7c17d 100644
--- a/cpp/plugins/cucim.kit.cuslide/src/cuslide/cuslide.cpp
+++ b/cpp/plugins/cucim.kit.cuslide/src/cuslide/cuslide.cpp
@@ -103,11 +103,8 @@ static bool CUCIM_ABI parser_parse(CuCIMFileHandle* handle, cucim::io::format::I
     size_t ifd_count = tif->ifd_count();
     size_t level_count = tif->level_count();
 
-    if (tif->ifd(0)->image_description().rfind("Aperio", 0) == 0)
-    {
-        // Aperio SVS format
-    }
-    else
+    // If not Aperio SVS format (== Ordinary Pyramid TIFF image)
+    if (tif->ifd(0)->image_description().rfind("Aperio", 0) != 0)
     {
         std::vector<size_t> main_ifd_list;
         for (size_t i = 0; i < ifd_count; i++)
diff --git a/cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp b/cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp
index c7a1d06..0bf7146 100644
--- a/cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp
+++ b/cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp
@@ -16,13 +16,14 @@
  */
 
 /**
- * Code below is derived from the libjpeg-turbo's example code (tjexample.c) which is under three compatible
+ * Code below is derived from the libjpeg-turbo's code which is under three compatible
  * BSD-style open source licenses
  * - The IJG (Independent JPEG Group) License
  * - The Modified (3-clause) BSD License
  * - The zlib License
  * Please see LICENSE-3rdparty.md for the detail.
- * (https://github.com/libjpeg-turbo/libjpeg-turbo/blob/00607ec260efa4cfe10f9b36d6e3d3590ae92d79/tjexample.c)
+ *  - https://github.com/libjpeg-turbo/libjpeg-turbo/blob/00607ec260efa4cfe10f9b36d6e3d3590ae92d79/tjexample.c
+ *  - https://github.com/libjpeg-turbo/libjpeg-turbo/blob/2.0.6/turbojpeg.c#L1241
  */
 
 #include "libjpeg_turbo.h"
@@ -231,7 +232,9 @@ bool read_jpeg_header_tables(const void* handle, const void* jpeg_buf, unsigned
     return true;
 }
 
-// tjDecompress2
+// The following implementation is borrowed from tjDecompress2() in libjpeg-turbo
+// (https://github.com/libjpeg-turbo/libjpeg-turbo/blob/2.0.6/turbojpeg.c#L1241)
+// to set color space of the input image from TIFF metadata.
 int jpeg_decode_buffer(const void* handle,
                        const unsigned char* jpegBuf,
                        unsigned long jpegSize,
@@ -246,6 +249,7 @@ int jpeg_decode_buffer(const void* handle,
     JSAMPROW* row_pointer = NULL;
     int i, retval = 0, jpegwidth, jpegheight, scaledw, scaledh;
 
+    // Replace `GET_DINSTANCE(handle);` by cuCIM
     tjinstance* instance = (tjinstance*)handle;
     j_decompress_ptr dinfo = NULL;
     dinfo = &instance->dinfo;
@@ -279,7 +283,7 @@ int jpeg_decode_buffer(const void* handle,
     jpeg_mem_src_tj(dinfo, jpegBuf, jpegSize);
     jpeg_read_header(dinfo, TRUE);
 
-    // Modified to set jpeg_color_space for cuCIM
+    // Modified to set jpeg_color_space by cuCIM
     if (jpegColorSpace)
     {
         dinfo->jpeg_color_space = static_cast<J_COLOR_SPACE>(jpegColorSpace);

gigony · 2021-11-09T22:33:49Z

    {
-        throw std::runtime_error(
-            fmt::format("This format has more than one image with Subfile Type 0 so cannot be loaded!"));
+        // Aperio SVS format


Thanks for the feedback!

I expected something but not anymore. Will consolidate it.

gigony · 2021-11-09T22:40:06Z

    boolean warning, stopOnWarning;
 };

+enum


The code is directly from libjpeg-turbo's c code and COMPRESS/DECOMPRESS are used in jpeg_decode_buffer() which is modified from tjDecompress2() in libjpeg-turbo.
Since those constants are used only inside libjpeg_turbo.cpp, I would like to keep them as they are.

gigony · 2021-11-09T23:02:38Z

+int jpeg_decode_buffer(const void* handle,
+                       const unsigned char* jpegBuf,
+                       unsigned long jpegSize,
+                       unsigned char* dstBuf,


This method (jpeg_decode_buffer()) is from the original function tjDecompress2() and did minimal change to make it work

line 249-253 are added to replace GET_DINSTANCE(handle); macro

line 282-286 are added to set color space

For other code that is borrowed from the external library, I would like to keep the source code as it is so that further changes from the original code are trackable.

Instead, added some more comments for explaining that.

gigony · 2021-11-09T23:03:05Z

+                       int flags,
+                       int jpegColorSpace)
+{
+    JSAMPROW* row_pointer = NULL;


Same reason for above.

gigony · 2021-11-09T23:03:55Z

+    instance->jerr.warning = FALSE;
+    instance->isInstanceError = FALSE;
+
+    instance->jerr.stopOnWarning = (flags & TJFLAG_STOPONWARNING) ? TRUE : FALSE;


Same reason for above.

TRUE/FALSE is defined by build-debug/_deps/deps-libjpeg-turbo-src/jmorecfg.h which is included by <jpeglib.h> in this file.

gigony · 2021-11-09T23:04:05Z

+    if (flags & TJFLAG_FASTDCT)
+        instance->dinfo.dct_method = JDCT_FASTEST;
+    if (flags & TJFLAG_FASTUPSAMPLE)
+        dinfo->do_fancy_upsampling = FALSE;


Same reason for above.

Copy tif_lzw.c file from libtiff for porting lzw decoder. The code is from the following link: https://gitlab.com/libtiff/libtiff/-/blob/8546f7ee994eacff0a563918096f16e0a6078fa2/libtiff/tif_lzw.c , which is after v4.3.0

- Update Jpeg decoder to consider colorspace of the JPEG image - We need to set a proper color space from TIFF metadata to the decoder - Update tjDecompress2() method call with jpeg_decode_buffer() that modified tjDecompress2() implementation - Add `jpeg_color_space_` to IFD Class. - Move additional includes to their own cmake files - Add Jpeg2k decoder with OpenJpeg - Added fast color conversion logic with pre-calculated table - Add LZW decoder with libtiff's implementation - Take part of the code to provide only LZW decoder part - Add `rows_per_strip_` and `predictor_` attributers to IFD class, to support LZW - Refactor TIFF:resolve_vendor_format() to support Aperio SVS metadata - Support LZW compressed image (with multi strips) for associated image. Addresses rapidsai#17

jakirkham · 2021-11-20T23:31:23Z

@gpucibot merge

gigony added feature request New feature or request non-breaking Introduces a non-breaking change labels Nov 3, 2021

gigony added this to the v21.10.01 milestone Nov 3, 2021

gigony requested review from jakirkham and quasiben November 3, 2021 20:05

gigony self-assigned this Nov 3, 2021

gigony requested review from a team as code owners November 3, 2021 20:05

gigony force-pushed the support_svs_format branch from 0aed4ed to 65b5ad3 Compare November 3, 2021 20:11

gigony linked an issue Nov 4, 2021 that may be closed by this pull request

[FEA] Support Aperio SVS format (using CPU Decoder) #35

Closed

jakirkham reviewed Nov 5, 2021

View reviewed changes

gigony force-pushed the support_svs_format branch 2 times, most recently from 30f1298 to b383ae9 Compare November 9, 2021 23:08

gigony commented Nov 9, 2021

View reviewed changes

This was referenced Nov 9, 2021

Add NVTX support for performance analysis #144

Merged

Fix thread contention in Cache #145

Merged

jakirkham approved these changes Nov 18, 2021

View reviewed changes

gigony added 2 commits November 20, 2021 11:15

Add tif_lzw.c from libtiff as it is

b39672f

Copy tif_lzw.c file from libtiff for porting lzw decoder. The code is from the following link: https://gitlab.com/libtiff/libtiff/-/blob/8546f7ee994eacff0a563918096f16e0a6078fa2/libtiff/tif_lzw.c , which is after v4.3.0

gigony force-pushed the support_svs_format branch from b383ae9 to 0a9803e Compare November 20, 2021 19:16

rapids-bot Bot merged commit 4eb41c5 into rapidsai:branch-21.12 Nov 20, 2021

	instance->jerr.stopOnWarning = (flags & TJFLAG_STOPONWARNING) ? TRUE : FALSE;
	instance->jerr.stopOnWarning = (flags & TJFLAG_STOPONWARNING) ? true : false;

Conversation

gigony commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakirkham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gigony left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakirkham commented Nov 20, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gigony commented Nov 3, 2021 •

edited

Loading