Implement ImageData::CropRect()

Implement ImageData::CropRect()

This change implements ImageData::CropRect() that crops the image data object to the given rectangle and flips if needed.

BUG=716136
Review-Url: https://codereview.chromium.org/2849643002
Cr-Commit-Position: refs/heads/master@{#476329}
Committed: https://chromium.googlesource.com/chromium/src/+/455eee4814157429627353c6fdce0f2c6412bb0d

[zakerinasab, 2017-04-27 20:07:00]

Description was changed from

==========
Implement ImageData::MakeSubset()

This change implements ImageData::MakeSubset() that crops the image data
object to the given rectangle.

BUG=716136
==========

to

==========
Implement ImageData::MakeSubset()

This change implements ImageData::MakeSubset() that crops the image data
object to the given rectangle.

BUG=716136
==========

[zakerinasab, 2017-04-27 20:07:00]

zakerinasab@chromium.org changed reviewers:
+ junov@chromium.org

[zakerinasab, 2017-04-27 20:10:15]

zakerinasab@chromium.org changed reviewers:
+ fserb@chromium.org

[zakerinasab, 2017-04-27 20:10:15]

This CL is originally extracted from https://codereview.chromium.org/2845193002
for easier review and landing.

[zakerinasab, 2017-04-28 04:00:55]

Description was changed from

==========
Implement ImageData::MakeSubset()

This change implements ImageData::MakeSubset() that crops the image data
object to the given rectangle.

BUG=716136
==========

to

==========
Implement ImageData MakeCopy() and MakeSubset()

This change implements ImageData::MakeCopy() that creates a copy of ImageData
and ImageData::MakeSubset() that crops the image data object to the given
rectangle.

BUG=716136
==========

[zakerinasab, 2017-04-28 13:56:21]

New CL uploaded. ImageData::MakeCopy() added with the respective unit test.

[fserb, 2017-04-28 14:56:54]

https://codereview.chromium.org/2849643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.h (right):

https://codereview.chromium.org/2849643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.h:114: ImageData* MakeCopy();
I don't know what use case you have in mind for those two functions, but I'd
reconsider this API.

Drop MakeCopy. Rename MakeSubset to CopyRect (or MakeCopy if you prefer). You
can check if it's a full rect there and do the single memcpy. And drop the
enforce_copy parameter.

[blink-reviews, 2017-04-28 14:57:37]

Good suggestions.

On Fri, Apr 28, 2017 at 10:56 AM, <fserb@chromium.org> wrote:

>
> https://codereview.chromium.org/2849643002/diff/20001/
> third_party/WebKit/Source/core/html/ImageData.h
> File third_party/WebKit/Source/core/html/ImageData.h (right):
>
> https://codereview.chromium.org/2849643002/diff/20001/
> third_party/WebKit/Source/core/html/ImageData.h#newcode114
> third_party/WebKit/Source/core/html/ImageData.h:114: ImageData*
> MakeCopy();
> I don't know what use case you have in mind for those two functions, but
> I'd reconsider this API.
>
> Drop MakeCopy. Rename MakeSubset to CopyRect (or MakeCopy if you
> prefer). You can check if it's a full rect there and do the single
> memcpy. And drop the enforce_copy parameter.
>
> https://codereview.chromium.org/2849643002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-04-28 14:57:38]

Good suggestions.

On Fri, Apr 28, 2017 at 10:56 AM, <fserb@chromium.org> wrote:

>
> https://codereview.chromium.org/2849643002/diff/20001/
> third_party/WebKit/Source/core/html/ImageData.h
> File third_party/WebKit/Source/core/html/ImageData.h (right):
>
> https://codereview.chromium.org/2849643002/diff/20001/
> third_party/WebKit/Source/core/html/ImageData.h#newcode114
> third_party/WebKit/Source/core/html/ImageData.h:114: ImageData*
> MakeCopy();
> I don't know what use case you have in mind for those two functions, but
> I'd reconsider this API.
>
> Drop MakeCopy. Rename MakeSubset to CopyRect (or MakeCopy if you
> prefer). You can check if it's a full rect there and do the single
> memcpy. And drop the enforce_copy parameter.
>
> https://codereview.chromium.org/2849643002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[zakerinasab, 2017-04-28 15:40:49]

New CL uploaded addressing fserb@ comments.

https://codereview.chromium.org/2849643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.h (right):

https://codereview.chromium.org/2849643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.h:114: ImageData* MakeCopy();
On 2017/04/28 14:56:53, fserb wrote:
> I don't know what use case you have in mind for those two functions, but I'd
> reconsider this API.
> 
> Drop MakeCopy. Rename MakeSubset to CopyRect (or MakeCopy if you prefer). You
> can check if it's a full rect there and do the single memcpy. And drop the
> enforce_copy parameter.

Good suggestions. Applied.

[zakerinasab, 2017-04-28 16:52:58]

Description was changed from

==========
Implement ImageData MakeCopy() and MakeSubset()

This change implements ImageData::MakeCopy() that creates a copy of ImageData
and ImageData::MakeSubset() that crops the image data object to the given
rectangle.

BUG=716136
==========

to

==========
Implement ImageData::CopyRect()

This change implements ImageData::CopyRect() that crops the image data object to
the given rectangle.

BUG=716136
==========

[fserb, 2017-04-28 18:54:03]

lgtm

https://codereview.chromium.org/2849643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/2849643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.cpp:380: unsigned data_size = 4 *
dst_rect.Width() * dst_rect.Height();
please specify the full type. Here and below. :)

