Skip to content

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

Merged
merged 8 commits into from
Oct 22, 2021

Conversation

Harvey-Huang
Copy link
Contributor

@Harvey-Huang Harvey-Huang commented Oct 14, 2021

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

  • 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 other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to 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

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.

force_builders=linux,docs

opencv_extra=master

@Harvey-Huang
Copy link
Contributor Author

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.

@Harvey-Huang
Copy link
Contributor Author

I modify the code

@alalek
Copy link
Member

alalek commented Oct 18, 2021

Thank you for contribution!

modules/imgcodecs/test/test_Icon.bmp

Please use lowercase letters in filenames.

This file should go to opencv_extra repository (like any other testdata).
Please move it there (and open corresponding PR).

Please add regression test for this case in modules/imgcodecs/test/.
There is documentation how to run OpenCV tests: https://github.com/opencv/opencv/wiki/QA_in_OpenCV

@alalek
Copy link
Member

alalek commented Oct 19, 2021

Please add test case similar to this.
Put test case after this test under the name TEST(Imgcodecs_Bmp, rgba_bit_mask)


if (!color)
icvCvt_BGRA2Gray_8u_C4C1R(src, 0, data, 0, Size(m_width, 1));
else if (img.channels() == 3)
Copy link
Member

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks!

@Harvey-Huang
Copy link
Contributor Author

I've finished coding, please review, thanks!

@Harvey-Huang
Copy link
Contributor Author

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.

@Harvey-Huang
Copy link
Contributor Author

I have xrgb test file, do I need to pust it to opencv_extra repo and add one more test case?

@alalek
Copy link
Member

alalek commented Oct 21, 2021

add one more test case

Sure!
BTW, no need to put large files. Image with 16x16 pixels is enough.


const Mat img = cv::imread(filenameInput, IMREAD_UNCHANGED);
ASSERT_FALSE(img.empty());
ASSERT_EQ(CV_8UC4, img.type());
Copy link
Member

@alalek alalek Oct 21, 2021

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@Harvey-Huang Harvey-Huang changed the title fixed bug: bmp specified BI_BITFIELDS should take care RGBA bit mask bmp specified BI_BITFIELDS should take care RGBA bit mask Oct 22, 2021
Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you 👍

@alalek alalek merged commit 9267536 into opencv:master Oct 22, 2021
@alalek alalek mentioned this pull request Dec 30, 2021
@alalek alalek mentioned this pull request Feb 22, 2022
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants