Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629433003:
ShapeDetection: use mojom::Bitmap for mojo interface. (Closed)
|
Created:
3 years, 11 months ago by junwei Modified:
3 years, 10 months ago Reviewers:
dominickn, Ken Rockot(use gerrit already), David Trainor- moved to gerrit, timvolodine, haraken, leonhsl(Using Gerrit), mcasas, yzshen1, xianglu, Tom Sepez CC:
Aaron Boodman, abarth-chromium, agrieve+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShapeDetection: 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}
Committed: https://chromium.googlesource.com/chromium/src/+/33adf4c85dd553eb607bcf9c02c5fa60c8738a68
Patch Set 1 : ShapeDetection: use mojom::Bitmap for mojo interface. #
Total comments: 4
Patch Set 2 : ShapeDetection: use mojom::Bitmap for mojo interface. #
Total comments: 4
Patch Set 3 : ShapeDetection: use mojom::Bitmap for mojo interface. #
Total comments: 8
Patch Set 4 : ShapeDetection: use mojom::Bitmap for mojo interface. #
Total comments: 2
Patch Set 5 : ShapeDetection: use mojom::Bitmap for mojo interface. #
Total comments: 2
Patch Set 6 : ShapeDetection: use mojom::Bitmap for mojo interface. #
Total comments: 4
Messages
Total messages: 168 (126 generated)
The CQ bit was checked by leon.han@intel.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by junwei.fu@intel.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by leon.han@intel.com 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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by leon.han@intel.com 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: 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 leon.han@intel.com 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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by leon.han@intel.com 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_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
junwei.fu@intel.com changed reviewers: + mcasas@chromium.org, xianglu@chromium.org, yzshen@chromium.org
I am trying to use mojo::Bitmap for ShapeDetection interfaces definition, but the layout tests are failed to time out on try bots, and it's also failed with the latest code without the CL on my local testing. The status of mockFaceDetectionProviderReady is pending after debugging with inspect. Any help would be great.
On 2017/01/16 05:35:59, junwei.fu wrote: > I am trying to use mojo::Bitmap for ShapeDetection interfaces definition, but > the layout tests are failed to time out on try bots, and it's also failed with > the latest code without the CL on my local testing. The status of > mockFaceDetectionProviderReady is pending after debugging with inspect. Any help > would be great. ï¼·ould you please specify which part you want me to look at? Thanks!
https://codereview.chromium.org/2629433003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js (right): https://codereview.chromium.org/2629433003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:48: this.buffer_data_ = new Uint32Array(image.pixel_data.buffer); I think just image.pixel_data is needed to construct a new array here. And since pixel_data is a uint8 vector, we might need to do some conversion before creating a Uint32Array?
On 2017/01/17 21:53:20, xianglu wrote: > https://codereview.chromium.org/2629433003/diff/80001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js > (right): > > https://codereview.chromium.org/2629433003/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:48: > this.buffer_data_ = new Uint32Array(image.pixel_data.buffer); > I think just image.pixel_data is needed to construct a new array here. And since > pixel_data is a uint8 vector, we might need to do some conversion before > creating a Uint32Array? Please rewrite the CL description to read what it changes, e.g.: ShapeDetection: use mojom::Bitmap for mojo interface. Then describe why you do the change in the CL description, and how it's being tested. Also, please remove all PatchSets with no reviewers' comments (expand the PS, then click on "delete" on the files area upper right corner).
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Patchset #1 (id:80001) has been deleted
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 uses mojo::Bitmap for mojo ShapeDetection interfaces definition.
BUG=665488
==========
to
==========
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
==========
Description was changed from
==========
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
==========
to
==========
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
==========
Description was changed from
==========
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
==========
to
==========
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
==========
Description was changed from
==========
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
==========
to
==========
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
==========
On 2017/01/18 00:25:56, mcasas wrote: > On 2017/01/17 21:53:20, xianglu wrote: > > > https://codereview.chromium.org/2629433003/diff/80001/third_party/WebKit/Layo... > > File > > third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js > > (right): > > > > > https://codereview.chromium.org/2629433003/diff/80001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:48: > > this.buffer_data_ = new Uint32Array(image.pixel_data.buffer); > > I think just image.pixel_data is needed to construct a new array here. And > since > > pixel_data is a uint8 vector, we might need to do some conversion before > > creating a Uint32Array? > > Please rewrite the CL description to read what it changes, e.g.: > ShapeDetection: use mojom::Bitmap for mojo interface. > Then describe why you do the change in the CL description, and > how it's being tested. > > Also, please remove all PatchSets with no reviewers' comments > (expand the PS, then click on "delete" on the files area upper right > corner). Thanks, done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/17 21:53:20, xianglu wrote: > https://codereview.chromium.org/2629433003/diff/80001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js > (right): > > https://codereview.chromium.org/2629433003/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:48: > this.buffer_data_ = new Uint32Array(image.pixel_data.buffer); > I think just image.pixel_data is needed to construct a new array here. And since > pixel_data is a uint8 vector, we might need to do some conversion before > creating a Uint32Array? Thanks, it's passed.
On 2017/01/17 17:59:31, yzshen1 wrote: > On 2017/01/16 05:35:59, junwei.fu wrote: > > I am trying to use mojo::Bitmap for ShapeDetection interfaces definition, but > > the layout tests are failed to time out on try bots, and it's also failed with > > the latest code without the CL on my local testing. The status of > > mockFaceDetectionProviderReady is pending after debugging with inspect. Any > help > > would be great. > > ï¼·ould you please specify which part you want me to look at? Thanks! Thanks,xianglu has helped me to resolve the failed test cases.
On 2017/01/17 17:59:31, yzshen1 wrote: > On 2017/01/16 05:35:59, junwei.fu wrote: > > I am trying to use mojo::Bitmap for ShapeDetection interfaces definition, but > > the layout tests are failed to time out on try bots, and it's also failed with > > the latest code without the CL on my local testing. The status of > > mockFaceDetectionProviderReady is pending after debugging with inspect. Any > help > > would be great. > > ï¼·ould you please specify which part you want me to look at? Thanks! Thanks,xianglu has helped me to resolve the failed test cases.
tsepez@chromium.org changed reviewers: + tsepez@chromium.org
lgtm
Please also copy the subject to the first line of CL description. This is because CL titles usually disappear in bugTracker(bugs.chromium.org). https://codereview.chromium.org/2629433003/diff/100001/services/shape_detecti... File services/shape_detection/public/interfaces/barcodedetection.mojom (right): https://codereview.chromium.org/2629433003/diff/100001/services/shape_detecti... services/shape_detection/public/interfaces/barcodedetection.mojom:21: // |frame_data| contains tightly packed image pixels in ARGB32 format, Change this comment accordingly. Same for other .mojom files https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right): https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:116: WTF::Vector<uint8_t> data; nit: Maybe we can call it something like "bitmapData", so there's less confusion at l.138 as well. Same for l.187. https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.h (right): https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/ShapeDetector.h:13: #include "services/shape_detection/public/interfaces/facedetection.mojom-blink.h" nit: I might use "#include "skia/public/interfaces/bitmap.mojom-blink.h". https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/BUILD.gn (right): https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/BUILD.gn:675: "//skia/public/interfaces", Remove this.
Description was changed from
==========
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
==========
to
==========
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
==========
On 2017/01/18 21:08:38, xianglu wrote: > Please also copy the subject to the first line of CL description. This is > because CL titles usually disappear in bugTracker(bugs.chromium.org). Thanks,Done. > https://codereview.chromium.org/2629433003/diff/100001/services/shape_detecti... > File services/shape_detection/public/interfaces/barcodedetection.mojom (right): > > https://codereview.chromium.org/2629433003/diff/100001/services/shape_detecti... > services/shape_detection/public/interfaces/barcodedetection.mojom:21: // > |frame_data| contains tightly packed image pixels in ARGB32 format, > Change this comment accordingly. Same for other .mojom files Thanks,Done. > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right): > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:116: > WTF::Vector<uint8_t> data; > nit: Maybe we can call it something like "bitmapData", so there's less confusion > at l.138 as well. Same for l.187. Thanks,Done. > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.h (right): > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/shapedetection/ShapeDetector.h:13: #include > "services/shape_detection/public/interfaces/facedetection.mojom-blink.h" > nit: I might use "#include "skia/public/interfaces/bitmap.mojom-blink.h". The below errors will be shown when uploading cl. ** Presubmit ERRORS ** You added one or more #includes that violate checkdeps rules. third_party/WebKit/Source/modules/shapedetection/ShapeDetector.h Illegal include: "skia/public/interfaces/bitmap.mojom-blink.h" Because of no rule applying. > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/pub... > File third_party/WebKit/public/BUILD.gn (right): > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/pub... > third_party/WebKit/public/BUILD.gn:675: "//skia/public/interfaces", > Remove this. Thanks, Done.
On 2017/01/19 02:22:08, junwei.fu wrote: > On 2017/01/18 21:08:38, xianglu wrote: > > Please also copy the subject to the first line of CL description. This is > > because CL titles usually disappear in bugTracker(bugs.chromium.org). > Thanks,Done. > > > > https://codereview.chromium.org/2629433003/diff/100001/services/shape_detecti... > > File services/shape_detection/public/interfaces/barcodedetection.mojom > (right): > > > > > https://codereview.chromium.org/2629433003/diff/100001/services/shape_detecti... > > services/shape_detection/public/interfaces/barcodedetection.mojom:21: // > > |frame_data| contains tightly packed image pixels in ARGB32 format, > > Change this comment accordingly. Same for other .mojom files > Thanks,Done. > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp > (right): > > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:116: > > WTF::Vector<uint8_t> data; > > nit: Maybe we can call it something like "bitmapData", so there's less > confusion > > at l.138 as well. Same for l.187. > Thanks,Done. > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.h (right): > > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/shapedetection/ShapeDetector.h:13: #include > > "services/shape_detection/public/interfaces/facedetection.mojom-blink.h" > > nit: I might use "#include "skia/public/interfaces/bitmap.mojom-blink.h". > The below errors will be shown when uploading cl. > > ** Presubmit ERRORS ** > You added one or more #includes that violate checkdeps rules. > third_party/WebKit/Source/modules/shapedetection/ShapeDetector.h > Illegal include: "skia/public/interfaces/bitmap.mojom-blink.h" > Because of no rule applying. > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/pub... > > File third_party/WebKit/public/BUILD.gn (right): > > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/pub... > > third_party/WebKit/public/BUILD.gn:675: "//skia/public/interfaces", > > Remove this. > Thanks, Done. You need to add that rule into third_party/WebKit/Source/modules/shapedetection/DEPS
On 2017/01/19 03:00:32, xianglu wrote: > On 2017/01/19 02:22:08, junwei.fu wrote: > > On 2017/01/18 21:08:38, xianglu wrote: > > > Please also copy the subject to the first line of CL description. This is > > > because CL titles usually disappear in bugTracker(bugs.chromium.org). > > Thanks,Done. > > > > > > > > https://codereview.chromium.org/2629433003/diff/100001/services/shape_detecti... > > > File services/shape_detection/public/interfaces/barcodedetection.mojom > > (right): > > > > > > > > > https://codereview.chromium.org/2629433003/diff/100001/services/shape_detecti... > > > services/shape_detection/public/interfaces/barcodedetection.mojom:21: // > > > |frame_data| contains tightly packed image pixels in ARGB32 format, > > > Change this comment accordingly. Same for other .mojom files > > Thanks,Done. > > > > > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp > > (right): > > > > > > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:116: > > > WTF::Vector<uint8_t> data; > > > nit: Maybe we can call it something like "bitmapData", so there's less > > confusion > > > at l.138 as well. Same for l.187. > > Thanks,Done. > > > > > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.h > (right): > > > > > > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/modules/shapedetection/ShapeDetector.h:13: > #include > > > "services/shape_detection/public/interfaces/facedetection.mojom-blink.h" > > > nit: I might use "#include "skia/public/interfaces/bitmap.mojom-blink.h". > > The below errors will be shown when uploading cl. > > > > ** Presubmit ERRORS ** > > You added one or more #includes that violate checkdeps rules. > > third_party/WebKit/Source/modules/shapedetection/ShapeDetector.h > > Illegal include: "skia/public/interfaces/bitmap.mojom-blink.h" > > Because of no rule applying. > > > > > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/pub... > > > File third_party/WebKit/public/BUILD.gn (right): > > > > > > > > > https://codereview.chromium.org/2629433003/diff/100001/third_party/WebKit/pub... > > > third_party/WebKit/public/BUILD.gn:675: "//skia/public/interfaces", > > > Remove this. > > Thanks, Done. > > You need to add that rule into > third_party/WebKit/Source/modules/shapedetection/DEPS Thanks, Done.
Patchset #2 (id:120001) has been deleted
LGTM
The CQ bit was checked by leon.han@intel.com 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2629433003/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java (right): https://codereview.chromium.org/2629433003/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:62: // TODO(xianglu): Consider worker pool as appropriate threads. Please update this comment. https://codereview.chromium.org/2629433003/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:64: bitmap.copyPixelsFromBuffer(imageBuffer); Here you're copying |imageBuffer| into |bitmap| which is initialized as Bitmap.Config.ARGB_8888 in l.56, but that's only correct if bitmapData.colorType is ARGB_8888 itself. You need to consider this here and in the other .java detector implementations. OTOH if you read l.66, findFaces() works in RGB_565, so if the incoming |bitmapData| is in that format, we can skip the intermediate imageBuffer --> bitmap --> unpremultipliedBitmap step, and go directly imageBuffer --> unpremultipliedBitmap, assuming |imageBuffer| is directly accessible. https://codereview.chromium.org/2629433003/diff/140001/services/shape_detecti... File services/shape_detection/public/interfaces/barcodedetection.mojom (right): https://codereview.chromium.org/2629433003/diff/140001/services/shape_detecti... services/shape_detection/public/interfaces/barcodedetection.mojom:21: // |bitmap_data| contains tightly packed image pixels in ARGB32 format, "in ARGB32 format" is incorrect, since mojom.Bitmap can have different pixel formats. Also correct the other .mojom files. https://codereview.chromium.org/2629433003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right): https://codereview.chromium.org/2629433003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:115: skia::mojom::blink::BitmapPtr bitmap = skia::mojom::blink::Bitmap::New(); Using Bitmap::New() will leave some fields initialized to their default values, in particular ColorType will be UNKNOWN [1], which is incorrect. You might like to take a look at [2] to see which is the byte ordering of the system. This comment also applies to the other Bitmap::New() calls in this file. Plz consider moving such creation to a static function in an anonymous namespace in this file. [1] https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap.mojom?q=fi... [2] https://cs.chromium.org/chromium/src/third_party/skia/include/core/SkImageInf...
The CQ bit was checked by leon.han@intel.com 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...
On 2017/01/19 18:09:48, mcasas wrote: > https://codereview.chromium.org/2629433003/diff/140001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java > (right): > > https://codereview.chromium.org/2629433003/diff/140001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:62: > // TODO(xianglu): Consider worker pool as appropriate threads. > Please update this comment. Thanks, Done. > https://codereview.chromium.org/2629433003/diff/140001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:64: > bitmap.copyPixelsFromBuffer(imageBuffer); > Here you're copying |imageBuffer| into |bitmap| which is initialized > as Bitmap.Config.ARGB_8888 in l.56, but that's only correct if > bitmapData.colorType is ARGB_8888 itself. You need to consider > this here and in the other .java detector implementations. > > OTOH if you read l.66, findFaces() works in RGB_565, so if the > incoming |bitmapData| is in that format, we can skip the intermediate > imageBuffer --> bitmap --> unpremultipliedBitmap step, and > go directly imageBuffer --> unpremultipliedBitmap, assuming > |imageBuffer| is directly accessible. Thanks, Done. > https://codereview.chromium.org/2629433003/diff/140001/services/shape_detecti... > File services/shape_detection/public/interfaces/barcodedetection.mojom (right): > > https://codereview.chromium.org/2629433003/diff/140001/services/shape_detecti... > services/shape_detection/public/interfaces/barcodedetection.mojom:21: // > |bitmap_data| contains tightly packed image pixels in ARGB32 format, > "in ARGB32 format" is incorrect, since mojom.Bitmap can have > different pixel formats. Also correct the other .mojom files. Thanks, Done. > https://codereview.chromium.org/2629433003/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right): > > https://codereview.chromium.org/2629433003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:115: > skia::mojom::blink::BitmapPtr bitmap = skia::mojom::blink::Bitmap::New(); > Using Bitmap::New() will leave some fields initialized > to their default values, in particular ColorType will > be UNKNOWN [1], which is incorrect. You might like to > take a look at [2] to see which is the byte ordering of > the system. This comment also applies to the other > Bitmap::New() calls in this file. Plz consider moving > such creation to a static function in an anonymous > namespace in this file. > > [1] > https://cs.chromium.org/chromium/src/skia/public/interfaces/bitmap.mojom?q=fi... > [2] > https://cs.chromium.org/chromium/src/third_party/skia/include/core/SkImageInf... Thanks, Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by junwei.fu@intel.com 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 checked by junwei.fu@intel.com 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:180001) has been deleted
Patchset #3 (id:160001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2629433003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2629433003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:86: || numPixels > (Long.MAX_VALUE / 4) || !validConfig(bitmapData.colorType)) { I doubt that any detector (this or TextDetectionImpl) would work with anything but 32bpp variations (the 8888 ones). Also, Blink sends one of two formats: BGRA_8888/RGBA_8888 but I don't see those two being handled or a note as to why that isn't the case. https://codereview.chromium.org/2629433003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/TextDetectionImpl.java (right): https://codereview.chromium.org/2629433003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/TextDetectionImpl.java:84: || numPixels > (Long.MAX_VALUE / 4) || !validConfig(bitmapData.colorType)) { Same comments here as in BarcodeDetectionImpl. https://codereview.chromium.org/2629433003/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java (right): https://codereview.chromium.org/2629433003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:32: private Config mBitmapConfig; We don't need a member variable, validConfig() could return Config.UNKNOWN to the caller. https://codereview.chromium.org/2629433003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:64: || numPixels > (Long.MAX_VALUE / 4) || !validConfig(bitmapData.colorType)) { Same comments as in Barcode/TextDetectionImpl regarding the handled pixel formats. https://codereview.chromium.org/2629433003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:78: if (bitmapData.colorType == ColorType.RGB_565) { If blink is never sending this format, how could we test this branch? https://codereview.chromium.org/2629433003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:82: // we get from |Bitmap| is directly allocated and does not have a supporting array. || in comments applies to variables, not types, so here I think you mean |bitmapData|. https://codereview.chromium.org/2629433003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right): https://codereview.chromium.org/2629433003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:26: int height, get is usually reserved for getters/accessors and in this case we are creating, and create needs a 'from', so: s/getBitmapOnData/createBitmapFromData/ ?
The CQ bit was checked by junwei.fu@intel.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
On 2017/01/24 00:52:06, mcasas wrote: > https://codereview.chromium.org/2629433003/diff/200001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java > (right): > > https://codereview.chromium.org/2629433003/diff/200001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:86: > || numPixels > (Long.MAX_VALUE / 4) || !validConfig(bitmapData.colorType)) { > I doubt that any detector (this or TextDetectionImpl) > would work with anything but 32bpp variations (the > 8888 ones). Also, Blink sends one of two formats: > BGRA_8888/RGBA_8888 but I don't see those two > being handled or a note as to why that isn't the case. I updated l.57 to handle BGRA_8888 the same as RGBA_8888, because it will be converted kN32_SkColorType with [1] when creating bitmap. [1] http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/jni/android/graphic... > https://codereview.chromium.org/2629433003/diff/200001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/TextDetectionImpl.java > (right): > > https://codereview.chromium.org/2629433003/diff/200001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/TextDetectionImpl.java:84: > || numPixels > (Long.MAX_VALUE / 4) || !validConfig(bitmapData.colorType)) { > Same comments here as in BarcodeDetectionImpl. > > https://codereview.chromium.org/2629433003/diff/200001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java > (right): > > https://codereview.chromium.org/2629433003/diff/200001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:32: > private Config mBitmapConfig; > We don't need a member variable, validConfig() > could return Config.UNKNOWN to the caller. Thanks, Done. > https://codereview.chromium.org/2629433003/diff/200001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:64: > || numPixels > (Long.MAX_VALUE / 4) || !validConfig(bitmapData.colorType)) { > Same comments as in Barcode/TextDetectionImpl > regarding the handled pixel formats. > > https://codereview.chromium.org/2629433003/diff/200001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:78: > if (bitmapData.colorType == ColorType.RGB_565) { > If blink is never sending this format, how could we test > this branch? Yes, how about marking TODO in this place? > https://codereview.chromium.org/2629433003/diff/200001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:82: > // we get from |Bitmap| is directly allocated and does not have a supporting > array. > || in comments applies to variables, not types, so here > I think you mean |bitmapData|. Thanks, Done. > https://codereview.chromium.org/2629433003/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right): > > https://codereview.chromium.org/2629433003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:26: int > height, > get is usually reserved for getters/accessors and in this > case we are creating, and create needs a 'from', so: > s/getBitmapOnData/createBitmapFromData/ ? Thanks, Done.
Patchset #4 (id:220001) has been deleted
The CQ bit was checked by junwei.fu@intel.com 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2629433003/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2629433003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:80: Config bitmapConfig = validConfig(bitmapData.colorType); The problem is that, even though the code reads as if a variety of bitmapData.colorType are supported, I believe only ARGB/BGRA_8888 would work, and anyway I can't imagine how to test this code, so what I would recommend is to limit the inputs to what we know works and can be tested today, i.e. remove validConfig() and instead of this line just do if (bitmapData.colorType != ColorType.RGBA_8888 && bitmapData.colorType != ColorType.BGRA_8888) { Log.e(TAG, "Unsupported bitmap pixel format"); callback.call(new BarcodeDetectionResult[0]); return; } remove the added check in l.84 and s/bitmapConfig/bitmapData.colorType/ in l.98. For bonus points, I'd make a new crbug.com in the lines of "add support for other bitmap pixel formats" and add a // TODO(junwei.fu): Consider supporting ... https://crbug.com/xyzxyz before the proposed if(bitmapData.colorType...) clause. All this also applies to the other DetectionImpl.java. https://codereview.chromium.org/2629433003/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java (right): https://codereview.chromium.org/2629433003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:102: } Similar comments to the others in this section: we don't use RGB_565 right now and the code in the first branch should be tested. Also, there is the issue of the directness [1] of |bitmapData|, which you are not explicitly testing for nor asserting, so it might well be that the RGB_565 branch would not work. Since the purpose of this CL is to migrate to mojom.Bitmap, I wouldn't add untested code, and just make sure that what we receive is RGBA/BGRA_8888, again with the same structure as before: if (bitmapData.colorType != ColorType.RGBA_8888 && bitmapData.colorType != ColorType.BGRA_8888) { Log.e(TAG, "Unsupported bitmap pixel format"); callback.call(new BarcodeDetectionResult[0]); return; } And add a TODO+bug to "use |bitmapData| directly for |unPremultipliedBitmap| to spare a copy" or some such description, which will be coded and tested subsequently but separatedly. [1] https://developer.android.com/reference/java/nio/ByteBuffer.html#isDirect()
The CQ bit was checked by junwei.fu@intel.com 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...
On 2017/01/24 20:48:32, mcasas wrote: > https://codereview.chromium.org/2629433003/diff/240001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java > (right): > > https://codereview.chromium.org/2629433003/diff/240001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:80: > Config bitmapConfig = validConfig(bitmapData.colorType); > The problem is that, even though the code reads as > if a variety of bitmapData.colorType are supported, > I believe only ARGB/BGRA_8888 would work, and anyway > I can't imagine how to test this code, so what I would > recommend is to limit the inputs to what we know works > and can be tested today, i.e. remove validConfig() > and instead of this line just do > > if (bitmapData.colorType != ColorType.RGBA_8888 > && bitmapData.colorType != ColorType.BGRA_8888) { > Log.e(TAG, "Unsupported bitmap pixel format"); > callback.call(new BarcodeDetectionResult[0]); > return; > } > > remove the added check in l.84 and > s/bitmapConfig/bitmapData.colorType/ in l.98. > > For bonus points, I'd make a new http://crbug.com in the lines > of "add support for other bitmap pixel formats" and > add a > // TODO(junwei.fu): Consider supporting ... https://crbug.com/xyzxyz > > before the proposed if(bitmapData.colorType...) clause. > > All this also applies to the other DetectionImpl.java. Thanks, Done. > https://codereview.chromium.org/2629433003/diff/240001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java > (right): > > https://codereview.chromium.org/2629433003/diff/240001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:102: > } > Similar comments to the others in this section: we don't > use RGB_565 right now and the code in the first branch > should be tested. Also, there is the issue of the > directness [1] of |bitmapData|, which you are not > explicitly testing for nor asserting, so it might well be > that the RGB_565 branch would not work. > > Since the purpose of this CL is to migrate to mojom.Bitmap, > I wouldn't add untested code, and just make sure that what > we receive is RGBA/BGRA_8888, again with the same > structure as before: > > if (bitmapData.colorType != ColorType.RGBA_8888 > && bitmapData.colorType != ColorType.BGRA_8888) { > Log.e(TAG, "Unsupported bitmap pixel format"); > callback.call(new BarcodeDetectionResult[0]); > return; > } > > > And add a TODO+bug to "use |bitmapData| directly for > |unPremultipliedBitmap| to spare a copy" or some such > description, which will be coded and tested subsequently > but separatedly. > > [1] https://developer.android.com/reference/java/nio/ByteBuffer.html#isDirect() Thanks, Done. I will take annual leave for 10 days, could you please help me to merge it if there are no other comments.
https://codereview.chromium.org/2629433003/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2629433003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:67: callback.call(new BarcodeDetectionResult[0]); Still need to return here https://codereview.chromium.org/2629433003/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/TextDetectionImpl.java (right): https://codereview.chromium.org/2629433003/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/TextDetectionImpl.java:65: callback.call(new TextDetectionResult[0]); Same here: return (otherwise execution would continue). https://codereview.chromium.org/2629433003/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java (right): https://codereview.chromium.org/2629433003/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:76: unPremultipliedBitmap.copyPixelsFromBuffer(imageBuffer); My comments are still unaddressed: We cannot have an RGB_565 section if we haven't tested it and known to work, and currently we cannot. So: keep l.50-55, remove the first branch of the if() and leave it as it was in ToT, with your TODO in the appropriate location.
The CQ bit was checked by junwei.fu@intel.com to run a CQ dry run
Patchset #5 (id:260001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/25 02:25:36, mcasas wrote: > https://codereview.chromium.org/2629433003/diff/260001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java > (right): > > https://codereview.chromium.org/2629433003/diff/260001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:67: > callback.call(new BarcodeDetectionResult[0]); > Still need to return here Thanks, Done. > https://codereview.chromium.org/2629433003/diff/260001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/TextDetectionImpl.java > (right): > > https://codereview.chromium.org/2629433003/diff/260001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/TextDetectionImpl.java:65: > callback.call(new TextDetectionResult[0]); > Same here: return (otherwise execution would continue). Thanks, Done. > https://codereview.chromium.org/2629433003/diff/260001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java > (right): > > https://codereview.chromium.org/2629433003/diff/260001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java:76: > unPremultipliedBitmap.copyPixelsFromBuffer(imageBuffer); > My comments are still unaddressed: We cannot have an > RGB_565 section if we haven't tested it and known to > work, and currently we cannot. So: keep l.50-55, remove the > first branch of the if() and leave it as it was in ToT, with > your TODO in the appropriate location. Thanks, Done.
lgtm
WebKit 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 junwei.fu@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, xianglu@chromium.org Link to the patchset: https://codereview.chromium.org/2629433003/#ps280001 (title: "ShapeDetection: use mojom::Bitmap for mojo interface.")
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...)
https://codereview.chromium.org/2629433003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2629433003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:86: || numPixels > (Long.MAX_VALUE / 4) || !validConfig(bitmapData.colorType)) { On 2017/01/24 00:52:05, mcasas wrote: > I doubt that any detector (this or TextDetectionImpl) > would work with anything but 32bpp variations (the > 8888 ones). Also, Blink sends one of two formats: > BGRA_8888/RGBA_8888 but I don't see those two > being handled or a note as to why that isn't the case. I updated l.57 to handle BGRA_8888 the same as RGBA_8888, because it will be converted kN32_SkColorType with [1] when creating bitmap. [1] http://androidxref.com/7.1.1_r6/xref/frameworks/base/core/jni/android/graphic...
PTAL for missing LGTM from an OWNER for these files: chrome/android/BUILD.gn chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java chrome/android/java/src/org/chromium/chrome/browser/shapedetection/TextDetectionImpl.java content/public/android/BUILD.gn content/public/android/java/src/org/chromium/content/browser/shapedetection/FaceDetectionImpl.java services/shape_detection/public/interfaces/BUILD.gn
chrome/android lgtm. Deferring to mcasas@ for some of the details on bitmap behavior/expectations here. Would it make sense to eventually pull some of this common code out for building a android.graphics.Bitmap from a org.chromium.skia.mojom.Bitmap?
junwei.fu@intel.com changed reviewers: + ben@chromium.org, timvolodine@chromium.org
The CQ bit was checked by junwei.fu@intel.com
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...)
On 2017/01/25 06:42:07, commit-bot: I haz the power wrote: > 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...) PTAL for missing LGTM from an OWNER for these files: services/shape_detection/public/interfaces/BUILD.gn
bauerb@chromium.org changed reviewers: - bauerb@chromium.org
rockot@chromium.org changed reviewers: + haraken@chromium.org
lgtm https://codereview.chromium.org/2629433003/diff/280001/services/shape_detecti... File services/shape_detection/public/interfaces/facedetection.mojom (right): https://codereview.chromium.org/2629433003/diff/280001/services/shape_detecti... services/shape_detection/public/interfaces/facedetection.mojom:26: Detect(skia.mojom.Bitmap bitmap_data) nit: formatting - should fit in one line now https://codereview.chromium.org/2629433003/diff/280001/services/shape_detecti... File services/shape_detection/public/interfaces/textdetection.mojom (right): https://codereview.chromium.org/2629433003/diff/280001/services/shape_detecti... services/shape_detection/public/interfaces/textdetection.mojom:17: Detect(skia.mojom.Bitmap bitmap_data) nit: formatting - should fit in one line now
The CQ bit was checked by xianglu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1485382577223640,
"parent_rev": "965cfcc7364062072a9dd574e39073f0f0d67abe", "commit_rev":
"33adf4c85dd553eb607bcf9c02c5fa60c8738a68"}
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/33adf4c85dd553eb607bcf9c02c5...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:280001) as https://chromium.googlesource.com/chromium/src/+/33adf4c85dd553eb607bcf9c02c5...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:280001) has been created in https://codereview.chromium.org/2655533005/ by maniscalco@chromium.org. The reason for reverting is: Suspected of breaking the build: https://build.chromium.org/p/chromium.gpu/builders/GPU%20Mac%20Builder/builds....
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 446142 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
aelias@chromium.org changed reviewers: - aelias@chromium.org, ben@chromium.org, mlamouri@chromium.org
Message was sent while issue was closed.
Description was changed from
==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/33adf4c85dd553eb607bcf9c02c5...
==========
to
==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/33adf4c85dd553eb607bcf9c02c5...
==========
The CQ bit was checked by junwei.fu@intel.com 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #6 (id:300001) has been deleted
The CQ bit was checked by junwei.fu@intel.com 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 junwei.fu@intel.com
The CQ bit was checked by junwei.fu@intel.com to run a CQ dry run
Patchset #6 (id:320001) has been deleted
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 junwei.fu@intel.com
The CQ bit was unchecked by junwei.fu@intel.com
Patchset #6 (id:340001) has been deleted
The CQ bit was checked by junwei.fu@intel.com to run a CQ dry run
The CQ bit was unchecked by junwei.fu@intel.com
The CQ bit was checked by junwei.fu@intel.com 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 junwei.fu@intel.com
Patchset #6 (id:360001) has been deleted
The CQ bit was checked by junwei.fu@intel.com to run a CQ dry run
The CQ bit was unchecked by junwei.fu@intel.com
The CQ bit was checked by junwei.fu@intel.com 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 junwei.fu@intel.com
Patchset #6 (id:380001) has been deleted
The CQ bit was checked by junwei.fu@intel.com to run a CQ dry run
The CQ bit was unchecked by junwei.fu@intel.com
The CQ bit was checked by junwei.fu@intel.com 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 junwei.fu@intel.com
Patchset #6 (id:400001) has been deleted
The CQ bit was checked by junwei.fu@intel.com 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 junwei.fu@intel.com
The CQ bit was checked by junwei.fu@intel.com 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 junwei.fu@intel.com
Patchset #6 (id:420001) has been deleted
@xianglu @mcasas PTAL for these files:
services/shape_detection/DEPS
services/shape_detection/barcode_detection_impl_mac.h
services/shape_detection/barcode_detection_impl_mac.mm
services/shape_detection/detection_utils_mac.h
services/shape_detection/detection_utils_mac.mm
services/shape_detection/face_detection_impl_mac.h
services/shape_detection/face_detection_impl_mac.mm
This CL has been reverted, so what you need to do is to upload a new CL, with the title RELAND:..., upload the landed CL verbatim as PatchSet 0, then the extra modifications as subsequent patches, e.g. like in [1]. Make sure the description quotes the original landed-reverted description, and what have you done, if applicable, to prevent another revert. (Also address my comments) [1] https://codereview.chromium.org/2623353004/ https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... File services/shape_detection/barcode_detection_impl_mac.mm (right): https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... services/shape_detection/barcode_detection_impl_mac.mm:53: void BarcodeDetectionImplMac::Detect(const SkBitmap& bitmap_data, s/bitmap_data/bitmap/ https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... services/shape_detection/barcode_detection_impl_mac.mm:66: int height = bitmap_data.height(); Const. https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... File services/shape_detection/detection_utils_mac.h (right): https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... services/shape_detection/detection_utils_mac.h:18: base::scoped_nsobject<CIImage> CreateCIImageFromData( s/CreateCIImageFromData/CreateCIImageFromSkBitmap/ https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... File services/shape_detection/face_detection_impl_mac.mm (right): https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... services/shape_detection/face_detection_impl_mac.mm:62: int height = bitmap_data.height(); Const.
On 2017/02/07 19:04:44, mcasas wrote: > This CL has been reverted, so what you need to do is to > upload a new CL, with the title RELAND:..., upload the landed > CL verbatim as PatchSet 0, then the extra modifications as > subsequent patches, e.g. like in [1]. Make sure the description > quotes the original landed-reverted description, and what > have you done, if applicable, to prevent another revert. > (Also address my comments) Thanks, the new CL is https://codereview.chromium.org/2681913003 > [1] https://codereview.chromium.org/2623353004/ > > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > File services/shape_detection/barcode_detection_impl_mac.mm (right): > > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > services/shape_detection/barcode_detection_impl_mac.mm:53: void > BarcodeDetectionImplMac::Detect(const SkBitmap& bitmap_data, > s/bitmap_data/bitmap/ Thanks, done. > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > services/shape_detection/barcode_detection_impl_mac.mm:66: int height = > bitmap_data.height(); > Const. Thanks, done. > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > File services/shape_detection/detection_utils_mac.h (right): > > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > services/shape_detection/detection_utils_mac.h:18: > base::scoped_nsobject<CIImage> CreateCIImageFromData( > s/CreateCIImageFromData/CreateCIImageFromSkBitmap/ Thanks, done. > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > File services/shape_detection/face_detection_impl_mac.mm (right): > > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > services/shape_detection/face_detection_impl_mac.mm:62: int height = > bitmap_data.height(); > Const. Thanks, done.
On 2017/02/07 19:04:44, mcasas wrote: > This CL has been reverted, so what you need to do is to > upload a new CL, with the title RELAND:..., upload the landed > CL verbatim as PatchSet 0, then the extra modifications as > subsequent patches, e.g. like in [1]. Make sure the description > quotes the original landed-reverted description, and what > have you done, if applicable, to prevent another revert. > (Also address my comments) Thanks, the new CL is https://codereview.chromium.org/2681913003 > [1] https://codereview.chromium.org/2623353004/ > > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > File services/shape_detection/barcode_detection_impl_mac.mm (right): > > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > services/shape_detection/barcode_detection_impl_mac.mm:53: void > BarcodeDetectionImplMac::Detect(const SkBitmap& bitmap_data, > s/bitmap_data/bitmap/ Thanks, done. > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > services/shape_detection/barcode_detection_impl_mac.mm:66: int height = > bitmap_data.height(); > Const. Thanks, done. > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > File services/shape_detection/detection_utils_mac.h (right): > > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > services/shape_detection/detection_utils_mac.h:18: > base::scoped_nsobject<CIImage> CreateCIImageFromData( > s/CreateCIImageFromData/CreateCIImageFromSkBitmap/ Thanks, done. > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > File services/shape_detection/face_detection_impl_mac.mm (right): > > https://codereview.chromium.org/2629433003/diff/440001/services/shape_detecti... > services/shape_detection/face_detection_impl_mac.mm:62: int height = > bitmap_data.height(); > Const. Thanks, done. |
