Issue 1631733003: Implementing ImageBitmap option imageOrientation of flipY

Issue 1631733003: Implementing ImageBitmap option imageOrientation of flipY (Closed)

Created:
4 years, 11 months ago by xidachen

Modified:
4 years, 10 months ago

Reviewers:
jbroman, Justin Novosad

CC:
blink-reviews, chromium-reviews

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Implementing option imageOrientation for createImageBitmap(ImageData) This CL implements the ImageBitmap option imageOrientation for the method createImageBitmap(ImageData) only. For createImageBitmap from other sources, there will be separate CLs to handle them. The whatwg wiki for the ImageBitmap options is here: https://wiki.whatwg.org/wiki/ImageBitmap_Options In the case of ImageData, the option none/topLeft should behave the same, which is "Do not change orientation". This is because ImageData does not contain any image media meta data. Similarly, For the option of flipY/topLeft, the behavior should be flip the image vertically. This CL also includes a layout test, which re-uses the existing test with the additional parameter of "flipY", and currently the layout test is for the ImageData as source only. BUG=249382 Committed: https://crrev.com/542c67487c8bac201e42f607f6f3727382831c97 Cr-Commit-Position: refs/heads/master@{#373274}

Patch Set 1 #

Total comments: 10

Patch Set 2 : support flipY and none at this moment #

Total comments: 2

Patch Set 3 : replace "flipY" by a static const char* #

Created: 4 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+3941 lines, -47 lines)			Patch
A	third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY.html	View	1	1 chunk	+553 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY-expected.txt	View	1	1 chunk	+3073 lines, -0 lines	0 comments	Download
A +	third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-video-flipY.html	View	1	7 chunks	+84 lines, -39 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-video-flipY-expected.txt	View	1	1 chunk	+177 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/frame/ImageBitmap.cpp	View	1 2	5 chunks	+52 lines, -8 lines	0 comments	Download
M	third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 24 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

jbroman

Could you please link the draft spec either here or in the bug? It's not ...

4 years, 11 months ago (2016-01-26 16:16:49 UTC) #3

xidachen

Description was changed from ========== Implementing option imageOrientation for createImageBitmap(ImageData) This CL implements the ImageBitmap ...

4 years, 11 months ago (2016-01-26 16:22:12 UTC) #4

xidachen

On 2016/01/26 16:16:49, jbroman wrote: > Could you please link the draft spec either here ...

4 years, 11 months ago (2016-01-26 16:23:27 UTC) #5

jbroman

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY.html File third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY.html (right): https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY.html#newcode6 third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY.html:6: <body> Would you mind also testing the other cases ...

4 years, 11 months ago (2016-01-26 17:03:37 UTC) #6

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY.html
(right):

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY.html:6:
<body>
Would you mind also testing the other cases (none, topLeft, bottomLeft)? I'd
suggest refactoring the test to avoid duplication while you're at it.

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY.html:172:
var p1 = createImageBitmap(element, {'orientation' : "flipY"}).then(function
(image) { imageBitmaps.noCrop = image });
style nit: prefer not to quote the keys, and to have no space before the colon,
as in:

{orientation: "flipY"}

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:92: IntPoint dstPoint =
IntPoint(std::min(0, -cropRect.x()), std::min(0, -cropRect.y()));
Is it intended that the crop rect apply to the image after the orientation has
been applied? It takes awhile to figure out whether the rectangle is specified
in the flipped or unflipped coordinate space, and a comment here (and one in the
spec) would be very helpful.

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:123: PassOwnPtr<uint8_t[]>
ImageBitmap::copyFlippedSourceData(const unsigned char* source, int width, int
height)
This assumes 4-byte pixels, and that the row size is exactly width * 4. This
seems to be true for ImageData, but not generally. Please either make this take
the ImageData instead, or rename it so that it's clear that these expectations
apply, to avoid misuse.

Also, this does not need to be a member of ImageBitmap, so I'd rather this were
a free static function, like normalizeRect and cropImage at the top of this
file. (It will then not need to be declared in the header.)

Justin Novosad

BTW, in the proposal, the dictionary attribute is called "imageOrientation". Calling it just "orientation" is ...

