face: fix facemark landmarks returned as all zeros on OpenCV 5 - #4190
face: fix facemark landmarks returned as all zeros on OpenCV 5#4190aarochu wants to merge 3 commits into
Conversation
Facemark::fit() handed back the correct number of landmarks with every point left at (0,0) when the output was a std::vector<std::vector<Point2f>>, which is what the documented usage and the samples pass. _copyVector2Output() writes each result through a Mat header obtained from OutputArray::getMat(i), which aliases the destination vector's storage. The copy therefore has to match that header's shape: copyTo() reallocates the header when it does not, and the copy then lands in a fresh buffer that is thrown away instead of in the caller's vector. The source was built as Mat(Mat(vec[i]).t()) to produce the 1 x N header OpenCV 4 returns for a vector<vector<T>>. OpenCV 5 returns a 1-D header there instead, so the transposed source no longer matched and every landmark was silently dropped. Build the source as a 1 x N two-channel row with reshape() so it matches on both, and size the destination from the actual landmark count rather than a hard-coded 68. The same helper is duplicated in the LBF, AAM and Kazemi implementations and all three were affected. The existing tests only checked the landmark count, which stayed correct, so they did not catch this. They now also assert that the points actually reached the caller. Fixes opencv/opencv#29703
|
Please drop irrelevant comments from the code. |
Addresses review feedback from @asmorkalov on opencv#4190.
|
Comments should be gone! |
Addresses review feedback from @asmorkalov on opencv#4190.
fallenmi
left a comment
There was a problem hiding this comment.
Reviewed with OpenAI Codex assistance; I independently built and tested the exact revision and verified the results below.
The OpenCV 5 output conversion is fixed. Keeping the new regression assertions while rolling back only the three production conversion hunks made both FacemarkAAM.test_workflow and FacemarkLBF.test_workflow fail deterministically: each returned 68 landmarks and all 68 were (0, 0). Restoring exact head 05f89d83f99373869fd9f4200443a2b2b7cc5c03 made both tests pass with every returned landmark nonzero.
I built the face module against current OpenCV 5.x core and ran the complete face test binary with the matching opencv_extra data: 19/19 tests passed. The dynamic vec[i].size() allocation also removes the hard-coded 68-point assumption while the two-channel reshape preserves the actual Point2f layout. git diff --check is clean.
The published Actions matrix is still red (3 success, 9 failure), so those jobs should be resolved or rerun before merge; my approval is based on the exact local base/head regression and full face-module suite above.
|
Hi I am the reporter of the original bug, and was wondering what is needed to move this forward, or how can we help further. Thank you @aarochu |
Fixes opencv/opencv#29703
Problem
Facemark::fit()returns the correct number of landmarks with every point left at(0,0)on OpenCV 5, while the same code works on 4.x. The reporter hit this withFacemarkLBFand the pretrainedlbfmodel.yaml.It only happens when the output is a
std::vector<std::vector<Point2f>>— which is what the documentation, the samples and the reporter's code all pass — solandmarks[i].size()looks right (68) and every coordinate is zero.Root cause
_copyVector2Output()writes each result through aMatheader obtained fromOutputArray::getMat(i). That header only aliases the destination vector's storage, so the source has to match its shape already:copyTo()callscreate()first, and when the shape differs the header is reallocated, so the copy lands in a fresh buffer that is discarded instead of in the caller's vector.The source was built as
Mat(Mat(vec[i]).t())to match the1 x Nheader OpenCV 4 hands out for avector<vector<T>>:OpenCV 5 returns a 1-D header there instead, so the transposed source no longer matches and every landmark is silently dropped.
Fix
Build the source as a
1 x Ntwo-channel row withreshape(), which matches the destination header on both 4.x and 5.x, and size the destination from the actual landmark count instead of a hard-coded68(so models with a different number of landmarks work too).The helper is duplicated in the LBF, AAM and Kazemi implementations and all three were affected, so all three are fixed.
The
Mat/UMatoutput branches are left alone: they bind the real destination object rather than an aliasing header, so a reallocatingcopyTo()still delivers the data there. Changing them would alter their output shape without fixing a bug.Testing
Verified on a local Windows/MSVC build of 5.x + contrib (
5.1.0-dev), using the reporter's exact sequence (cascade →detectMultiScale→FacemarkLBF::create/loadModel/fit) with the pretrainedlbfmodel.yamlfrom the issue:Before:
After:
I also reproduced the underlying shape mismatch in isolation against
opencv_core5.x and confirmed the replacement copies correctly for 5, 68 and 194 landmarks as well as the empty case.The existing
CV_Face_FacemarkLBF.test_workflowandCV_Face_FacemarkAAM.test_workflowonly asserted the landmark count, which stayed correct throughout — that is why this went unnoticed. They now also assert the points actually reached the caller. Confirmed this is a real regression test: with the source fix reverted and only the test change applied, it fails withFull
opencv_test_facesuite passes with the fix (19/19, withOPENCV_TEST_DATA_PATHplus theface_landmark_model.datthe module downloads at configure time).