-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
GSOC17 - Facemark API #1257
GSOC17 - Facemark API #1257
Conversation
1abc204
to
843ef4b
Compare
@kurnianggoro Do you have plans to add implementation of algorithm from dlib ? |
e284e30
to
491f554
Compare
967d07d
to
28f7894
Compare
@kurnianggoro What is the license of images from opencv_extra patch? |
hi @alalek , I just found them on google, will it be problem? |
test_stereo has sporadic failures - build restarted. All images should have known proper usage rights. It is better to reuse existed OpenCV images, or use Google filtering (but anyway double check is required). |
Initial structure of the facemark API and AAM header
28f7894
to
7fc15de
Compare
@alalek hi, i have changed the file and provide the source links (the license is available,you can check), they are from wikipedia, so i hope it will not be problem |
@kurnianggoro, let's not put faces of public people (or not public, it does not matter) to OpenCV. lena is the exception, 'cause it's considered a standard test image. |
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 contribution!
Please take a look on comments below.
modules/face/src/facemarkLBF.cpp
Outdated
|
||
FacemarkLBF::Params::Params(){ | ||
|
||
cascade_face = "../data/haarcascade_frontalface_alt.xml"; |
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.
Hardcoded paths should be avoided in sources.
Required parameters should be passed explicitly.
modules/face/src/facemarkLBF.cpp
Outdated
tree_n=6; | ||
tree_depth=5; | ||
bagging_overlap = 0.4; | ||
model_filename = "LBF.model"; |
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.
The same.
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 is just the default filename for the saving file, is it also problem? if it were for reading file like the cascade face then for sure it will be problem
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 is minor problem (at least there is no relative path to subdirectory).
modules/face/src/facemarkLBF.cpp
Outdated
} | ||
|
||
FILE *fd = fopen(s.c_str(), "rb"); | ||
regressor.read(fd, params); |
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.
OpenCV modules should not read/write files directly, especially via old "C-API" (there are a lot of missing error checks at least; there is no in-memory storage support; cross-platform issues).
Why FileStorage doesn't work here?
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, i will try to change to filestorage.
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.
modules/face/src/facemarkLBF.cpp
Outdated
regressor.trainRegressor(imgs, gt_shapes, current_shapes, bboxes, mean_shape, 0, params); | ||
|
||
FILE *fd = fopen(params.model_filename.c_str(), "wb"); | ||
assert(fd); |
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.
assert()
should not be used in OpenCV, use CV_Assert()
instead.
modules/face/src/facemarkLBF.cpp
Outdated
|
||
// random shuffle | ||
unsigned int seed = (unsigned int)std::time(0); | ||
std::srand(seed); |
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.
rand()
is not thread-safe - can't provide reproducible results (for testing).
Consider to use cv::rand() , cv::theRNG() 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.
according to the documentation http://docs.opencv.org/3.2.0/d2/de8/group__core__array.html#ga75843061d150ad6564b5447e38e57722 theRNG function will use different random generator for each thread, but in this case the program only needs to randomize the order of training sample, which should produce same random order for each sample category (i.e. the randomized order or image sample should corresponds to the randomized order of the annotation). If reproducible results is expected, then i think we just need to set the seed value to a fixed number, or just remove this shuffling part (i don't think it will produce significantly different results)
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.
cv::theRNG
is thread-safe (maintains own state for each thread).
Seed initialization from time()
should be removed from algorithm implementations - seed should be controlled externally.
Regarding this exact algorithm, I believe it should work without shuffling of input data.
modules/face/src/liblinear.hpp
Outdated
void cdotc_(fcomplex *dotval, int *n, fcomplex *cx, int *incx, | ||
fcomplex *cy, int *incy); | ||
|
||
void cdotu_(fcomplex *dotval, int *n, fcomplex *cx, int *incx, |
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.
Using of external libraries should be avoided. Especially in case of duplicated functionality.
Why does cv::ml::LogisticRegression
not work for you?
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 tried to remove the liblinear dependency but was not successful (using gradient descent regression and simple regression w^T x = y). The LogisticRegression result is in range 0..1, isn't it? I considered to use this function before but worried that the result will be different to the original version (i need to normalize the target values, i.e. the landmark points, into 0..1 range,maybe by dividing the values by width and height of input image, and not sure about the result) and the deadline was approaching, so i did not test LogisticRegression. But let me try now...
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. Thank you for information.
string model_filename = "LBF.model"; | ||
|
||
Ptr<Facemark> facemark = FacemarkLBF::create(); | ||
EXPECT_NO_THROW(facemark->loadModel(model_filename.c_str())); |
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.
Each test should be standalone and don't rely on results of other tests.
Also tests should not write any data (models) if it is not required.
Probably we should put model somewhere or we can merge train/load tests into one.
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.
merging the train-load test is easier, i can put a flag for not saving the model.
@vpisarev the test function needs at least 2 images, so i put Mark Zuckerberg and Dwayne Johnson photos from wikipedia (the license allows it, CCA 2.0 and CCA 3.0), will that be a problem? also regarding to the lena image, in other library it was removed due to copyright matter, i am not sure why opencv still have it. check it here torch/nn#854 also in wikipedia it is stated that lena is non free image https://en.wikipedia.org/wiki/File:Lenna.png |
AAM tests are crashed in the "Debug" build. Please take a look. |
modules/face/src/facemarkAAM.cpp
Outdated
@@ -560,8 +560,8 @@ namespace face { | |||
reduce(Ys,sumYs, 0, CV_REDUCE_SUM); | |||
|
|||
//calculate the normrnd | |||
double normX = sqrt(sumXs.at<float>(0)+sumXs.at<float>(1)); | |||
double normY = sqrt(sumYs.at<float>(0)+sumYs.at<float>(1)); | |||
double normX = sqrt(Mat(sumXs.reshape(1)).at<float>(0)+Mat(sumXs.reshape(1)).at<float>(1)); |
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.
@alalek the error in build debug is fixed here, but why it is not checked anymore?
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 build is optional, it is not triggered automatically.
Debug build is passed, thanks!
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.
Please take a look on comments below.
using namespace cv::face; | ||
|
||
CascadeClassifier face_detector; | ||
bool customDetector( InputArray image, OutputArray ROIs, void * config = 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.
Make this function local to prevent conflicts via static
qualifier or anonymous namespace.
modules/face/src/facemarkAAM.cpp
Outdated
char x[256]; | ||
for(int i=0;i< (int)AAM.scales.size();i++){ | ||
sprintf(x,"scale%i_max_m",i); | ||
fs << x << AAM.textures[i].max_m; |
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.
sprintf
is unsafe in general and banned by static code analyzers. consider to use cv::format()
instead
modules/face/src/facemarkAAM.cpp
Outdated
} | ||
|
||
void FacemarkAAMImpl::saveModel(String s){ | ||
FileStorage fs(s.c_str(),FileStorage::WRITE); |
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 try WRITE_BASE64
(should reduce the model size)?
EXPECT_NO_THROW(facemark->training()); | ||
} | ||
|
||
TEST(CV_Face_FacemarkAAM, can_load_model) { |
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.
The same note about standalone tests.
a8025ff
to
9f90785
Compare
Hi @alalek , I already fixed all the problems you've mentioned, but the building bot failed due to stereo module, can you please restart the building? also regarding the liblinear depency, I've tried to use opencv's logistric regression and neural network but both of them are not compatible. For the log-res, it seems like that the algorithm automatically assign categories to the training data and the number of resulted categories are inconsistent across different training data. As for the neural network, it do not support linear activation, I tried to train but the results are nan. So, I grab the support vector regression code from the liblinear, it just one function, not too long. I've tested it and the result is good. You can try to train using the Helen dataset if you want. The tutorial is here http://pullrequest.opencv.org/buildbot/export/pr_contrib/1257/docs/d7/dec/tutorial_facemark_usage.html but make sure your system has around 9GB of RAM and just wait 45in~1hour for the for the training. I've tried to train on smaller dataset but the results are not good. |
@kurnianggoro Thank you! Update looks good to me. |
The current issue is related to test images. We can't accept such risk. |
modules/face/src/facemarkLBF.cpp
Outdated
Gnorm1_new = 0; | ||
|
||
for(i=0; i<active_size; i++){ | ||
int j = i+rand()%(active_size-i); |
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 using of rand()
- it doesn't provide reproducible results.
Like "stereo" test (it uses rand()
too), which fails time-to-time.
hi @alalek i have changed the test images using frames taken from the tracking module test video, I hope it will not be a problem now. |
Thank you! Looks good to me 👍 |
Is there a python wrapper of this API in any release version? |
Merge with extra: opencv/opencv_extra#365
Project: API for Facial Landmark Detector
Mentor: Delia Passalacqua
This pull request contains the implementation of Facemark API as well as two facemark algorithm, AAM and LBF.
video: https://www.youtube.com/watch?v=B7WGyhl2zm8
report: https://gist.github.com/kurnianggoro/74de9121e122ad0bd825176751d47ecc
Currently, there are several interfaces provided in the Facemark base class including:
For the AAM algorithm, all of its training functionalities are already coded.
Several related functionalities are listed as follows: