ShapeDetectionImpl: Android implementation of detectFace()

content/public/android/java/src/org/chromium/content/browser/shapedetection/ShapeDetectionImpl.java

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.content.browser.shapedetection;
+
+import android.graphics.Bitmap;
+import android.graphics.PointF;
+import android.media.FaceDetector;
+import android.media.FaceDetector.Face;
+
+import org.chromium.blink.mojom.FaceDetectionResult;
+import org.chromium.blink.mojom.ShapeDetection;
+import org.chromium.gfx.mojom.RectF;
+import org.chromium.mojo.system.MojoException;
+import org.chromium.mojo.system.SharedBufferHandle;
+import org.chromium.mojo.system.SharedBufferHandle.MapFlags;
+
+import java.nio.ByteBuffer;
+
+/**
+ * Android implementation of the shapedetection service defined in
+ * third_party/WebKit/public/platform/modules/shapedetection/shapedetection.mojom
+ */
+public class ShapeDetectionImpl implements ShapeDetection {
+    private static final String TAG = "ShapeDetectionImpl";
+    // By default, there is no limit in the number of faces detected.
+    private static final int MAX_FACES = 10;
+
+    public ShapeDetectionImpl() {}
+
+    @Override
+    public void detectFace(
+            SharedBufferHandle frameData, int width, int height, DetectFaceResponse callback) {
+        final int numBytes = width * height * 4;

[ncarter (slow), 2016/10/12 20:08:35]

You definitely need a security review here.

The parameters to |detectFace| need to be treated as untrusted, and validated.

[Tom Sepez, 2016/10/12 20:46:01]

Exactly.  For example, they could be < 0.  Or the multiplication could overflow.

+
+        ByteBuffer imageBuffer = frameData.map(0, numBytes, MapFlags.none());

[Tom Sepez, 2016/10/12 20:46:01]

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?

[xianglu, 2016/10/13 00:02:32]

Done.

+        Bitmap bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888);

[Tom Sepez, 2016/10/12 20:46:01]

ditto here.

[xianglu, 2016/10/13 00:02:32]

createBitmap() will throw IllegalArgumentException if the width or height are <=
0

+
+        // An int array is needed to construct a Bitmap. However the Bytebuffer
+        // we get from |sharedBufferHandle| is directly allocated and does not
+        // have a supporting array. Therefore we need to copy from |imageBuffer|
+        // to create this intermediate Bitmap.
+        bitmap.copyPixelsFromBuffer(imageBuffer);

[ncarter (slow), 2016/10/12 20:08:35]

What thread does this run on?

[xianglu, 2016/10/13 00:02:33]

It runs on the default thread.

[ncarter (slow), 2016/10/13 21:54:09]

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 #.

+
+        // A Bitmap must be in 565 format for findFaces() to work. See
+        // http://androidxref.com/7.0.0_r1/xref/frameworks/base/media/java/android/media/FaceDetector.java#124
+        //
+        // It turns out that FaceDetector is not able to detect correctly if
+        // simply using pixmap.setConfig(). The reason might be that findFaces()
+        // needs non-premultiplied ARGB arrangement, while the alpha type in the
+        // original image is premultiplied. We can use getPixels() which does
+        // the unmultiplication while copying to a new array. See
+        // http://androidxref.com/7.0.0_r1/xref/frameworks/base/graphics/java/android/graphics/Bitmap.java#538
+        int[] pixels = new int[width * height];
+        bitmap.getPixels(pixels, 0, width, 0, 0, width, height);
+        Bitmap unPremultipliedBitmap =
+                Bitmap.createBitmap(pixels, width, height, Bitmap.Config.RGB_565);

[ncarter (slow), 2016/10/12 20:08:35]

This looks really slow. Couldn't the renderer supply a preconverted 565 bitmap?

[xianglu, 2016/10/13 00:02:33]

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. 

+
+        FaceDetector detector = new FaceDetector(width, height, MAX_FACES);
+        Face[] detectedFaces = new Face[MAX_FACES];
+        final int numberOfFaces = detector.findFaces(unPremultipliedBitmap, detectedFaces);

[Tom Sepez, 2016/10/12 20:46:01]

does findFaces throw if the detectedFaces array isn't big enough?  Or does it
stop at 10?  

[xianglu, 2016/10/13 00:02:33]

It will stop at 10.

+
+        FaceDetectionResult faceDetectionResult = new FaceDetectionResult();
+        faceDetectionResult.boundingBoxes = new RectF[numberOfFaces];
+        for (int i = 0; i < numberOfFaces; i++) {
+            final Face face = detectedFaces[i];
+            final PointF midPoint = new PointF();
+            face.getMidPoint(midPoint);
+            final float eyesDistance = face.eyesDistance();
+
+            RectF boundingBox = new RectF();
+            boundingBox.x = midPoint.x - eyesDistance;
+            boundingBox.y = midPoint.y - eyesDistance;
+            boundingBox.width = 2 * eyesDistance;
+            boundingBox.height = 2 * eyesDistance;
+
+            // TODO(xianglu): Consider adding Face.confidence and Face.pose.
+            faceDetectionResult.boundingBoxes[i] = boundingBox;
+        }
+
+        callback.call(faceDetectionResult);
+    }
+
+    @Override
+    public void close() {}
+
+    @Override
+    public void onConnectionError(MojoException e) {
+        close();
+    }
+}