ShapeDetection: add support for all CanvasImageSource input types

ShapeDetection: add support for all CanvasImageSource input types

Currently ShapeDetector::detectFacesOn{ImageBitmap,Videoelement}()
extract the pixels out of the corresponding object; I haven't noticed
that CanvasImageSource::getSourceImageForCanvas() is a virtual method,
implemented for each CanvasImageSource, doing exactly what I needed,
which is extracting an WTF::Image.

getSourceImageForCanvas() implementations have the same checks
being performed on ToT now, so swapping one for the other is
not reducing the early-bail outs.

So, this CL factors out the code to use getSourceImageForCanvas(),
making the Shape Detection detect() work for all these different
input types, and adds LayoutTests for them.

BUG=670977, 659138
Committed: https://crrev.com/7d0018f762087ab6624de7dd09cf15207dda57c6
Cr-Commit-Position: refs/heads/master@{#437144}

[mcasas, 2016-12-07 01:15:23]

Description was changed from

==========
ShapeDetection: add support for all CanvasImageSource input types

Currently ShapeDetector::detectFacesOn{ImageBitmap,Videoelement}()
extract the pixels out of the corresponding object; I haven't noticed
that CanvasImageSource::getSourceImageForCanvas() is a virtual method,
implemented for each CanvasImageSource, doing exactly what I needed,
which is extracting an WTF::Image.

So, this CL factors out the code to use getSourceImageForCanvas(),
making the Shape Detection detect() work for all these different
input types, and adds LayoutTests for them.

BUG=670975, 670977, 659138
==========

to

==========
ShapeDetection: add support for all CanvasImageSource input types

Currently ShapeDetector::detectFacesOn{ImageBitmap,Videoelement}()
extract the pixels out of the corresponding object; I haven't noticed
that CanvasImageSource::getSourceImageForCanvas() is a virtual method,
implemented for each CanvasImageSource, doing exactly what I needed,
which is extracting an WTF::Image.

So, this CL factors out the code to use getSourceImageForCanvas(),
making the Shape Detection detect() work for all these different
input types, and adds LayoutTests for them.

BUG=670977, 659138
==========

[mcasas, 2016-12-07 01:17:27]

Description was changed from

==========
ShapeDetection: add support for all CanvasImageSource input types

Currently ShapeDetector::detectFacesOn{ImageBitmap,Videoelement}()
extract the pixels out of the corresponding object; I haven't noticed
that CanvasImageSource::getSourceImageForCanvas() is a virtual method,
implemented for each CanvasImageSource, doing exactly what I needed,
which is extracting an WTF::Image.

So, this CL factors out the code to use getSourceImageForCanvas(),
making the Shape Detection detect() work for all these different
input types, and adds LayoutTests for them.

BUG=670977, 659138
==========

to

==========
ShapeDetection: add support for all CanvasImageSource input types

**wip** do not review :-)

Currently ShapeDetector::detectFacesOn{ImageBitmap,Videoelement}()
extract the pixels out of the corresponding object; I haven't noticed
that CanvasImageSource::getSourceImageForCanvas() is a virtual method,
implemented for each CanvasImageSource, doing exactly what I needed,
which is extracting an WTF::Image.

So, this CL factors out the code to use getSourceImageForCanvas(),
making the Shape Detection detect() work for all these different
input types, and adds LayoutTests for them.

BUG=670977, 659138
==========

[mcasas, 2016-12-07 17:47:58]

Description was changed from

==========
ShapeDetection: add support for all CanvasImageSource input types

**wip** do not review :-)

Currently ShapeDetector::detectFacesOn{ImageBitmap,Videoelement}()
extract the pixels out of the corresponding object; I haven't noticed
that CanvasImageSource::getSourceImageForCanvas() is a virtual method,
implemented for each CanvasImageSource, doing exactly what I needed,
which is extracting an WTF::Image.

So, this CL factors out the code to use getSourceImageForCanvas(),
making the Shape Detection detect() work for all these different
input types, and adds LayoutTests for them.

BUG=670977, 659138
==========

to

==========
ShapeDetection: add support for all CanvasImageSource input types

Currently ShapeDetector::detectFacesOn{ImageBitmap,Videoelement}()
extract the pixels out of the corresponding object; I haven't noticed
that CanvasImageSource::getSourceImageForCanvas() is a virtual method,
implemented for each CanvasImageSource, doing exactly what I needed,
which is extracting an WTF::Image.

getSourceImageForCanvas() implementations have the same checks
being performed on ToT now, so swapping one for the other is
not reducing the early-bail outs.

