Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2399593005:
ShapeDetectionImpl: Android implementation of detectFace() (Closed)
DescriptionShapeDetectionImpl: Android implementation of detectFace()
detectFace() method calls android FaceDetecor API to get Face positions.
A temporary github demo is used for testing and visualization.
Currently it can crash if the image comes from camera.
BUG=646083
TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/c43957493361040d68a66bd4df7d8693d954799c
Cr-Commit-Position: refs/heads/master@{#425270}
Patch Set 1 : Android implementation of detectFace() #
Total comments: 14
Patch Set 2 : ShapeDetectionImpl: Added explanation on bitmap. #
Total comments: 12
Patch Set 3 : mcasas@ comments #
Total comments: 6
Patch Set 4 : Move factory class into a separate file and rebase #
Total comments: 3
Patch Set 5 : Reorder alphabetically #
Total comments: 17
Patch Set 6 : Add security check in ShapeDetectionImpl.java #
Total comments: 2
Patch Set 7 : tsepez@ comment #
Total comments: 2
Patch Set 8 : ncarter@ comment #
Total comments: 1
Patch Set 9 #
Messages
Total messages: 84 (54 generated)
Description was changed from ========== First commit for ShapeDetectionImpl First commit for ShapeDetectionImpl on android. BUG=646083 ========== to ========== First commit for ShapeDetectionImpl First commit for ShapeDetectionImpl on android. BUG=646083 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== First commit for ShapeDetectionImpl First commit for ShapeDetectionImpl on android. BUG=646083 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get the Face position. A temporary github demo is used for testing and visualization. Can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html ==========
Description was changed from ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get the Face position. A temporary github demo is used for testing and visualization. Can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html ========== to ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get the Face position. A temporary github demo is used for testing and visualization. Can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:40002) has been deleted
xianglu@chromium.org changed reviewers: + mcasas@chromium.org
mcasas@ ptal.
Description was changed from ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get the Face position. A temporary github demo is used for testing and visualization. Can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get the Face position. A temporary github demo is used for testing and visualization. Can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get the Face position. A temporary github demo is used for testing and visualization. Can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get Face position. A temporary github demo is used for testing and visualization. Currently it can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get Face position. A temporary github demo is used for testing and visualization. Currently it can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get Face positions. A temporary github demo is used for testing and visualization. Currently it can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:70001) has been deleted
looking good, a few comments https://codereview.chromium.org/2399593005/diff/90001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2399593005/diff/90001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2166: ->CreateInterfaceFactory<blink::mojom::ShapeDetection>()); These 3 lines should be protected by a #if defined(OS_ANDROID) guard, since there no Java outside that platform. https://codereview.chromium.org/2399593005/diff/90001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2172: nit: No need for this extra space. https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:28: * https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html This line should go in the CL description (below the BUG=...). https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:40: int numBytes = width * height * 4; nit: make this final, also in l.53, l. 58, 59, 61 and wherever else makes sense. https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:49: Bitmap bitmap2 = Bitmap.createBitmap(pixels, width, height, Bitmap.Config.RGB_565); Ok, IIUC here we do two copies: |imageBuffer| --> |bitmap| and |bitmap| --> |bitmap2|, if this is the only way, we need to document it properly, so developers of the future won't be left scratching their heads. Also, would this [1] bitmap.reconfigure(width, height, Bitmap.Config.RGB_565; work? (even better: bitmap.setConfig(Bitmap.Config.RGB_565); would do the same, if it works -- this would need a sizable comment, since it's not evident that a simple setBla() will do that much work). [1] https://developer.android.com/reference/android/graphics/Bitmap.html#reconfig..., int, android.graphics.Bitmap.Config) https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:69: faceDetectionResult.boundingBoxes[i] = boundingBox; Please add a TODO(xianglu) here with something like: // TODO(xianglu): consider adding Face.confidence and Face.pose as well. https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:94: return new ShapeDetectionImpl(mContext); I think we don't really need |context| here nor in ShapeDetectionImpl, so we can just not hold on to it in this file.
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/2399593005/diff/90001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2399593005/diff/90001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2166: ->CreateInterfaceFactory<blink::mojom::ShapeDetection>()); On 2016/10/11 02:51:17, mcasas wrote: > These 3 lines should be protected by a > #if defined(OS_ANDROID) > guard, since there no Java outside that > platform. Done. https://codereview.chromium.org/2399593005/diff/90001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:2172: On 2016/10/11 02:51:17, mcasas wrote: > nit: No need for this extra space. Done. https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:28: * https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html On 2016/10/11 02:51:18, mcasas wrote: > This line should go in the CL description (below > the BUG=...). Done. https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:40: int numBytes = width * height * 4; On 2016/10/11 02:51:18, mcasas wrote: > nit: make this final, also in l.53, l. 58, 59, 61 > and wherever else makes sense. Done. https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:49: Bitmap bitmap2 = Bitmap.createBitmap(pixels, width, height, Bitmap.Config.RGB_565); On 2016/10/11 02:51:18, mcasas wrote: > Ok, IIUC here we do two copies: > |imageBuffer| --> |bitmap| and > |bitmap| --> |bitmap2|, if this is the > only way, we need to document it properly, > so developers of the future won't be left > scratching their heads. > > Also, would this [1] > > bitmap.reconfigure(width, height, Bitmap.Config.RGB_565; > > work? > > (even better: bitmap.setConfig(Bitmap.Config.RGB_565); > would do the same, if it works -- this would need a > sizable comment, since it's not evident that a simple > setBla() will do that much work). > > [1] > https://developer.android.com/reference/android/graphics/Bitmap.html#reconfig..., > int, android.graphics.Bitmap.Config) I tried bitmap.setConfig(), but it seems FaceDetector doesn't work correctly with it. I have to make this intermediate int array as a workaround. https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:69: faceDetectionResult.boundingBoxes[i] = boundingBox; On 2016/10/11 02:51:18, mcasas wrote: > Please add a TODO(xianglu) here with something > like: > // TODO(xianglu): consider adding Face.confidence and Face.pose as well. Done. https://codereview.chromium.org/2399593005/diff/90001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:94: return new ShapeDetectionImpl(mContext); On 2016/10/11 02:51:18, mcasas wrote: > I think we don't really need |context| here nor > in ShapeDetectionImpl, so we can just not hold > on to it in this file. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
xianglu@chromium.org changed reviewers: + qinmin@chromium.org
@qinmin ptal.
lgtm % comments/nits https://codereview.chromium.org/2399593005/diff/110001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2399593005/diff/110001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2171: ->CreateInterfaceFactory<blink::mojom::ShapeDetection>()); nit: move this before l.2165, so the VibrationManager instantiations in Android/non Android are contiguous. https://codereview.chromium.org/2399593005/diff/110001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/110001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:45: // bytebuffer to create this intermediate bitmap. If you are talking about types, use camel case: Bitmap, ByteBuffer, SharedBufferHandle, Array, If you're referring to a variable, put pipes around it: |bitmap|. Also: s/we need to create copy/we need to create a copy/ https://codereview.chromium.org/2399593005/diff/110001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:51: // It turns out that facedetector is not able to detect correctly if Same here and in l.48: |bitmap|, FaceDetector. nit: When a variable or data type is subject of the phrase, it doesn't need "The": // |bitmap| must be in 565 format... https://codereview.chromium.org/2399593005/diff/110001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:52: // you simply use pixmap.setConfig(). I believe this is because Maybe s/I believe this is/This might be/ Usually all code comments are written in impersonal style ;) https://codereview.chromium.org/2399593005/diff/110001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:54: // alphatype in original image is premultiplied. We can use getPixels() Here, not being subject, usually needs determinant: ...alpha type in _the_ original_ image... https://codereview.chromium.org/2399593005/diff/110001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:59: Bitmap bitmapUnpremul = Bitmap.createBitmap(pixels, width, height, Bitmap.Config.RGB_565); s/bitmpUnpremul/unPremultipliedBitmap/ (no abbreviations in variable names).
https://codereview.chromium.org/2399593005/diff/130001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2399593005/diff/130001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2166: GetGlobalJavaInterfaces() nit: fix the indent here https://codereview.chromium.org/2399593005/diff/130001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/130001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:27: nit: remove extra lone https://codereview.chromium.org/2399593005/diff/130001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:99: public static class Factory implements InterfaceFactory<ShapeDetection> { is this class ever being used here? If not, moving it to a standalone file
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...
@qinmin ptal. https://codereview.chromium.org/2399593005/diff/110001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2399593005/diff/110001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2171: ->CreateInterfaceFactory<blink::mojom::ShapeDetection>()); On 2016/10/11 23:04:48, mcasas wrote: > nit: move this before l.2165, so the VibrationManager > instantiations in Android/non Android are > contiguous. Done. https://codereview.chromium.org/2399593005/diff/110001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/110001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:45: // bytebuffer to create this intermediate bitmap. On 2016/10/11 23:04:49, mcasas wrote: > If you are talking about types, use camel case: > Bitmap, ByteBuffer, SharedBufferHandle, Array, > If you're referring to a variable, put pipes around > it: |bitmap|. > > Also: s/we need to create copy/we need to create a copy/ Done. https://codereview.chromium.org/2399593005/diff/110001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:51: // It turns out that facedetector is not able to detect correctly if On 2016/10/11 23:04:48, mcasas wrote: > Same here and in l.48: |bitmap|, FaceDetector. > > nit: When a variable or data type is subject of the > phrase, it doesn't need "The": > // |bitmap| must be in 565 format... Done. https://codereview.chromium.org/2399593005/diff/110001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:52: // you simply use pixmap.setConfig(). I believe this is because On 2016/10/11 23:04:48, mcasas wrote: > Maybe s/I believe this is/This might be/ > Usually all code comments are written > in impersonal style ;) Done. https://codereview.chromium.org/2399593005/diff/110001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:54: // alphatype in original image is premultiplied. We can use getPixels() On 2016/10/11 23:04:49, mcasas wrote: > Here, not being subject, usually needs determinant: > ...alpha type in _the_ original_ image... Done. https://codereview.chromium.org/2399593005/diff/110001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:59: Bitmap bitmapUnpremul = Bitmap.createBitmap(pixels, width, height, Bitmap.Config.RGB_565); On 2016/10/11 23:04:48, mcasas wrote: > s/bitmpUnpremul/unPremultipliedBitmap/ > (no abbreviations in variable names). Done. https://codereview.chromium.org/2399593005/diff/130001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2399593005/diff/130001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2166: GetGlobalJavaInterfaces() On 2016/10/12 17:28:16, qinmin wrote: > nit: fix the indent here Done. https://codereview.chromium.org/2399593005/diff/130001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/130001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:27: On 2016/10/12 17:28:16, qinmin wrote: > nit: remove extra lone Done. https://codereview.chromium.org/2399593005/diff/130001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:99: public static class Factory implements InterfaceFactory<ShapeDetection> { On 2016/10/12 17:28:16, qinmin wrote: > is this class ever being used here? If not, moving it to a standalone file Done.
@qinmin ptal.
lgtm % nits https://codereview.chromium.org/2399593005/diff/150001/content/browser/frame_... File content/browser/frame_host/DEPS (right): https://codereview.chromium.org/2399593005/diff/150001/content/browser/frame_... content/browser/frame_host/DEPS:10: "+public/platform/modules/shapedetection/shapedetection.mojom.h" nit: order the include alphabetically https://codereview.chromium.org/2399593005/diff/150001/content/public/android... File content/public/android/BUILD.gn (right): https://codereview.chromium.org/2399593005/diff/150001/content/public/android... content/public/android/BUILD.gn:192: "java/src/org/chromium/content/browser/shapedetection/ShapeDetectionFactory.java", nit: fix the ordering here https://codereview.chromium.org/2399593005/diff/150001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionFactory.java (right): https://codereview.chromium.org/2399593005/diff/150001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionFactory.java:16: static final String TAG = "ShapeDetectionFactory"; nit: unused variable, remove it
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...
xianglu@chromium.org changed reviewers: + nick@chromium.org
@ncarter ptal.
Just some quick first-impression comments. I'm a little scared of this as attack surface, so I'd like to involve an experience IPC security reviewer (tsepez? dcheng?) in the discussion. https://codereview.chromium.org/2399593005/diff/170001/content/browser/frame_... File content/browser/frame_host/DEPS (right): https://codereview.chromium.org/2399593005/diff/170001/content/browser/frame_... content/browser/frame_host/DEPS:9: "+public/platform/modules/shapedetection/shapedetection.mojom.h", I'd put this in content/browser/DEPS with the other .mojom.h exceptions, so that they're all there. https://codereview.chromium.org/2399593005/diff/170001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2399593005/diff/170001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2190: ->CreateInterfaceFactory<blink::mojom::ShapeDetection>()); Is it possible to only do AddInterface if the flag to enable the feature is set? https://codereview.chromium.org/2399593005/diff/170001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:35: final int numBytes = width * height * 4; You definitely need a security review here. The parameters to |detectFace| need to be treated as untrusted, and validated. https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:44: bitmap.copyPixelsFromBuffer(imageBuffer); What thread does this run on? https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:58: Bitmap.createBitmap(pixels, width, height, Bitmap.Config.RGB_565); This looks really slow. Couldn't the renderer supply a preconverted 565 bitmap?
xianglu@chromium.org changed reviewers: + tsepez@chromium.org
xianglu@chromium.org changed reviewers: + tsepez@chromium.org
tsepez@ ptal.
tsepez@ ptal.
https://codereview.chromium.org/2399593005/diff/170001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:35: final int numBytes = width * height * 4; On 2016/10/12 20:08:35, ncarter wrote: > You definitely need a security review here. > > The parameters to |detectFace| need to be treated as untrusted, and validated. Exactly. For example, they could be < 0. Or the multiplication could overflow. https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:37: ByteBuffer imageBuffer = frameData.map(0, numBytes, MapFlags.none()); Even if the previous didn't overflow, I'd expect this call can fail and we'd want to check the result. Or does it throw? https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:38: Bitmap bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888); ditto here. https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:62: final int numberOfFaces = detector.findFaces(unPremultipliedBitmap, detectedFaces); does findFaces throw if the detectedFaces array isn't big enough? Or does it stop at 10?
Also: can you write a browsertest that exercises this codepath?
(a layouttest might suffice too. is it possible to transform the github example into a layout test? is an implementation of the face detection api available on our android bots?)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:190001) has been deleted
ptal. ncarter@, there is a layout test with mock service in third_party/WebKit/LayoutTests/shapedetection/detectface.html. For browsertest, since it will need real devices I think it is better to include it in a separate CL. https://codereview.chromium.org/2399593005/diff/170001/content/browser/frame_... File content/browser/frame_host/DEPS (right): https://codereview.chromium.org/2399593005/diff/170001/content/browser/frame_... content/browser/frame_host/DEPS:9: "+public/platform/modules/shapedetection/shapedetection.mojom.h", On 2016/10/12 20:08:35, ncarter wrote: > I'd put this in content/browser/DEPS with the other .mojom.h exceptions, so that > they're all there. Done. https://codereview.chromium.org/2399593005/diff/170001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2399593005/diff/170001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2190: ->CreateInterfaceFactory<blink::mojom::ShapeDetection>()); On 2016/10/12 20:08:35, ncarter wrote: > Is it possible to only do AddInterface if the flag to enable the feature is set? There is a flag in Blink https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/Runti... The flag is not visible here. But the shapedetection service won't be created until the client connects to it. https://codereview.chromium.org/2399593005/diff/170001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:37: ByteBuffer imageBuffer = frameData.map(0, numBytes, MapFlags.none()); On 2016/10/12 20:46:01, Tom Sepez wrote: > Even if the previous didn't overflow, I'd expect this call can fail and we'd > want to check the result. Or does it throw? Done. https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:38: Bitmap bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888); On 2016/10/12 20:46:01, Tom Sepez wrote: > ditto here. createBitmap() will throw IllegalArgumentException if the width or height are <= 0 https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:44: bitmap.copyPixelsFromBuffer(imageBuffer); It runs on the default thread. https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:58: Bitmap.createBitmap(pixels, width, height, Bitmap.Config.RGB_565); On 2016/10/12 20:08:35, ncarter wrote: > This looks really slow. Couldn't the renderer supply a preconverted 565 bitmap? The renderer can preconvert to 565, but SkImage library cannot generate an unpremultiplied format on output. So even the format is correct I think we still need a conversion here. https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:62: final int numberOfFaces = detector.findFaces(unPremultipliedBitmap, detectedFaces); On 2016/10/12 20:46:01, Tom Sepez wrote: > does findFaces throw if the detectedFaces array isn't big enough? Or does it > stop at 10? It will stop at 10.
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2399593005/diff/210001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/210001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:46: if (numBytes > 16777216 || numBytes < 0) { Sorry, this check isn't sufficient, consider 10764329 * 100 * 4 = 4305731600, which when truncated into 32 bits becomes 10764304 which is in the range above. You'll either have to divide first, or promote these to the java 64-bit type before multiplying.
tsepez@ ptal. https://codereview.chromium.org/2399593005/diff/210001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/210001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:46: if (numBytes > 16777216 || numBytes < 0) { On 2016/10/13 18:08:11, Tom Sepez wrote: > Sorry, this check isn't sufficient, consider > > 10764329 * 100 * 4 = 4305731600, which when truncated into > 32 bits becomes 10764304 which is in the range above. > > You'll either have to divide first, or promote these to the java 64-bit type > before multiplying. > > > > Sorry I should have been more careful with that. Thanks for the example.
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...
https://codereview.chromium.org/2399593005/diff/230001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:45: final long numBytes = ((long) width * height) * 4; If it's available here, java.lang.Math.multiplyExact() seems to be the Java way of doing multiplication with safe overflow behavior.
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...
ncarter@ ptal. https://codereview.chromium.org/2399593005/diff/230001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:45: final long numBytes = ((long) width * height) * 4; On 2016/10/13 18:43:08, ncarter wrote: > If it's available here, java.lang.Math.multiplyExact() seems to be the Java way > of doing multiplication with safe overflow behavior. I saw this method too. But handling exceptions here seems a little cumbersome to me. Please take a look at the newest patch. That might look better.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2399593005/diff/250001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/250001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:32: private static final int MOJO_SHAREDBUFFER_MAX_SIZE = 16 * 1024 * 1024; nit: can we call this MOJO_SHAREDBUFFER_MAX_BYTES to avoid size in bytes vs. size in pixels confusion?
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...
lgtm https://codereview.chromium.org/2399593005/diff/170001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java (right): https://codereview.chromium.org/2399593005/diff/170001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java:44: bitmap.copyPixelsFromBuffer(imageBuffer); On 2016/10/13 00:02:33, xianglu wrote: > It runs on the default thread. The worker pool sounds like a appropriate thread for this kind of operation. Doing image operations on the UI thread can cause jank and delays in processing input events. https://www.chromium.org/developers/design-documents/threading#Keeping_the_br... However, I understand this is a work in progress. If you think that's a worthwhile idea, please file a crbug to track it, and leave a TODO() in the code referencing the crbug #.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xianglu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org, qinmin@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2399593005/#ps270001 (title: "")
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 commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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...
xianglu@chromium.org changed reviewers: + haraken@chromium.org
@haraken ptal.
Patchset #9 (id:270001) has been deleted
modules/ LGTM
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
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, nick@chromium.org, mcasas@chromium.org, qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/2399593005/#ps290001 (title: "")
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 ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get Face positions. A temporary github demo is used for testing and visualization. Currently it can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get Face positions. A temporary github demo is used for testing and visualization. Currently it can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #9 (id:290001)
Message was sent while issue was closed.
Description was changed from ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get Face positions. A temporary github demo is used for testing and visualization. Currently it can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== ShapeDetectionImpl: Android implementation of detectFace() detectFace() method calls android FaceDetecor API to get Face positions. A temporary github demo is used for testing and visualization. Currently it can crash if the image comes from camera. BUG=646083 TEST=https://rawgit.com/Nadia-mint/chrome-demos/shapedetection/shapedetection.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/c43957493361040d68a66bd4df7d8693d954799c Cr-Commit-Position: refs/heads/master@{#425270} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c43957493361040d68a66bd4df7d8693d954799c Cr-Commit-Position: refs/heads/master@{#425270} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
