| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
imgcodecs: fix PNG read on BE platforms (s390x) #26915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
modules/imgcodecs/src/grfmt_png.cpp
Outdated
| @@ -732,7 +739,7 @@ uint32_t PngDecoder::read_chunk(Chunk& chunk) | |||
| chunk.p.resize(size); | |||
| memcpy(chunk.p.data(), size_id, 8); | |||
| if (readFromStreamOrBuffer(&chunk.p[8], chunk.p.size() - 8)) | |||
| return id; | |||
| return isBigEndian() ? id : *((uint32_t*)(&size_id[4])); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we just made id correct in both cases, why change the return here? wouldn't just return id; now be ok for both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it needs further investigation - tests on other platforms fail with other PNG files.
modules/imgcodecs/src/grfmt_png.cpp
Outdated
| @@ -699,7 +699,14 @@ uint32_t PngDecoder::read_chunk(Chunk& chunk) | |||
| return 0; | |||
| const size_t size = static_cast<size_t>(png_get_uint_32(size_id)) + 12; | |||
|
|
|||
| const uint32_t id = *(uint32_t*)(&size_id[4]); | |||
| uint32_t id = *((uint32_t*)(&size_id[4])); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the proper way is to use:
const uint32_t id = png_get_uint_32(size_d + 4);
|
If I understand correctly the problem has been introduced here: #25715 with introduction of constants |
|
@mshabunin , you are right. Let's use png_get_uint_32 because it has a constant endianness. We just need to have the chunk constants have the same endianness as what this function does, BE cf https://github.com/pnggroup/libpng/blob/b525503b78392335a43558c6a053e0209140762d/pngrutil.c#L63 |
|
I tried and apparently const uint8_t * id_IHDR = "IHDR";
...
if (0 == strncmp(size_id + 4, id_IHDR, 4))
...
Maybe we could even switch to UPD: oh, I think I made a mistake with byte swap and png routine can work |
mshabunin
force-pushed
the
fix-png-be
branch
from
ed33d9a to
7d4b5a5
Compare
mshabunin
force-pushed
the
fix-png-be
branch
from
7d4b5a5 to
c7bbcb0
Compare
|
OK, the last variant should work 😮💨 |
|
Perfect, LGTM ! @asmorkalov , ready to go. |
|
@vrabaud please put formal approve. |
|
@asmorkalov , how do I do that? (we LGTM internally :) ) |
|
thanks a lot, I will test this and see if I can confirm the fix. |
|
Fix looks good in testing here! Thanks. |
|
Oh, er, but...worryingly...it makes the test fail on x86_64, here. |
|
@AdamWill which test fails? Our CI had passed. Did you use the latest variant of the patch? |
|
ah, yikes, probably my bad - I manually rebased the patch to 4.11.0 and missed the second chunk, only had the changes to the const definitions. I'll fix it and retest. |
|
OK yeah, so I instead backported every change to the PNG code since 4.11.0, including this PR, and the test script passes on both x86_64 and s390x. Yay. Thanks. |
Resolves opencv#26913 Related(?): opencv#25715 opencv#26832
Resolves #26913
Related(?): #25715 #26832