Skip to content

improvements on tests of imagecodecs module#25525

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
sturkmen72:png-spng-test
May 17, 2024
Merged

improvements on tests of imagecodecs module#25525
asmorkalov merged 1 commit intoopencv:4.xfrom
sturkmen72:png-spng-test

Conversation

@sturkmen72
Copy link
Copy Markdown
Contributor

@sturkmen72 sturkmen72 commented May 2, 2024 •
edited
Loading

[WIP] trivial improvements on tests of imagecodecs module

@sturkmen72 sturkmen72 force-pushed the png-spng-test branch 3 times, most recently from cd6cce4 to f925363 Compare May 3, 2024 15:37
@sturkmen72 sturkmen72 changed the title png spng test improvements on tests of imagecodecs module May 3, 2024
@sturkmen72 sturkmen72 force-pushed the png-spng-test branch 2 times, most recently from 9e41d0b to 8fa2fb7 Compare May 3, 2024 17:43
@sturkmen72
Copy link
Copy Markdown
Contributor Author

sturkmen72 commented May 3, 2024 •
edited
Loading

i was not intending to prepare a PR to merge. but now i thing the proposed changes will be useful.

please review and consider the file changes baboon.ppm and read.png ( let me create a PR to opencv_extra if this PR acceptable)

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented May 6, 2024

@sturkmen72 What is your motivation of png -> ppm replacement?

@asmorkalov asmorkalov self-requested a review May 6, 2024 05:43
@asmorkalov asmorkalov self-assigned this May 6, 2024
@asmorkalov asmorkalov added this to the 4.10.0 milestone May 6, 2024
@sturkmen72
Copy link
Copy Markdown
Contributor Author

sturkmen72 commented May 6, 2024

@sturkmen72 What is your motivation of png -> ppm replacement?

  1. i replaced baboon.ppm with correct one ( older was an unrelated image and probably not used in tests) using this new image in tests will be meaningful.
  2. "#if defined(HAVE_PNG) || defined(HAVE_SPNG)" alredy exist in code and HAVE_PNG can be OFF in very rare cases.
  3. a bit related "IT’S TIME TO RETIRE LENA FROM COMPUTER SCIENCE"

As a final word, the proposed changes are open to discussion. It may be unimportant. Let me do required changes according devs opinions.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented May 6, 2024

@mshabunin @opencv-alalek what is you opinion?

mshabunin
mshabunin reviewed May 6, 2024
View reviewed changes
modules/imgcodecs/test/test_grfmt.cpp
"readwrite/uint16-mono2.dcm",
"readwrite/uint8-rgb.dcm",
#endif
#if defined(HAVE_PNG) || defined(HAVE_SPNG)
Copy link
Copy Markdown
Contributor

@mshabunin mshabunin May 6, 2024

Choose a reason for hiding this comment

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

We have GDAL library integration which should support all these image types (PNG, JPEG, TIFF). Perhaps it should be accounted too, though I didn't try to test GDAL backend specifically (with no other backends).

@sturkmen72 sturkmen72 force-pushed the png-spng-test branch 3 times, most recently from 68f2fad to f3199a0 Compare May 12, 2024 20:14
@vpisarev
Copy link
Copy Markdown
Contributor

vpisarev commented May 14, 2024

I would choose beautiful Lena over less-than-beautiful baboon. Honestly, I don't see any real justification to make this change.

asmorkalov
asmorkalov approved these changes May 17, 2024
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.

👍

@asmorkalov asmorkalov merged commit 5e78878 into opencv:4.x May 17, 2024
@sturkmen72 sturkmen72 deleted the png-spng-test branch May 17, 2024 09:14
@mshabunin mshabunin mentioned this pull request Jun 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mshabunin mshabunin mshabunin left review comments

@asmorkalov asmorkalov asmorkalov approved these changes

Assignees

@asmorkalov asmorkalov

Labels

category: imgcodecs test

Projects

None yet

Milestone

4.10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@sturkmen72 @asmorkalov @vpisarev @mshabunin