opencv / opencv_contrib

Repository for OpenCV's extra modules
Apache License 2.0
9.43k stars 5.76k forks source link

BRISQUE: empty don't works as expected #2548

Open Nuzhny007 opened 4 years ago

Nuzhny007 commented 4 years ago
System information (version)

When I create QualityBRISQUE with wrong path to the range file the QualityBRISQUE::create returns a valid pointer. Ho can I check that all data was loaded correctly? I call QualityBase::empty - but it always returns true: https://github.com/opencv/opencv_contrib/blob/master/modules/quality/include/opencv2/quality/qualitybase.hpp#L49

May be it can add overloaded method empty to the QualityBRISQUE? And if model or range was not loaded correctly than it will return true: CV_WRAP bool empty() const CV_OVERRIDE { return _range.empty() || _model.empty(); }

alalek commented 4 years ago

/cc @clunietp

clunietp commented 4 years ago

Thanks @Nuzhny007 - IIRC the constructor would throw an exception if the range file was empty, but apparently either I am mistaken or that behavior has changed. Regardless, I agree there should be a fix. Are you still going to pursue your PR?

My original intent was to throw an exception on invalid files (thus never leaving the brisque obj in an invalid state), but I can see no harm in implementing empty() as well

Nuzhny007 commented 4 years ago

Now I can set the wrong path to the range file and it will be exception in compute(). I was about to make PR, but the tests failed. Can you see what is wrong with them?

clunietp commented 4 years ago

When you fork, you need to create a new branch as well in your repo. You need to create a new branch in your repo, and then initiate a PR to opencv:master from that. Make sense?

Also, in your empty() method, you may also want to check for _model != nullptr (or similar) prior to checking _model.empty(). Eg: return _range.empty() || !_model || _model.empty();

Nuzhny007 commented 4 years ago

Ok: https://github.com/opencv/opencv_contrib/pull/2565