4 years, 10 months ago (2016-02-01 22:52:16 UTC) #7

jbroman

On 2016/02/01 at 22:52:16, junov wrote: > BTW, in the proposal, the dictionary attribute is ...

4 years, 10 months ago (2016-02-02 16:06:40 UTC) #8

jbroman

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl File third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl (right): https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl#newcode8 third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl:8: ImageOrientation orientation = "topLeft"; As junov says, this is ...

4 years, 10 months ago (2016-02-02 16:06:53 UTC) #9

Justin Novosad

On 2016/02/02 16:06:53, jbroman wrote: > https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl > File third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl (right): > > https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl#newcode8 > ...

4 years, 10 months ago (2016-02-02 18:46:31 UTC) #10

xidachen

Please take a look at PS#2. In this patch, we changed the code so that ...

4 years, 10 months ago (2016-02-03 02:17:20 UTC) #11

Please take a look at PS#2.

In this patch, we changed the code so that it is crop the source first, and
apply flipY.

Also, the new patch makes the flipY option be able to be applied to all sources
for createImageBitmap.

At this moment, we support two options only which are none and flipY. The
options of topLeft and bottomLeft is more tricky because it requires identifying
the visual orientation of the source, it will be solved in future CL(s).

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY.html
(right):

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY.html:6:
<body>
On 2016/01/26 17:03:37, jbroman wrote:
> Would you mind also testing the other cases (none, topLeft, bottomLeft)? I'd
> suggest refactoring the test to avoid duplication while you're at it.

Acknowledged.

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-flipY.html:172:
var p1 = createImageBitmap(element, {'orientation' : "flipY"}).then(function
(image) { imageBitmaps.noCrop = image });
On 2016/01/26 17:03:37, jbroman wrote:
> style nit: prefer not to quote the keys, and to have no space before the
colon,
> as in:
> 
> {orientation: "flipY"}

Acknowledged.

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:92: IntPoint dstPoint =
IntPoint(std::min(0, -cropRect.x()), std::min(0, -cropRect.y()));
On 2016/01/26 17:03:37, jbroman wrote:
> Is it intended that the crop rect apply to the image after the orientation has
> been applied? It takes awhile to figure out whether the rectangle is specified
> in the flipped or unflipped coordinate space, and a comment here (and one in
the
> spec) would be very helpful.

Upon discussion with kbr@, it actually should be applying crop first and then
flip.

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:123: PassOwnPtr<uint8_t[]>
ImageBitmap::copyFlippedSourceData(const unsigned char* source, int width, int
height)
On 2016/01/26 17:03:37, jbroman wrote:
> This assumes 4-byte pixels, and that the row size is exactly width * 4. This
> seems to be true for ImageData, but not generally. Please either make this
take
> the ImageData instead, or rename it so that it's clear that these expectations
> apply, to avoid misuse.
> 
> Also, this does not need to be a member of ImageBitmap, so I'd rather this
were
> a free static function, like normalizeRect and cropImage at the top of this
> file. (It will then not need to be declared in the header.)

This function won't be needed anymore because we don't need to apply flip to the
source data.

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl (right):

https://codereview.chromium.org/1631733003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl:8:
ImageOrientation orientation = "topLeft";
On 2016/02/02 16:06:52, jbroman wrote:
> As junov says, this is "ImageOrientation imageOrientation" as it's written in
> the wiki page right now.

Acknowledged.

Justin Novosad

https://codereview.chromium.org/1631733003/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1631733003/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode74 third_party/WebKit/Source/core/frame/ImageBitmap.cpp:74: if (options.imageOrientation() == "flipY") Put this string value in ...

4 years, 10 months ago (2016-02-03 14:32:52 UTC) #12

xidachen

PTAL. https://codereview.chromium.org/1631733003/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/1631733003/diff/20001/third_party/WebKit/Source/core/frame/ImageBitmap.cpp#newcode74 third_party/WebKit/Source/core/frame/ImageBitmap.cpp:74: if (options.imageOrientation() == "flipY") On 2016/02/03 14:32:52, Justin ...

4 years, 10 months ago (2016-02-03 15:29:51 UTC) #13