Support Aperio SVS with CPU LZW and jpeg2k decoder #141

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

cpp/plugins/cucim.kit.cuslide/src/cuslide/cuslide.cpp

    {
-        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

cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp

@@ -41,6 +45,12 @@ struct my_error_mgr
    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

cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp

+    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

cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp

+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

cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp

+                       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

cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp

+    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

cpp/plugins/cucim.kit.cuslide/src/cuslide/cuslide.cpp

    {
-        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

cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp

@@ -41,6 +45,12 @@ struct my_error_mgr
    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

cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp

+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

cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp

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


Same reason for above.

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

cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp

+    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

cpp/plugins/cucim.kit.cuslide/src/cuslide/jpeg/libjpeg_turbo.cpp

+    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 quasiben and jakirkham 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Aperio SVS with CPU LZW and jpeg2k decoder #141

Support Aperio SVS with CPU LZW and jpeg2k decoder #141

gigony commented Nov 3, 2021 •

edited

Loading

jakirkham left a comment

jakirkham Nov 5, 2021

gigony Nov 9, 2021

jakirkham Nov 5, 2021

gigony Nov 9, 2021

jakirkham Nov 18, 2021

gigony Nov 19, 2021

jakirkham Nov 19, 2021

jakirkham Nov 5, 2021

gigony Nov 9, 2021

jakirkham Nov 5, 2021

gigony Nov 9, 2021

jakirkham Nov 5, 2021

gigony Nov 9, 2021

jakirkham Nov 5, 2021

gigony Nov 9, 2021

gigony left a comment

gigony Nov 9, 2021

gigony Nov 9, 2021

gigony Nov 9, 2021

gigony Nov 9, 2021

gigony Nov 9, 2021

gigony Nov 9, 2021

jakirkham commented Nov 20, 2021

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

Support Aperio SVS with CPU LZW and jpeg2k decoder #141

Support Aperio SVS with CPU LZW and jpeg2k decoder #141

Conversation

gigony commented Nov 3, 2021 • edited Loading

jakirkham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gigony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Nov 20, 2021

gigony commented Nov 3, 2021 •

edited

Loading