Revert of RELAND: ShapeDetection: use mojom::Bitmap for mojo interfac…

…e. (patchset chromium#2 id:20001 of https://codereview.chromium.org/2753413002/ ) Reason for revert: Broke shapedetection/detection-on-worker.html shapedetection/detection-security-test.html reliably: 18:14:35.412 4150 [10306:39683:0319/181429.446067:1229254778376:WARNING:url_request_job_manager.cc(90)] Failed to map: layout-test-mojom://content/shell/renderer/layout_test/frame_interface_registry 18:14:35.413 4150 [10306:39683:0319/181429.446194:1229254899173:WARNING:url_request_job_manager.cc(90)] Failed to map: layout-test-mojom://content/shell/renderer/layout_test/interface_registry 18:14:35.413 4150 [10308:771:0319/181429.450430:1229259136455:ERROR:mojo_context_state.cc(199)] Failed to fetch source for module "content/shell/renderer/layout_test/frame_interface_registry" 18:14:35.413 4150 [10308:771:0319/181429.450505:1229259209121:ERROR:mojo_context_state.cc(199)] Failed to fetch source for module "content/shell/renderer/layout_test/interface_registry" 18:14:35.413 4150 [10308:771:0319/181429.452582:1229261287723:ERROR:mojo_context_state.cc(199)] Failed to fetch source for module "skia/public/interfaces/bitmap.mojom" 18:14:35.414 4111 [46631/51882] shapedetection/detection-security-test.html failed unexpectedly (test timed out) Original issue's description: > RELAND: ShapeDetection: use mojom::Bitmap for mojo interface. > > Original CL got reverted due to shapedetection/detection-on-worker.html > fail to fetch source for module skia/public/interfaces/bitmap.mojom. > > This CL includes the generated JS bindings for skia/public/interfaces/bitmap.mojom > in the layout tests archive. > > Original CL description ------------------------------------------------ > ShapeDetection: use mojom::Bitmap for mojo interface. > > This CL uses mojo::Bitmap for mojo ShapeDetection interfaces definition, > so that the Detect API is completely flexible and friendly. > > BUG=665488 > TEST(Layout)= > third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-creation.html > third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-empty-input.html > third_party/WebKit/LayoutTests/shapedetection/detection-HTMLCanvasElement.html > third_party/WebKit/LayoutTests/shapedetection/detection-HTMLImageElement.html > third_party/WebKit/LayoutTests/shapedetection/detection-HTMLVideoElement.html > third_party/WebKit/LayoutTests/shapedetection/detection-ImageBitmap.html > third_party/WebKit/LayoutTests/shapedetection/detection-ImageData.html > > Review-Url: https://codereview.chromium.org/2629433003 > Cr-Commit-Position: refs/heads/master@{#446142} > (cherry picked from commit 33adf4c) > > Conflicts: > third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp > third_party/WebKit/Source/modules/shapedetection/TextDetector.cpp > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:mac_optional_gpu_tests_rel > > TBR=rockot@chromium.org, dtrainor@chromium.org, haraken@chromium.org, bsalomon@google.com, tsepez@chromium.org > since the gist of the CL hasn't changed. > > Review-Url: https://codereview.chromium.org/2681913003 > Cr-Original-Commit-Position: refs/heads/master@{#450582} > Review-Url: https://codereview.chromium.org/2753413002 > Cr-Commit-Position: refs/heads/master@{#458010} > Committed: https://chromium.googlesource.com/chromium/src/+/b1caeee1ee08c8b3bcaa423c8aa4a9f06e9ce40a TBR=bsalomon@google.com,dtrainor@chromium.org,haraken@chromium.org,rockot@chromium.org,tsepez@chromium.org,mcasas@chromium.org,dpranke@chromium.org,junwei.fu@intel.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=665488 Review-Url: https://codereview.chromium.org/2755393002 Cr-Commit-Position: refs/heads/master@{#458033}
Denger-Network · Mar 20, 2017 · 0919785 · 0919785
1 parent 531d81f
commit 0919785
Show file tree

Hide file tree

Showing 36 changed files with 252 additions and 164 deletions.
diff --git a/AUTHORS b/AUTHORS
@@ -225,7 +225,6 @@ Frankie Dintino <fdintino@theatlantic.com>
 Franklin Ta <fta2012@gmail.com>
 Frédéric Jacob <frederic.jacob.78@gmail.com>
 Frédéric Wang <fred.wang@free.fr>
-Fu Junwei <junwei.fu@intel.com>
 Gaetano Mendola <mendola@gmail.com>
 Gajendra N <gajendra.n@samsung.com>
 Gajendra Singh <wxjg68@motorola.com>

diff --git a/BUILD.gn b/BUILD.gn
@@ -899,7 +899,6 @@ if (!is_ios) {
     data_deps = [
       "//content/shell:content_shell",
       "//mojo/public/interfaces/bindings/tests:test_interfaces",
-      "//skia/public/interfaces",
       "//third_party/WebKit/public:blink_devtools_frontend_resources_files",
       "//third_party/mesa:osmesa",
       "//tools/imagediff",

diff --git a/chrome/android/BUILD.gn b/chrome/android/BUILD.gn
@@ -213,7 +213,6 @@ android_library("chrome_java") {
     "//services/service_manager/public/interfaces:interfaces_java",
     "//services/service_manager/public/java:service_manager_java",
     "//services/shape_detection/public/interfaces:interfaces_java",
-    "//skia/public/interfaces:interfaces_java",
     "//third_party/WebKit/public:android_mojo_bindings_java",
     "//third_party/WebKit/public:blink_headers_java",
     "//third_party/WebKit/public:mojo_bindings_java",

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java b/chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java
@@ -20,10 +20,11 @@
 import org.chromium.gfx.mojom.PointF;
 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 org.chromium.services.service_manager.InterfaceFactory;
 import org.chromium.shape_detection.mojom.BarcodeDetection;
 import org.chromium.shape_detection.mojom.BarcodeDetectionResult;
-import org.chromium.skia.mojom.ColorType;
 
 import java.nio.ByteBuffer;
 
@@ -42,7 +43,8 @@ public BarcodeDetectionImpl(Context context) {
     }
 
     @Override
-    public void detect(org.chromium.skia.mojom.Bitmap bitmapData, DetectResponse callback) {
+    public void detect(
+            SharedBufferHandle frameData, int width, int height, DetectResponse callback) {
         if (!ExternalAuthUtils.getInstance().canUseGooglePlayServices(
                     mContext, new UserRecoverableErrorHandler.Silent())) {
             Log.e(TAG, "Google Play Services not available");
@@ -59,29 +61,17 @@ public void detect(org.chromium.skia.mojom.Bitmap bitmapData, DetectResponse cal
             return;
         }
 
-        // TODO(junwei.fu): Consider supporting other bitmap pixel formats,
-        // https://crbug.com/684921.
-        if (bitmapData.colorType != ColorType.RGBA_8888
-                && bitmapData.colorType != ColorType.BGRA_8888) {
-            Log.e(TAG, "Unsupported bitmap pixel format");
-            callback.call(new BarcodeDetectionResult[0]);
-            return;
-        }
-
-        int width = bitmapData.width;
-        int height = bitmapData.height;
         final long numPixels = (long) width * height;
         // TODO(mcasas): https://crbug.com/670028 homogeneize overflow checking.
-        if (bitmapData.pixelData == null || width <= 0 || height <= 0
-                || numPixels > (Long.MAX_VALUE / 4)) {
+        if (!frameData.isValid() || width <= 0 || height <= 0 || numPixels > (Long.MAX_VALUE / 4)) {
             callback.call(new BarcodeDetectionResult[0]);
             return;
         }
 
         // Mapping |frameData| will fail if the intended mapped size is larger
         // than its actual capacity, which is limited by the appropriate
         // mojo::edk::Configuration entry.
-        ByteBuffer imageBuffer = ByteBuffer.wrap(bitmapData.pixelData);
+        ByteBuffer imageBuffer = frameData.map(0, numPixels * 4, MapFlags.none());
         if (imageBuffer.capacity() <= 0) {
             callback.call(new BarcodeDetectionResult[0]);
             return;

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/shapedetection/TextDetectionImpl.java b/chrome/android/java/src/org/chromium/chrome/browser/shapedetection/TextDetectionImpl.java
@@ -18,10 +18,11 @@
 import org.chromium.chrome.browser.externalauth.UserRecoverableErrorHandler;
 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 org.chromium.services.service_manager.InterfaceFactory;
 import org.chromium.shape_detection.mojom.TextDetection;
 import org.chromium.shape_detection.mojom.TextDetectionResult;
-import org.chromium.skia.mojom.ColorType;
 
 import java.nio.ByteBuffer;
 
@@ -40,7 +41,8 @@ public TextDetectionImpl(Context context) {
     }
 
     @Override
-    public void detect(org.chromium.skia.mojom.Bitmap bitmapData, DetectResponse callback) {
+    public void detect(
+            SharedBufferHandle frameData, int width, int height, DetectResponse callback) {
         if (!ExternalAuthUtils.getInstance().canUseGooglePlayServices(
                     mContext, new UserRecoverableErrorHandler.Silent())) {
             Log.e(TAG, "Google Play Services not available");
@@ -57,29 +59,17 @@ public void detect(org.chromium.skia.mojom.Bitmap bitmapData, DetectResponse cal
             return;
         }
 
-        // TODO(junwei.fu): Consider supporting other bitmap pixel formats,
-        // https://crbug.com/684921.
-        if (bitmapData.colorType != ColorType.RGBA_8888
-                && bitmapData.colorType != ColorType.BGRA_8888) {
-            Log.e(TAG, "Unsupported bitmap pixel format");
-            callback.call(new TextDetectionResult[0]);
-            return;
-        }
-
-        int width = bitmapData.width;
-        int height = bitmapData.height;
         final long numPixels = (long) width * height;
         // TODO(xianglu): https://crbug.com/670028 homogeneize overflow checking.
-        if (bitmapData.pixelData == null || width <= 0 || height <= 0
-                || numPixels > (Long.MAX_VALUE / 4)) {
+        if (!frameData.isValid() || width <= 0 || height <= 0 || numPixels > (Long.MAX_VALUE / 4)) {
             callback.call(new TextDetectionResult[0]);
             return;
         }
 
         // Mapping |frameData| will fail if the intended mapped size is larger
         // than its actual capacity, which is limited by the appropriate
         // mojo::edk::Configuration entry.
-        ByteBuffer imageBuffer = ByteBuffer.wrap(bitmapData.pixelData);
+        ByteBuffer imageBuffer = frameData.map(0, numPixels * 4, MapFlags.none());
         if (imageBuffer.capacity() <= 0) {
             callback.call(new TextDetectionResult[0]);
             return;

diff --git a/content/public/android/BUILD.gn b/content/public/android/BUILD.gn
@@ -60,7 +60,6 @@ android_library("content_java") {
     "//services/service_manager/public/interfaces:interfaces_java",
     "//services/service_manager/public/java:service_manager_java",
     "//services/shape_detection/public/interfaces:interfaces_java",
-    "//skia/public/interfaces:interfaces_java",
     "//third_party/WebKit/public:blink_headers_java",
     "//third_party/WebKit/public:mojo_bindings_java",
     "//third_party/android_tools:android_support_annotations_java",

diff --git a/...ublic/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java b/...ublic/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java
@@ -12,10 +12,11 @@
 import org.chromium.base.Log;
 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 org.chromium.shape_detection.mojom.FaceDetection;
 import org.chromium.shape_detection.mojom.FaceDetectionResult;
 import org.chromium.shape_detection.mojom.FaceDetectorOptions;
-import org.chromium.skia.mojom.ColorType;
 
 import java.nio.ByteBuffer;
 
@@ -35,42 +36,29 @@ public class FaceDetectionImpl implements FaceDetection {
     }
 
     @Override
-    public void detect(org.chromium.skia.mojom.Bitmap bitmapData, DetectResponse callback) {
-        int width = bitmapData.width;
-        int height = bitmapData.height;
+    public void detect(
+            SharedBufferHandle frameData, int width, int height, DetectResponse callback) {
         final long numPixels = (long) width * height;
         // TODO(xianglu): https://crbug.com/670028 homogeneize overflow checking.
-        if (bitmapData.pixelData == null || width <= 0 || height <= 0
-                || numPixels > (Long.MAX_VALUE / 4)) {
+        if (!frameData.isValid() || width <= 0 || height <= 0 || numPixels > (Long.MAX_VALUE / 4)) {
             Log.d(TAG, "Invalid argument(s).");
             callback.call(new FaceDetectionResult());
             return;
         }
 
-        // TODO(junwei.fu): Consider supporting other bitmap pixel formats,
-        // https://crbug.com/684921.
-        if (bitmapData.colorType != ColorType.RGBA_8888
-                && bitmapData.colorType != ColorType.BGRA_8888) {
-            Log.e(TAG, "Unsupported bitmap pixel format");
-            callback.call(new FaceDetectionResult());
-            return;
-        }
-
-        ByteBuffer imageBuffer = ByteBuffer.wrap(bitmapData.pixelData);
+        ByteBuffer imageBuffer = frameData.map(0, numPixels * 4, MapFlags.none());
         if (imageBuffer.capacity() <= 0) {
-            Log.d(TAG, "Failed to wrap from Bitmap.");
+            Log.d(TAG, "Failed to map from SharedBufferHandle.");
             callback.call(new FaceDetectionResult());
             return;
         }
 
-        // TODO(junwei.fu): Use |bitmapData| directly for |unPremultipliedBitmap| to spare a copy
-        // if the bitmap pixel format is RGB_565, the ARGB_8888 Bitmap doesn't need to be created
-        // in this case, https://crbug.com/684930.
         Bitmap bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888);
 
         // An int array is needed to construct a Bitmap. However the Bytebuffer
-        // we get from |bitmapData| is directly allocated and does not have a supporting array.
-        // Therefore we need to copy from |imageBuffer| to create this intermediate Bitmap.
+        // 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.
         // TODO(xianglu): Consider worker pool as appropriate threads.
         // http://crbug.com/655814
         bitmap.copyPixelsFromBuffer(imageBuffer);

diff --git a/services/shape_detection/DEPS b/services/shape_detection/DEPS
@@ -1,6 +1,5 @@
 include_rules = [
   "+media/capture/video/scoped_result_callback.h",
-  "+skia/ext/skia_utils_mac.h",
   "+third_party/skia/include",
   "+ui/gfx/codec",
   "+ui/gl/gl_switches.h"

diff --git a/services/shape_detection/barcode_detection_impl_mac.h b/services/shape_detection/barcode_detection_impl_mac.h
@@ -7,7 +7,6 @@
 
 #include "base/mac/scoped_nsobject.h"
 #include "services/shape_detection/public/interfaces/barcodedetection.mojom.h"
-#include "third_party/skia/include/core/SkBitmap.h"
 
 @class CIDetector;
 
@@ -19,7 +18,9 @@ class BarcodeDetectionImplMac
   BarcodeDetectionImplMac();
   ~BarcodeDetectionImplMac() override;
 
-  void Detect(const SkBitmap& bitmap,
+  void Detect(mojo::ScopedSharedBufferHandle frame_data,
+              uint32_t width,
+              uint32_t height,
               const shape_detection::mojom::BarcodeDetection::DetectCallback&
                   callback) override;
 

diff --git a/services/shape_detection/barcode_detection_impl_mac.mm b/services/shape_detection/barcode_detection_impl_mac.mm
@@ -50,20 +50,22 @@ void RunCallbackWithNoBarcodes(
 
 BarcodeDetectionImplMac::~BarcodeDetectionImplMac() {}
 
-void BarcodeDetectionImplMac::Detect(const SkBitmap& bitmap,
+void BarcodeDetectionImplMac::Detect(mojo::ScopedSharedBufferHandle frame_data,
+                                     uint32_t width,
+                                     uint32_t height,
                                      const DetectCallback& callback) {
   media::ScopedResultCallback<DetectCallback> scoped_callback(
       base::Bind(&RunCallbackWithBarcodes, callback),
       base::Bind(&RunCallbackWithNoBarcodes));
 
-  base::scoped_nsobject<CIImage> ci_image = CreateCIImageFromSkBitmap(bitmap);
+  base::scoped_nsobject<CIImage> ci_image =
+      CreateCIImageFromSharedMemory(std::move(frame_data), width, height);
   if (!ci_image)
     return;
 
   NSArray* const features = [detector_ featuresInImage:ci_image];
 
   std::vector<mojom::BarcodeDetectionResultPtr> results;
-  const int height = bitmap.height();
   for (CIQRCodeFeature* const f in features) {
     shape_detection::mojom::BarcodeDetectionResultPtr result =
         shape_detection::mojom::BarcodeDetectionResult::New();

diff --git a/services/shape_detection/barcode_detection_impl_mac_unittest.mm b/services/shape_detection/barcode_detection_impl_mac_unittest.mm
@@ -12,7 +12,6 @@
 #include "base/run_loop.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "third_party/skia/include/utils/mac/SkCGUtils.h"
 #include "ui/gl/gl_switches.h"
 
 namespace shape_detection {
@@ -65,6 +64,7 @@ void DetectCallback(std::vector<mojom::BarcodeDetectionResultPtr> results) {
 
   const gfx::Size size([qr_code_image extent].size.width,
                        [qr_code_image extent].size.height);
+  const int num_bytes = size.GetArea() * 4 /* bytes per pixel */;
 
   base::scoped_nsobject<CIContext> context([[CIContext alloc] init]);
 
@@ -73,16 +73,30 @@ void DetectCallback(std::vector<mojom::BarcodeDetectionResultPtr> results) {
   EXPECT_EQ(static_cast<size_t>(size.width()), CGImageGetWidth(cg_image));
   EXPECT_EQ(static_cast<size_t>(size.height()), CGImageGetHeight(cg_image));
 
-  SkBitmap bitmap;
-  ASSERT_TRUE(SkCreateBitmapFromCGImage(&bitmap, cg_image));
+  base::ScopedCFTypeRef<CFDataRef> raw_cg_image_data(
+      CGDataProviderCopyData(CGImageGetDataProvider(cg_image)));
+  EXPECT_TRUE(CFDataGetBytePtr(raw_cg_image_data));
+  EXPECT_EQ(num_bytes, CFDataGetLength(raw_cg_image_data));
+
+  // Generate a new ScopedSharedBufferHandle of the aproppriate size, map it and
+  // copy the generated qr code image pixels into it.
+  mojo::ScopedSharedBufferHandle handle =
+      mojo::SharedBufferHandle::Create(num_bytes);
+  ASSERT_TRUE(handle->is_valid());
+
+  mojo::ScopedSharedBufferMapping mapping = handle->Map(num_bytes);
+  ASSERT_TRUE(mapping);
+
+  memcpy(mapping.get(), CFDataGetBytePtr(raw_cg_image_data), num_bytes);
 
   base::RunLoop run_loop;
   base::Closure quit_closure = run_loop.QuitClosure();
   // Send the image Detect() and expect the response in callback.
   EXPECT_CALL(*this, Detection(1, kInfoString))
       .WillOnce(RunClosure(quit_closure));
-  impl_.Detect(bitmap, base::Bind(&BarcodeDetectionImplMacTest::DetectCallback,
-                                  base::Unretained(this)));
+  impl_.Detect(std::move(handle), size.width(), size.height(),
+               base::Bind(&BarcodeDetectionImplMacTest::DetectCallback,
+                          base::Unretained(this)));
 
   run_loop.Run();
 }

diff --git a/services/shape_detection/detection_utils_mac.h b/services/shape_detection/detection_utils_mac.h
@@ -9,14 +9,15 @@
 
 #include "base/mac/scoped_nsobject.h"
 #include "services/shape_detection/public/interfaces/barcodedetection.mojom.h"
-#include "third_party/skia/include/core/SkBitmap.h"
 
 namespace shape_detection {
 
 // Takes a ScopedSharedBufferHandle with dimensions and produces a new CIImage
 // with the same contents, or a null scoped_nsobject is something goes wrong.
-base::scoped_nsobject<CIImage> CreateCIImageFromSkBitmap(
-    const SkBitmap& bitmap);
+base::scoped_nsobject<CIImage> CreateCIImageFromSharedMemory(
+    mojo::ScopedSharedBufferHandle frame_data,
+    uint32_t width,
+    uint32_t height);
 
 }  // namespace shape_detection