So, this CL factors out the code to use getSourceImageForCanvas(),
making the Shape Detection detect() work for all these different
input types, and adds LayoutTests for them.

BUG=670977, 659138
==========

[mcasas, 2016-12-07 17:49:52]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-12-07 17:51:57]

mcasas@chromium.org changed reviewers:
+ reillyg@chromium.org, xianglu@chromium.org

[mcasas, 2016-12-07 17:51:57]

xianglu@, reillyg@ PTAL

[xianglu, 2016-12-07 18:45:10]

LGTM % comment

https://codereview.chromium.org/2553343003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right):

https://codereview.chromium.org/2553343003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:118: //
TODO(mcasas): retrieve the pixels from elsewhere.
In previous version we can get address via copyBitmapData(). Maybe add a comment
here for future reference?

[mcasas, 2016-12-07 20:02:09]

mcasas@chromium.org changed reviewers:
+ haraken@chromium.org

[mcasas, 2016-12-07 20:02:10]

ping reillyg@

haraken@ rs plz modules/shapedetection

https://codereview.chromium.org/2553343003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right):

https://codereview.chromium.org/2553343003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:118: //
TODO(mcasas): retrieve the pixels from elsewhere.
On 2016/12/07 18:45:10, xianglu wrote:
> In previous version we can get address via copyBitmapData(). Maybe add a
comment
> here for future reference?

Oh, I forgot that one. Brought here one now -- 
although it seems like our tests did not over this, 
or that code was never used, hmmm.

[Reilly Grant (use Gerrit), 2016-12-07 21:37:06]

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/shapedetection/detection-HTMLCanvasElement.html
(right):

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/detection-HTMLCanvasElement.html:18:
var canvas = createCanvas(); ////document.createElement('canvas');
Leftover comment?

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/detection-HTMLCanvasElement.html:25:
var detector = eval("new " + detectorName + "();");
Why not pass the constructor function instead of a string?

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/detection-HTMLCanvasElement.html:43:
img.src = '../media/content/greenbox.png';
Be consistent about ' vs. ".

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right):

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:115:
pixelDataPtr = static_cast<uint8_t*>(pixmap.writable_addr());
Use pixmap.writable_addr8() and avoid the static_cast.

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:121: :
DontPremultiplyAlpha,
Please double check that this isn't backwards. If the image is already
premultiplied then shouldn't we not premultiply again on copy?

[mcasas, 2016-12-07 22:02:37]

ptal

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/shapedetection/detection-HTMLCanvasElement.html
(right):

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/detection-HTMLCanvasElement.html:18:
var canvas = createCanvas(); ////document.createElement('canvas');
On 2016/12/07 21:37:06, Reilly Grant wrote:
> Leftover comment?

Oops, yes, removed.

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/detection-HTMLCanvasElement.html:25:
var detector = eval("new " + detectorName + "();");
On 2016/12/07 21:37:06, Reilly Grant wrote:
> Why not pass the constructor function instead of a string?

Done. Note that you can't pass |bla| and
evaluate bla() as a constructor, operator
new would not work properly. But I removed
the odious eval() and instead I have a function
in the arguments list.

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/detection-HTMLCanvasElement.html:43:
img.src = '../media/content/greenbox.png';
On 2016/12/07 21:37:06, Reilly Grant wrote:
> Be consistent about ' vs. ".

Done.

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right):

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:115:
pixelDataPtr = static_cast<uint8_t*>(pixmap.writable_addr());
On 2016/12/07 21:37:06, Reilly Grant wrote:
> Use pixmap.writable_addr8() and avoid the static_cast.

Looks like that would be the one, but, misleadingly enough,
addr8() family of functions only work for 8 bit pixel formats,
see the check in [1].

[1]
https://cs.chromium.org/chromium/src/third_party/skia/include/core/SkPixmap.h...

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:121: :
DontPremultiplyAlpha,
On 2016/12/07 21:37:06, Reilly Grant wrote:
> Please double check that this isn't backwards. If the image is already
> premultiplied then shouldn't we not premultiply again on copy?

This is copied from [1]. |PremultiplyAlpha| boils down to
kPremul_SkAlphaType in [2]. What's your concern 
about using the wrong premultiplied type?

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Ima...

[Reilly Grant (use Gerrit), 2016-12-07 22:09:35]

lgtm

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp (right):

