Skip to content

Color Calibration Algorithm Implementation #2653

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

Color Calibration Algorithm Implementation #2653

wants to merge 8 commits into from

Conversation

@riskiest
Copy link
Contributor

@riskiest riskiest commented Aug 31, 2020 •
edited
Loading 

New PR is submitted: https://github.com/opencv/opencv_contrib/pull/2671. This one is closed.

Color calibration algorithm of OE.30. https://github.com/opencv/opencv/wiki/OE-30.-Color-Calibration
An example of using ColorChecker detector to detect and then color calibration algorithm to correct: https://[email protected]/crystaldust/opencv-mcc-cc-test.git
Algorithm struct and other details: https://github.com/riskiest/color_calibration/tree/v4/doc/pdf/English/Algorithm

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 OpenCV (BSD) 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
force_builders=linux,Docs,Win64,Mac,Win32,android,ios

@riskiest riskiest force-pushed the master branch 3 times, most recently from 375e070 to 85fe73b Compare September 4, 2020 14:02
@garybradski
Copy link
Contributor

garybradski commented Sep 8, 2020

Great that you added an example using the macbeth detector!

vpisarev
vpisarev reviewed Sep 10, 2020
View reviewed changes
modules/mcc/include/opencv2/mcc/ccm.hpp Outdated
{
public:
// detected colors, the referenceand the RGB colorspace for conversion
cv::Mat src;
Copy link
Contributor

@vpisarev vpisarev Sep 10, 2020

Choose a reason for hiding this comment

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

do not use cv::, since this all is declared inside cv namespace

vpisarev
vpisarev reviewed Sep 10, 2020
View reviewed changes
modules/mcc/include/opencv2/mcc/ccm.hpp
double gamma = 2.2, int deg = 3, std::vector<double> saturated_threshold = { 0, 0.98 }, cv::Mat weights_list = Mat(), double weights_coeff = 0,
INITIAL_METHOD_TYPE initial_method_type = LEAST_SQUARE, int max_count_ = 5000, double epsilon_ = 1.e-4) :
src(src_), dst(dst_), cs(cs_), ccm_type(ccm_type_), distance(distance_), max_count(max_count_), epsilon(epsilon_)
{
Copy link
Contributor

@vpisarev vpisarev Sep 10, 2020

Choose a reason for hiding this comment

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

please, move all implementations of the methods into .cpp files

vpisarev
vpisarev reviewed Sep 10, 2020
View reviewed changes
modules/mcc/include/opencv2/mcc/ccm.hpp Outdated
// std::cout << "Loss: " << loss << std::endl;
// }

private:
Copy link
Contributor

@vpisarev vpisarev Sep 10, 2020

Choose a reason for hiding this comment

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

the public classes should not contain private methods and members. Either use abstract class to declare interface with separate "implementation" class (https://github.com/opencv/opencv/wiki/Coding_Style_Guide#high-level-c-interface-algorithms) or use private: class Impl; Ptr<Impl> pImpl; technique (https://en.cppreference.com/w/cpp/language/pimpl)

vpisarev
vpisarev reviewed Sep 10, 2020
View reviewed changes
modules/mcc/include/opencv2/mcc/ccm.hpp Outdated
return out_img;
};

// void report() {
Copy link
Contributor

@vpisarev vpisarev Sep 10, 2020

Choose a reason for hiding this comment

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

remove commented off code from public headers

vpisarev
vpisarev reviewed Sep 10, 2020
View reviewed changes
modules/mcc/include/opencv2/mcc/color.hpp Outdated
*\ param colored mask of colored color
*\ param history storage of historical conversion
*/
cv::Mat colors;
Copy link
Contributor

@vpisarev vpisarev Sep 10, 2020

Choose a reason for hiding this comment

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

the same as above are requested: get rid of cv::, move implementation outside of .hpp files

vpisarev
vpisarev reviewed Sep 10, 2020
View reviewed changes
modules/mcc/include/opencv2/mcc/distance.hpp
RGBL
};

double deltaCIE76(cv::Vec3d lab1, cv::Vec3d lab2);
Copy link
Contributor

@vpisarev vpisarev Sep 10, 2020

Choose a reason for hiding this comment

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

do we really need all this functionality in the public headers?

vpisarev
vpisarev reviewed Sep 10, 2020
View reviewed changes
modules/mcc/include/opencv2/mcc/colorspace.hpp
{
/* *\ brief Basic class for ColorSpace.
*/
class ColorSpace
Copy link
Contributor

@vpisarev vpisarev Sep 10, 2020

Choose a reason for hiding this comment

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

all the exported classes should use CV_EXPORTS macro; look at the other exported from OpenCV classes

@vpisarev
Copy link
Contributor

vpisarev commented Sep 10, 2020

@riskiest, thank you for the contribution! It's a lot of new functionality. Can you provide some unit tests to check that this functionality works correctly?

@vpisarev
Copy link
Contributor

vpisarev commented Sep 11, 2020 •
edited
Loading

@riskiest, also, there is build warning on Windows (see https://pullrequest.opencv.org/#/summary/contrib):

C:\build\precommit-contrib_windows64\opencv_contrib\modules\mcc\samples\color_correction_model.cpp(116):
warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data
[C:\build\precommit-contrib_windows64\build\modules\mcc\example_mcc_color_correction_model.vcxproj]

Please, fix it.

@riskiest
Copy link
Contributor Author

riskiest commented Sep 11, 2020

Thanks for reviewing the code. We are now trying to fix all the problems you mentioned. Some problems is too tricky for us. We will be very grateful if we get help from you.

@vpisarev
Copy link
Contributor

vpisarev commented Sep 11, 2020

Oh, I just noticed that you submitted pull request from your master branch. It's not very good. Please, make a dedicated feature branch, e.g. "colorcalib", put your code there and submit pull request from this branch

@riskiest riskiest mentioned this pull request Sep 17, 2020
Merged
6 tasks
@riskiest
Copy link
Contributor Author

riskiest commented Sep 17, 2020

Changes are made in the new PR following the review, let's talk about it here: #2671.

This one is closed.

@riskiest riskiest closed this Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vpisarev vpisarev vpisarev left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@riskiest @garybradski @vpisarev @JinhengZhang