Skip to content

TIFFTAG_PREDICTOR should not be applied unless LZW, DEFLATE or ADOBE_DEFLATE#21871

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
xiongzhen:apply-predictor-to-lzw-only
May 13, 2022
Merged

TIFFTAG_PREDICTOR should not be applied unless LZW, DEFLATE or ADOBE_DEFLATE#21871
opencv-pushbot merged 1 commit intoopencv:3.4from
xiongzhen:apply-predictor-to-lzw-only

Conversation

@xiongzhen
Copy link
Copy Markdown
Contributor

@xiongzhen xiongzhen commented Apr 15, 2022

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 18, 2022

@xiongzhen Thanks for the patch. Could you describe your use case and why TIFFTAG_PREDICTOR option should be set for LZW case only?

@xiongzhen xiongzhen changed the title TIFFTAG_PREDICTOR should not be applied if not LZW TIFFTAG_PREDICTOR should not be applied unless LZW, DEFLATE or ADOBE_DEFLATE Apr 18, 2022
@xiongzhen
Copy link
Copy Markdown
Contributor Author

xiongzhen commented Apr 18, 2022 •
edited
Loading

@asmorkalov We used to support packbits when saving a TIF image. We are replacing LEADTOOLS with openCV, finding that it fails to save a packbits tif image in that predictor = PREDICTOR_HORIZONTAL is applicable to just several compression methods (otherwise it throws): LZW, DEFLATE and ADOBE_DEFLATE.

https://www.awaresystems.be/imaging/tiff/tifftags/predictor.html
Theoretically, the predictor step is independent from the compression step, and thus can be combined with any compression scheme. It is, however, likely to be most useful when combined with either LZW or Deflate compression.

I suppose it should not opt-in predictor except for the appropriate methods.

Thanks.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 20, 2022

Thanks for details. I dig a bit in documentation and got your point. Could you add couple of test cases for your case (PACKBITS + LZW and something out of condition) like this:

opencv/modules/imgcodecs/test/test_tiff.cpp

Line 331 in 438fe3f

TEST(Imgcodecs_Tiff, readWrite_32FC3_RAW)
. We will be sure that there are no regressions with TIFF update or other code changes.

@asmorkalov asmorkalov added pr: needs test New functionality requires minimal tests set and removed pr: Discussion Required labels Apr 20, 2022
xiongzhen added a commit to xiongzhen/opencv that referenced this pull request Apr 25, 2022
@xiongzhen
testing case for opencv#21871
c363d73
xiongzhen added a commit to xiongzhen/opencv that referenced this pull request Apr 25, 2022
@xiongzhen
testing case for opencv#21871
1bfa17d
@asmorkalov asmorkalov removed the pr: needs test New functionality requires minimal tests set label Apr 25, 2022
asmorkalov
asmorkalov reviewed Apr 26, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the test!

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 27, 2022

Thanks a lot! The patch looks good to me! Please squash the commits to merge solid patch.

@xiongzhen xiongzhen closed this Apr 28, 2022
@xiongzhen xiongzhen deleted the apply-predictor-to-lzw-only branch April 28, 2022 02:35
@xiongzhen xiongzhen restored the apply-predictor-to-lzw-only branch April 28, 2022 02:38
@xiongzhen xiongzhen reopened this Apr 28, 2022
@asmorkalov asmorkalov self-requested a review May 13, 2022 06:45
@asmorkalov asmorkalov force-pushed the apply-predictor-to-lzw-only branch from 36cf4b3 to 2238ac7 Compare May 13, 2022 06:46
asmorkalov
asmorkalov approved these changes May 13, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented May 13, 2022

@xiongzhen Thanks for the contribution. I squashed commits in your branch to have solid OpenCV commit history after merge.

@opencv-pushbot opencv-pushbot merged commit 612d43f into opencv:3.4 May 13, 2022
@opencv-pushbot opencv-pushbot added this to the 3.4.18 milestone May 13, 2022
@opencv-pushbot opencv-pushbot mentioned this pull request May 14, 2022
Merged
@alalek alalek mentioned this pull request Aug 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov approved these changes

Assignees

@opencv-pushbot opencv-pushbot

Labels

category: imgcodecs

Projects

None yet

Milestone

3.4.18

Development

Successfully merging this pull request may close these issues.

3 participants

@xiongzhen @asmorkalov @opencv-pushbot