https://codereview.chromium.org/2553343003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp:121: :
DontPremultiplyAlpha,
On 2016/12/07 at 22:02:37, mcasas wrote:
> On 2016/12/07 21:37:06, Reilly Grant wrote:
> > Please double check that this isn't backwards. If the image is already
> > premultiplied then shouldn't we not premultiply again on copy?
> 
> This is copied from [1]. |PremultiplyAlpha| boils down to
> kPremul_SkAlphaType in [2]. What's your concern 
> about using the wrong premultiplied type?
> 
> [1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
> [2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Ima...

Then it is probably right. My concern was that passing PremultiplyAlpha when the
bitmap data is premultiplied would apply the alpha twice. I'm not familiar with
the semantics of these arguments though so I will defer to you and the example
you cite.

[mcasas, 2016-12-07 22:53:36]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 22:53:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 23:59:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 23:59:26]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-12-08 01:17:17]

LGTM

[mcasas, 2016-12-08 01:56:24]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-12-08 01:56:24]

The patchset sent to the CQ was uploaded after l-g-t-m from xianglu@chromium.org
Link to the patchset: https://codereview.chromium.org/2553343003/#ps60001
(title: "reillyg@s comments")

[commit-bot: I haz the power, 2016-12-08 01:57:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 02:12:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481162184228510,
"parent_rev": "52fef77801766f8a4ad697c18c1dfb6f8ed40af7", "commit_rev":
"840d81074502d90aa4a43f4022544b8dc38a76bc"}

[commit-bot: I haz the power, 2016-12-08 02:13:32]

Description was changed from

==========
ShapeDetection: add support for all CanvasImageSource input types

Currently ShapeDetector::detectFacesOn{ImageBitmap,Videoelement}()
extract the pixels out of the corresponding object; I haven't noticed
that CanvasImageSource::getSourceImageForCanvas() is a virtual method,
implemented for each CanvasImageSource, doing exactly what I needed,
which is extracting an WTF::Image.

getSourceImageForCanvas() implementations have the same checks
being performed on ToT now, so swapping one for the other is
not reducing the early-bail outs.

So, this CL factors out the code to use getSourceImageForCanvas(),
making the Shape Detection detect() work for all these different
input types, and adds LayoutTests for them.

BUG=670977, 659138
==========

to

==========
ShapeDetection: add support for all CanvasImageSource input types

Currently ShapeDetector::detectFacesOn{ImageBitmap,Videoelement}()
extract the pixels out of the corresponding object; I haven't noticed
that CanvasImageSource::getSourceImageForCanvas() is a virtual method,
implemented for each CanvasImageSource, doing exactly what I needed,
which is extracting an WTF::Image.

getSourceImageForCanvas() implementations have the same checks
being performed on ToT now, so swapping one for the other is
not reducing the early-bail outs.

So, this CL factors out the code to use getSourceImageForCanvas(),
making the Shape Detection detect() work for all these different
input types, and adds LayoutTests for them.

BUG=670977, 659138
==========

[commit-bot: I haz the power, 2016-12-08 02:13:33]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-12-08 02:17:20]

Description was changed from

==========
ShapeDetection: add support for all CanvasImageSource input types

Currently ShapeDetector::detectFacesOn{ImageBitmap,Videoelement}()
extract the pixels out of the corresponding object; I haven't noticed
that CanvasImageSource::getSourceImageForCanvas() is a virtual method,
implemented for each CanvasImageSource, doing exactly what I needed,
which is extracting an WTF::Image.

getSourceImageForCanvas() implementations have the same checks
being performed on ToT now, so swapping one for the other is
not reducing the early-bail outs.

So, this CL factors out the code to use getSourceImageForCanvas(),
making the Shape Detection detect() work for all these different
input types, and adds LayoutTests for them.

BUG=670977, 659138
==========

to

==========
ShapeDetection: add support for all CanvasImageSource input types

Currently ShapeDetector::detectFacesOn{ImageBitmap,Videoelement}()
extract the pixels out of the corresponding object; I haven't noticed
that CanvasImageSource::getSourceImageForCanvas() is a virtual method,
implemented for each CanvasImageSource, doing exactly what I needed,
which is extracting an WTF::Image.

getSourceImageForCanvas() implementations have the same checks
being performed on ToT now, so swapping one for the other is
not reducing the early-bail outs.

So, this CL factors out the code to use getSourceImageForCanvas(),
making the Shape Detection detect() work for all these different
input types, and adds LayoutTests for them.

BUG=670977, 659138

Committed: https://crrev.com/7d0018f762087ab6624de7dd09cf15207dda57c6
Cr-Commit-Position: refs/heads/master@{#437144}
==========

[commit-bot: I haz the power, 2016-12-08 02:17:21]

Patchset 3 (id:??) landed as
https://crrev.com/7d0018f762087ab6624de7dd09cf15207dda57c6
Cr-Commit-Position: refs/heads/master@{#437144}