-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
bmp specified BI_BITFIELDS should take care RGBA bit mask #20875
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
use cv::imread(pcszFileName, cv::IMREAD_UNCHANGED) function to read the bmp file test_Icon.bmp(I added into "\opencv\modules\imgcodecs\test" folder), you will find the color is wrong, the bitmap data order in test_Icon.bmp is ABGR, but the BmpDecoder::readData method copy it as BGRA. |
I modify the code |
Thank you for contribution!
Please use lowercase letters in filenames. This file should go to opencv_extra repository (like any other testdata). Please add regression test for this case in modules/imgcodecs/test/. |
modules/imgcodecs/src/grfmt_bmp.cpp
Outdated
|
||
if (!color) | ||
icvCvt_BGRA2Gray_8u_C4C1R(src, 0, data, 0, Size(m_width, 1)); | ||
else if (img.channels() == 3) |
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.
indentation in OpenCV is 4 spaces (no tabs)
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.
ok, thanks!
I've finished coding, please review, thanks! |
I found that xrgb bmp file which export by GIMP is specified the BI_BITFIELDS, the alpha channel bit mask in this kind of file is 0, it is means no alpha channel, so need to set alpha as 255. |
I have xrgb test file, do I need to pust it to opencv_extra repo and add one more test case? |
Sure! |
|
||
const Mat img = cv::imread(filenameInput, IMREAD_UNCHANGED); | ||
ASSERT_FALSE(img.empty()); | ||
ASSERT_EQ(CV_8UC4, img.type()); |
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.
Extra checks are necessary here.
These tests should FAIL without changes in modules/imgcodecs/src/grfmt_bmp.cpp
Tests should PASS with changes.
Otherwise we can lost or break code for these cases in the future.
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.
oh! the difference between those two cases is the alpha byte, I updated the test case.
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.
Thank you for update! Currently tests work as expected.
Please restore the .type()
checks.
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.
done!
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.
Thank you 👍
* bmp specified BI_BITFIELDS should take care RGBA bit mask * change the name * support xrgb bmp file * support xrgb bmp file(add test case) * update testing code
Merge with extra: opencv/opencv_extra#925
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.
How to test: use cv::imread(pcszFileName, cv::IMREAD_UNCHANGED) function to read the bmp file test_Icon.bmp(I added into "\opencv\modules\imgcodecs\test" folder), you will find the color is wrong, the bitmap data order in test_Icon.bmp is ABGR, but the BmpDecoder::readData method copy as BGRA.