[zakerinasab, 2017-05-01 14:24:39]

https://codereview.chromium.org/2849643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/2849643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.cpp:380: unsigned data_size = 4 *
dst_rect.Width() * dst_rect.Height();
On 2017/04/28 18:54:03, fserb wrote:
> please specify the full type. Here and below. :)

By "full type" do you mean "unsigned int"? Using "unsigned int" generates the
following presubmit error:

check-webkit-style failed
  third_party/WebKit/Source/core/html/ImageData.cpp:51:  Omit int when using
unsigned  [runtime/unsigned] [1]

[Justin Novosad, 2017-05-02 19:00:54]

https://codereview.chromium.org/2849643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/2849643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.cpp:380: unsigned data_size = 4 *
dst_rect.Width() * dst_rect.Height();
Are we sure this arithmetic is safe with respect to overflows?

[zakerinasab, 2017-05-03 14:45:55]

https://codereview.chromium.org/2849643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/2849643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.cpp:380: unsigned data_size = 4 *
dst_rect.Width() * dst_rect.Height();
On 2017/05/02 19:00:54, Justin Novosad wrote:
> Are we sure this arithmetic is safe with respect to overflows?

It should be, as it is an intersection of the size of the current image data
(which should be already safe) and the crop_rect.

[Justin Novosad, 2017-05-30 19:45:22]

https://codereview.chromium.org/2849643002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/2849643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.cpp:394: dst_index = flip_y ?
(dst_rect.Height() - i - 1) * dst_rect.Width() * 4
You can avoid having this conditional branch and arithmetic inside the loop by
computing a "row stride" before the loop and here you would just have dst_index
+= dst_row_stride;  it also save a bunch of arithmetic.

[zakerinasab, 2017-05-30 20:06:16]

New CL uploaded. Comments addressed.

https://codereview.chromium.org/2849643002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/2849643002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.cpp:394: dst_index = flip_y ?
(dst_rect.Height() - i - 1) * dst_rect.Width() * 4
On 2017/05/30 19:45:22, Justin Novosad wrote:
> You can avoid having this conditional branch and arithmetic inside the loop by
> computing a "row stride" before the loop and here you would just have
dst_index
> += dst_row_stride;  it also save a bunch of arithmetic.

Done.

[zakerinasab, 2017-05-30 20:06:17]

New CL uploaded. Comments addressed.

[Justin Novosad, 2017-05-31 18:46:03]

lgtm

[zakerinasab, 2017-05-31 18:46:44]

The CQ bit was checked by zakerinasab@chromium.org

[zakerinasab, 2017-05-31 18:46:45]

The patchset sent to the CQ was uploaded after l-g-t-m from fserb@chromium.org
Link to the patchset: https://codereview.chromium.org/2849643002/#ps100001
(title: "Addressing comments")

[commit-bot: I haz the power, 2017-05-31 18:47:43]

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, 2017-05-31 19:38:03]

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

[commit-bot: I haz the power, 2017-05-31 19:38:04]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[zakerinasab, 2017-05-31 20:03:56]

Description was changed from

==========
Implement ImageData::CopyRect()

This change implements ImageData::CopyRect() that crops the image data object to
the given rectangle.

BUG=716136
==========

to

==========
Implement ImageData::CropRect()

This change implements ImageData::CropRect() that crops the image data object to
the given rectangle and flips if needed.

BUG=716136
==========

[zakerinasab, 2017-06-01 15:09:30]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2017-06-01 15:09:50]

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, 2017-06-01 16:58:17]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496329770324660,
"parent_rev": "1bc3d8c1ce10761406e57b07f7a2dc1ad301221e", "commit_rev":
"455eee4814157429627353c6fdce0f2c6412bb0d"}

[commit-bot: I haz the power, 2017-06-01 16:58:26]

Description was changed from

==========
Implement ImageData::CropRect()

This change implements ImageData::CropRect() that crops the image data object to
the given rectangle and flips if needed.

BUG=716136
==========

to

==========
Implement ImageData::CropRect()

This change implements ImageData::CropRect() that crops the image data object to
the given rectangle and flips if needed.

BUG=716136

Review-Url: https://codereview.chromium.org/2849643002
Cr-Commit-Position: refs/heads/master@{#476329}
Committed:
https://chromium.googlesource.com/chromium/src/+/455eee4814157429627353c6fdce...
==========

[commit-bot: I haz the power, 2017-06-01 16:58:28]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/455eee4814157429627353c6fdce...