-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Adding dnn based super resolution module. #2229
Conversation
Thank you for the contribution! Please ask your mentor to take a look and approve PR changes. |
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.
Links from CI:
|
||
#ifdef _WIN32 | ||
#include <Windows.h> | ||
#endif |
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.
Do we really need platform specific include in public includes?
#include <opencv2/highgui.hpp>
...
A lot of these headers are not needed to define this API.
Move these includes into corresponding .cpp files.
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.
Corrected. I also don't need platform specific include anymore, and eliminated it.
namespace dnn_superres | ||
{ | ||
|
||
int DnnSuperResImpl::layer_loaded = 0; |
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.
Please avoid unnecessary indentation in namespaces.
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, I reformatted the code.
|
||
#ifdef _WIN32 | ||
QueryPerformanceFrequency(&fq); | ||
QueryPerformanceCounter(&st); |
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.
Use int64 getTickCount();
or TickMeter
helper class instead:
https://github.com/opencv/opencv/blob/4.1.1/modules/core/include/opencv2/core/utility.hpp#L285-L332
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.
Changed it to TickMeter.
fontColor = fontcolor; | ||
} | ||
|
||
double DnnSuperResQuality::psnr(Mat img, Mat orig) |
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.
Please use PSNR()
function from core module instead
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.
Replaced it with core module function.
} | ||
|
||
double DnnSuperResQuality::ssim(Mat img, Mat orig) | ||
{ |
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.
Did you take a look on SSIM from quality
module (opencv_contrib)?
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.
Replaced it with using quality module implementation.
|
||
Mat imgUpscaled; | ||
|
||
double elapsed = 0.0; |
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.
avoid "tabs". Indentation is 4 spaces
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. Reformatted the code to match the guideline more.
|
||
void DnnSuperResQuality::benchmark(DnnSuperResImpl sr, Mat img, | ||
std::vector<double>& psnrValues, | ||
std::vector<double>& ssimValues, |
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.
I believe this should be moved into a separate sample (module itself is not good place for functions with a lot of customization parameters and/or drawing).
For reproducible benchmark results (in meaning of time) consider using of performance tests which collects multiple samples and drop outliers automatically.
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.
I removed it from the module itself, and moved it to just sample codes. I agree that it's better that way. And I added performance test to the module.
TEST(CV_DnnSuperResSingleOutputTest, accuracy) | ||
{ | ||
CV_DnnSuperResSingleOutputTest test; | ||
test.safe_run(); |
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.
We don't need testing class here and below (it is from legacy testing system).
Just call runOneModel(...)
directly (or move its content into test):
TEST(DnnSuperResSingleOutputTest, accuracy)
{
cv::Ptr<DnnSuperResImpl> dnn_sr = makePtr<DnnSuperResImpl>();
... all your test code from runOneModel() ...
}
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.
I updated the test codes.
if ( this->alg != "lapsrn" ) | ||
{ | ||
std::cout << "Only LapSRN support multiscale upsampling for now!" << std::endl; | ||
return; |
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.
Please use CV_Error()
instead of stdout.
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.
I replaced them with CV_Error.
(int) inputVideo.get(CAP_PROP_FRAME_HEIGHT) * this->sc); | ||
|
||
VideoWriter outputVideo; | ||
outputVideo.open(outputPath, ex, inputVideo.get(CAP_PROP_FPS), S, true); |
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.
This code of writer creation doesn't work well.
Do we really need this method in the module code? What is about cameras?
Does it make sense? Video compression algorithms can significantly amend results of algorithms.
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.
Removed it from part of the module into just sample code, it makes more sense there.
- avoid highgui dependency - cleanup public API - use InputArray/OutputArray - test: avoid legacy test API
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.
@fannymonori @Saafke Well done! Thank you 👍
I will take a look on #2231 tomorrow.
Merge with extra: opencv/opencv_extra#662
Final and complete PR of GSoC 2019, project of Fanny Monori. Parts of the code was done with Xavier Weber, who worked on the same GSoC project. His PR is available here: #2231
My own contribution was implementing the ESPCN and LapSRN code in TensorFlow, doing parts of the main functionality, do the benchmarking functionality, and corresponding tutorials, samples, and tests. However we shared most of the work with the main functionality (importing the models and doing the super-resolution).
This pull request changes: