Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2419273002:
Shape Detection: Add two content browser tests for face detection (Closed)
DescriptionThis CL adds two content browser tests for face detection.
It also includes a single-face image for testing.
BUG=646083
TEST=content/test/data/media/face_detection_test.html
Committed: https://crrev.com/6ba9555c974b3d4c458fa8921b7ac5f681bc145c
Cr-Commit-Position: refs/heads/master@{#426653}
Patch Set 1 : Two browsertests for zero-face-photo and one-face-photo #
Total comments: 1
Patch Set 2 : reillyg@ comments #
Total comments: 19
Patch Set 3 : Approximate comparisons #
Total comments: 11
Patch Set 4 : mcasas@ and reillyg@ comments #
Total comments: 2
Patch Set 5 : Move MAYBE_ to the test fixture class #
Messages
Total messages: 51 (35 generated)
Description was changed from ========== Added a simple browsertest for face detection, unable to read image url correctly. BUG=646083 ========== to ========== Image source: https://commons.wikimedia.org/wiki/File%3A--%D0%98%D0%B7%D0%BE%D0%B1%D1%80%D0... BUG=646083 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Image source: https://commons.wikimedia.org/wiki/File%3A--%D0%98%D0%B7%D0%BE%D0%B1%D1%80%D0... BUG=646083 ========== to ========== Image source: https://commons.wikimedia.org/wiki/File%3A--%D0%98%D0%B7%D0%BE%D0%B1%D1%80%D0... BUG=646083 TEST= ==========
Description was changed from ========== Image source: https://commons.wikimedia.org/wiki/File%3A--%D0%98%D0%B7%D0%BE%D0%B1%D1%80%D0... BUG=646083 TEST= ========== to ========== This CL adds two browser test cases for face detection. Image source: https://commons.wikimedia.org/wiki/File%3A--%D0%98%D0%B7%D0%BE%D0%B1%D1%80%D0... BUG=646083 TEST=content/test/data/media/face_detection_test.html ==========
Description was changed from ========== This CL adds two browser test cases for face detection. Image source: https://commons.wikimedia.org/wiki/File%3A--%D0%98%D0%B7%D0%BE%D0%B1%D1%80%D0... BUG=646083 TEST=content/test/data/media/face_detection_test.html ========== to ========== This CL adds two browser test cases for face detection. It also adds a single-face-image for testing. Image source: https://commons.wikimedia.org/wiki/File%3A--%D0%98%D0%B7%D0%BE%D0%B1%D1%80%D0... BUG=646083 TEST=content/test/data/media/face_detection_test.html ==========
xianglu@chromium.org changed reviewers: + mcasas@chromium.org
ptal.
xianglu@chromium.org changed reviewers: + reillyg@chromium.org
ptal.
https://codereview.chromium.org/2419273002/diff/40001/content/browser/shapede... File content/browser/shapedetection/shapedetection_browsertest.cc (right): https://codereview.chromium.org/2419273002/diff/40001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:15: const char faceDetectionTestHtml[] = "/media/face_detection_test.html"; kFaceDetectionTestHtml
Description was changed from ========== This CL adds two browser test cases for face detection. It also adds a single-face-image for testing. Image source: https://commons.wikimedia.org/wiki/File%3A--%D0%98%D0%B7%D0%BE%D0%B1%D1%80%D0... BUG=646083 TEST=content/test/data/media/face_detection_test.html ========== to ========== This CL adds two browser test cases for face detection. It also adds a single-face-image for testing. BUG=646083 TEST=content/test/data/media/face_detection_test.html ==========
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== This CL adds two browser test cases for face detection. It also adds a single-face-image for testing. BUG=646083 TEST=content/test/data/media/face_detection_test.html ========== to ========== This CL adds two browser tests for face detection. It also adds a single-face-image for testing. BUG=646083 TEST=content/test/data/media/face_detection_test.html ==========
Description was changed from ========== This CL adds two browser tests for face detection. It also adds a single-face-image for testing. BUG=646083 TEST=content/test/data/media/face_detection_test.html ========== to ========== This CL adds two browser tests for face detection. It also includes a single-face image for testing. BUG=646083 TEST=content/test/data/media/face_detection_test.html ==========
ptal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== This CL adds two browser tests for face detection. It also includes a single-face image for testing. BUG=646083 TEST=content/test/data/media/face_detection_test.html ========== to ========== This CL adds two content browser tests for face detection. It also includes a single-face image for testing. BUG=646083 TEST=content/test/data/media/face_detection_test.html ==========
looking good, I have a bunch of small comments. https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... File content/browser/shapedetection/shapedetection_browsertest.cc (right): https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:20: // allows for generating bounding boxes of face for a still image. ... for generating bounding boxes for faces on still images. ? https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:31: const std::string expected_results) { const std::string& (both parameters). https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:37: std::string js_command = const, here and in l. 34, 35 and 37. https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:49: #endif Bundle these l.45-49 and l.56-60 around l.10 and add a reference to the bug we're using for development in a comment, e.g.: // TODO(xianglu): Enable other platforms with support: https://crbug.com/646803. https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:65: "x=68.640625,y=102.96875,w=171.5625,h=171.5625#"); Hmm I wonder if this would not produce failures due to numerical accuracy, i.e. perhaps device A says x=68.6... and device B says x=68.7... It's OK if you don't address this concern in this particular CL, but chances are that MAC would return something slightly different, so keep that in mind, and optimally add a comment TODO detailing that maybe in the future you'll need to pass the points as an array, sth like: // TODO(xianglu): consider passing the coordinates as an // array to allow for approximate comparisons. https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... File content/test/data/media/face_detection_test.html (right): https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... content/test/data/media/face_detection_test.html:13: function detectFacesOnImageUrl(url) { This code, and the content_browsertest, seem to be ready to detect several faces, but that functionality is not exercised, and I believe we don't expect to do it anytime soon, so consider restricting the code here to support one face or none (including renaming where necessary). https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... content/test/data/media/face_detection_test.html:19: .then(faceDetectionResult => { micro-nit: s/faceDetectionResult/detectedFaces/ ? https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... content/test/data/media/face_detection_test.html:21: var boundingBox = faceDetectionResult[i]; const? https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... content/test/data/media/face_detection_test.html:32: Remove this empty line?
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mcasas@ ptal. https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... File content/browser/shapedetection/shapedetection_browsertest.cc (right): https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:20: // allows for generating bounding boxes of face for a still image. On 2016/10/18 23:15:13, mcasas wrote: > ... for generating bounding boxes for faces on still images. > ? Done. https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:31: const std::string expected_results) { On 2016/10/18 23:15:13, mcasas wrote: > const std::string& (both parameters). Done. https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:37: std::string js_command = On 2016/10/18 23:15:13, mcasas wrote: > const, here and in l. 34, 35 and 37. Done. https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:49: #endif On 2016/10/18 23:15:13, mcasas wrote: > Bundle these l.45-49 and l.56-60 around l.10 > and add a reference to the bug we're using for > development in a comment, e.g.: > // TODO(xianglu): Enable other platforms with support: https://crbug.com/646803. Done. https://codereview.chromium.org/2419273002/diff/60001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:65: "x=68.640625,y=102.96875,w=171.5625,h=171.5625#"); On 2016/10/18 23:15:13, mcasas wrote: > Hmm I wonder if this would not produce failures due to > numerical accuracy, i.e. perhaps device A says x=68.6... > and device B says x=68.7... > > It's OK if you don't address this concern in this particular CL, > but chances are that MAC would return something slightly > different, so keep that in mind, and optimally add a comment > TODO detailing that maybe in the future you'll need to pass > the points as an array, sth like: > // TODO(xianglu): consider passing the coordinates as an > // array to allow for approximate comparisons. Done. https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... File content/test/data/media/face_detection_test.html (right): https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... content/test/data/media/face_detection_test.html:13: function detectFacesOnImageUrl(url) { On 2016/10/18 23:15:14, mcasas wrote: > This code, and the content_browsertest, seem to be > ready to detect several faces, but that functionality > is not exercised, and I believe we don't expect to do > it anytime soon, so consider restricting the code here > to support one face or none (including renaming where > necessary). Face detection implementation in Java will return a list even if only one face is detected. I think if there are suitable images, test cases for more than one faces should be included in the future as well. How about adding a comment here instead of restricting the code itself? https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... content/test/data/media/face_detection_test.html:19: .then(faceDetectionResult => { On 2016/10/18 23:15:13, mcasas wrote: > micro-nit: s/faceDetectionResult/detectedFaces/ ? Done. https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... content/test/data/media/face_detection_test.html:21: var boundingBox = faceDetectionResult[i]; On 2016/10/18 23:15:14, mcasas wrote: > const? If I understand correctly, const in javascript means the variable name is dedicated to a specific object, and it should never be reassigned? Changing content of the referred object is still allowed though. https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... content/test/data/media/face_detection_test.html:32: On 2016/10/18 23:15:14, mcasas wrote: > Remove this empty line? Done.
Couple of nits. https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... File content/test/data/media/face_detection_test.html (right): https://codereview.chromium.org/2419273002/diff/60001/content/test/data/media... content/test/data/media/face_detection_test.html:13: function detectFacesOnImageUrl(url) { On 2016/10/19 23:26:10, xianglu wrote: > On 2016/10/18 23:15:14, mcasas wrote: > > This code, and the content_browsertest, seem to be > > ready to detect several faces, but that functionality > > is not exercised, and I believe we don't expect to do > > it anytime soon, so consider restricting the code here > > to support one face or none (including renaming where > > necessary). > > Face detection implementation in Java will return a list even if only one face > is detected. I think if there are suitable images, test cases for more than one > faces should be included in the future as well. How about adding a comment here > instead of restricting the code itself? Hmm I guess in that case, no comment is needed, since a method should not restrict its callers. https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... File content/browser/shapedetection/shapedetection_browsertest.cc (right): https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:62: } nit: no need for curly brackets {} for one line bodies. https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:66: ASSERT_EQ(expected_results.size(), results.size()); Good! https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:70: for (size_t pram_idx = 0; pram_idx < 4; pram_idx++) { Micro-nit: try to get used to preincrement the index variable in for loops; in this case, it doesn't matter, but in certain situatiosns, pre and post increments are different, and you want the pre. (This applies to l. 67 and 70). Also, variable names should not be abbreviations: |pram_idx| is illegible. What about just |index|, or |i| ? https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:72: // different platforms. You can also consider using e.g. EXPECT_NEAR(expected_result[pram_idx], result[pram_idx], 0.1), where 0.1 is the tolerated error, and then it should work in all platforms (and 0.1 pixels of error in a face bounding box is totally cool), and then you don't need the comment :) https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:90: 171.5625}; nit: const :)
https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... File content/browser/shapedetection/shapedetection_browsertest.cc (right): https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:73: EXPECT_FLOAT_EQ(expected_result[pram_idx], result[pram_idx]); Tip: Add << " at index " << param_index; to the end of test expectations like this because it'll make it easier to debug failures in the future.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal:) https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... File content/browser/shapedetection/shapedetection_browsertest.cc (right): https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:62: } On 2016/10/19 23:41:11, mcasas wrote: > nit: no need for curly brackets {} for one line bodies. Done. https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:70: for (size_t pram_idx = 0; pram_idx < 4; pram_idx++) { On 2016/10/19 23:41:11, mcasas wrote: > Micro-nit: try to get used to preincrement > the index variable in for loops; in this case, it > doesn't matter, but in certain situatiosns, pre and > post increments are different, and you want the > pre. (This applies to l. 67 and 70). > > Also, variable names should not be abbreviations: > |pram_idx| is illegible. What about just |index|, or |i| ? Done. https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:72: // different platforms. On 2016/10/19 23:41:11, mcasas wrote: > You can also consider using e.g. > EXPECT_NEAR(expected_result[pram_idx], result[pram_idx], 0.1), > where 0.1 is the tolerated error, and then it should > work in all platforms (and 0.1 pixels of error in a > face bounding box is totally cool), and then you don't > need the comment :) Done. https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:73: EXPECT_FLOAT_EQ(expected_result[pram_idx], result[pram_idx]); On 2016/10/19 23:48:40, Reilly Grant wrote: > Tip: Add << " at index " << param_index; to the end of test expectations like > this because it'll make it easier to debug failures in the future. Done. https://codereview.chromium.org/2419273002/diff/80001/content/browser/shapede... content/browser/shapedetection/shapedetection_browsertest.cc:90: 171.5625}; On 2016/10/19 23:41:11, mcasas wrote: > nit: const :) Done.
lgtm with a nit. https://codereview.chromium.org/2419273002/diff/120001/content/browser/shaped... File content/browser/shapedetection/shapedetection_browsertest.cc (right): https://codereview.chromium.org/2419273002/diff/120001/content/browser/shaped... content/browser/shapedetection/shapedetection_browsertest.cc:86: 171.5625}; Nit: indent. (run `git cl format` before uploading).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
xianglu@chromium.org changed reviewers: + nick@chromium.org
nick@ RS ptal.
lgtm with one idea. https://codereview.chromium.org/2419273002/diff/120001/content/browser/shaped... File content/browser/shapedetection/shapedetection_browsertest.cc (right): https://codereview.chromium.org/2419273002/diff/120001/content/browser/shaped... content/browser/shapedetection/shapedetection_browsertest.cc:20: #define MAYBE_DetectFacesOnImageWithOneFace DISABLED_DetectFacesOnImageWithOneFace Might be simpler to do the DISABLED_ trick on the test fixture class, rather than each individual test. Here's an example of that pattern: https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_browsertes...
The CQ bit was checked by xianglu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by xianglu@chromium.org
The CQ bit was checked by xianglu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, mcasas@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/2419273002/#ps140001 (title: "Move MAYBE_ to the test fixture class")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== This CL adds two content browser tests for face detection. It also includes a single-face image for testing. BUG=646083 TEST=content/test/data/media/face_detection_test.html ========== to ========== This CL adds two content browser tests for face detection. It also includes a single-face image for testing. BUG=646083 TEST=content/test/data/media/face_detection_test.html ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== This CL adds two content browser tests for face detection. It also includes a single-face image for testing. BUG=646083 TEST=content/test/data/media/face_detection_test.html ========== to ========== This CL adds two content browser tests for face detection. It also includes a single-face image for testing. BUG=646083 TEST=content/test/data/media/face_detection_test.html Committed: https://crrev.com/6ba9555c974b3d4c458fa8921b7ac5f681bc145c Cr-Commit-Position: refs/heads/master@{#426653} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6ba9555c974b3d4c458fa8921b7ac5f681bc145c Cr-Commit-Position: refs/heads/master@{#426653} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
