Re-use UnpredictImageU8 and implement for 16bpp
#53
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the TIFF loader’s predictor logic by introducing a generic UnpredictImageU8Impl template, centralizing horizontal differencing (predictor 2) into a single function, and extending support to 16-bit samples. Key changes:
- Added
UnpredictImageU8Impl<T>and unifiedUnpredictImageU8to handle both 8 bpp and 16 bpp. - Replaced in-place loops in
DecompressZIPedTileandLoadDNGFromMemorywith calls toUnpredictImageU8. - Removed duplicated prediction code blocks and consolidated failure paths.
Comments suppressed due to low confidence (3)
tiny_dng_loader.h:2865
- [nitpick] The function name
UnpredictImageU8implies 8-bit only, but it now handles 16-bit samples too. Consider renaming (e.g.,UnpredictImage) to accurately reflect its capabilities.
static bool UnpredictImageU8(std::vector<uint8_t>& dst, // inout
tiny_dng_loader.h:2876
- No automated tests cover the 16-bpp predictor path. Please add a unit test for LZW + predictor==2 on 16-bit images to prevent regressions.
} else if (bps == 16) {
tiny_dng_loader.h:2847
- Neither
UnpredictImageU8ImplnorUnpredictImageU8have doc comments explaining parameters, behavior, and supportedbpsvalues. Adding brief function headers would improve readability.
static void UnpredictImageU8Impl(T* dst, // inout
tiny_dng_loader.h
Outdated
| int predictor, const size_t width, | ||
| const size_t rows, const size_t spp) { |
There was a problem hiding this comment.
The parameter predictor is accepted by UnpredictImageU8Impl but never used; consider removing it or asserting its expected value (e.g., predictor == 2) to avoid confusion.
| int predictor, const size_t width, | |
| const size_t rows, const size_t spp) { | |
| const size_t width, const size_t rows, | |
| const size_t spp) { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Good point! Will fix it!
tiny_dng_loader.h
Outdated
| UnpredictImageU8Impl(dst.data(), predictor, width, rows, spp); | ||
| return true; | ||
| } else if (bps == 16) { | ||
| UnpredictImageU8Impl(reinterpret_cast<uint16_t*>(dst.data()), predictor, width, rows, spp); |
There was a problem hiding this comment.
Reinterpreting a vector<uint8_t> buffer as uint16_t* can lead to unaligned accesses on platforms that require 2-byte alignment. Consider copying into a properly aligned vector<uint16_t> or processing bytes directly.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
The codebase already contains similar cases (e.g. in read2), so fixing this I think would not have any benefit, but would impact the performance negatively.
Good point, will fix it. |
We suspect the issue with 16bit tiff files are related to the predictor. In the case where the loader fails, the tiff file uses the LZW compression and predictor==2. It seems that in 16bit mode the predictor still assumes 8bit data, and hence the bug. As a first step we rewrite the code to use the `UnpredictImageU8` function in more places, so we can fix the issue in less places. Then implemented `UnpredictImageU8` for 16bpp images as well.
0903961 to
8258cc0
Compare
We had issues using tinydng to load 16bit TIFF files: the files are loaded, but the data seems to be 8000 garbage.
We suspect the issue with 16bit tiff files are related to the predictor. In the case where the loader fails, the tiff file uses the LZW compression and predictor==2. It seems that in 16bit mode the predictor still assumes 8bit data, and hence the bug.
As a first step we rewrite the code to use the
UnpredictImageU8function in more places, so we can fix the issue in less places.Then implemented
UnpredictImageU8for 16bpp images as well.I'm not 100% happy that I had to use templates, since templates were not used before in the code. If you want, I can just create duplicated functions (i.e. one function for uint8_t case and another for uint16_t case)... or use C macros ;-)
Here is a test image to reprodoce the issue:
test_image_00.tiff.zip
(I patched locally test_loader.cc to dump the pixel values to verify it... and we can see the fix in our own image viewer using tinydng).