imgcodecs: fix PNG read on BE platforms (s390x)#26915
Conversation
modules/imgcodecs/src/grfmt_png.cpp
Outdated
| 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.
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.
Yes, it needs further investigation - tests on other platforms fail with other PNG files.
modules/imgcodecs/src/grfmt_png.cpp
Outdated
| 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.
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 |
ed33d9a to
7d4b5a5
Compare
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