fix size check for drawBitmap fast-path in SkBitmapDevice::drawBitmapRect

Fix the size check for the drawBitmap fast-path in SkBitmapDevice::drawBitmapRect. It would fail when the source rectangle had a non-zero offset, in which case it would compare the source rectangle with the offset to the extracted bitmap size, which always fails. The only thing that should matter is that the source rectangle and extract bitmap have the same size, since the offset gets added onto the matrix.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2089583002

Review-Url: https://codereview.chromium.org/2089583002
Committed: https://skia.googlesource.com/skia/+/ea9bc0c07b5dae78a9a449d7d7a07fc79262d41a
Review-Url: https://codereview.chromium.org/2089583002
Committed: https://skia.googlesource.com/skia/+/c0e52f44aac391da632d88d9702aea4f41cfe17a

[lsalzman1, 2016-06-21 01:48:42]

Description was changed from

==========
fix size check for drawBitmap fast-path in SkBitmapDevice::drawBitmapRect

BUG=skia:
==========

to

==========
fix size check for drawBitmap fast-path in SkBitmapDevice::drawBitmapRect

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2089583002
==========

[lsalzman1, 2016-06-21 01:54:06]

Description was changed from

==========
fix size check for drawBitmap fast-path in SkBitmapDevice::drawBitmapRect

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2089583002
==========

to

==========
Fix the size check for the drawBitmap fast-path in
SkBitmapDevice::drawBitmapRect. It would fail when the source rectangle had a
non-zero offset, in which case it would compare the source rectangle with the
offset to the extracted bitmap size, which always fails. The only thing that
should matter is that the source rectangle and extract bitmap have the same
size, since the offset gets added onto the matrix.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2089583002
==========

[lsalzman1, 2016-06-21 01:54:06]

lsalzman@mozilla.com changed reviewers:
+ reed@google.com

[reed1, 2016-06-21 15:35:26]

Is this motivate by correctness or performance or something else?

I'm seeing several GMs draw differently w/ this applied:
- bleed_image
- bleed_alpha_image_shader
- bleed_alpha_image_bmp
- bleed

[jmuizelaar, 2016-06-21 15:52:46]

On 2016/06/21 15:35:26, reed1 wrote:
> Is this motivate by correctness or performance or something else?

performance

[reed1, 2016-07-12 22:41:42]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 22:41:49]

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-07-12 22:50:44]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 22:50:45]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on master.client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2017-03-05 23:52:33]

Just reran this, and got some GM differences, especially in
bleed_alpha_bmp_shader. Are these expected?

[lsalzman1, 2017-03-07 19:11:02]

On 2017/03/05 23:52:33, reed1 wrote:
> Just reran this, and got some GM differences, especially in
> bleed_alpha_bmp_shader. Are these expected?

I believe the check I added against the extracted offset should alleviate this
now.

[lsalzman1, 2017-03-07 19:14:43]

The CQ bit was checked by lsalzman@mozilla.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 19:14:47]

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, 2017-03-07 19:14:48]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2017-03-08 01:14 UTC

[commit-bot: I haz the power, 2017-03-08 01:14:49]

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

[commit-bot: I haz the power, 2017-03-08 01:14:50]

Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer

[lsalzman1, 2017-03-10 16:40:01]

On 2017/03/07 19:14:48, commit-bot: I haz the power wrote:
> Note for Reviewers:
> The CQ is waiting for an approval.
> If you believe that the CL is not ready yet, or if you would like to L-G-T-M
> with comments then please uncheck the CQ checkbox.
> 
> Waiting for LGTM from valid reviewer(s) till 2017-03-08 01:14 UTC

Dry run confirms this is now fixed. Can we get this in?

[lsalzman1, 2017-03-22 18:52:17]

On 2017/03/10 16:40:01, lsalzman1 wrote:
> On 2017/03/07 19:14:48, commit-bot: I haz the power wrote:
> > Note for Reviewers:
> > The CQ is waiting for an approval.
> > If you believe that the CL is not ready yet, or if you would like to L-G-T-M
> > with comments then please uncheck the CQ checkbox.
> > 
> > Waiting for LGTM from valid reviewer(s) till 2017-03-08 01:14 UTC
> 
> Dry run confirms this is now fixed. Can we get this in?

Is anybody going to look into this at all?

[caryclark, 2017-03-27 16:54:25]

caryclark@google.com changed reviewers:
+ caryclark@google.com

[caryclark, 2017-03-27 16:56:20]

Sorry about dropping the ball on this. 

I'm a bit confused by the conversation up to this point, though.
Do you expect any GM to change?
We're still seeing changes in bleed_alpha_image_shader
This change may be closer to correct, though.
Do you have a GM or test that better shows the before / after?

[lsalzman1, 2017-03-29 21:22:00]

On 2017/03/27 16:56:20, caryclark wrote:
> Sorry about dropping the ball on this. 
> 
> I'm a bit confused by the conversation up to this point, though.
> Do you expect any GM to change?
> We're still seeing changes in bleed_alpha_image_shader
> This change may be closer to correct, though.
> Do you have a GM or test that better shows the before / after?

This is intended to just make sure the code stays on a fastpath where it should,
while it currently falls off it... so it's a change related to performance where
it shouldn't ideally make a perceptual difference.

At least on the trybot and when I run it locally, it comes out fine. How do I
need to run the test to make it fail?

[caryclark, 2017-03-30 12:16:59]

On 2017/03/29 21:22:00, lsalzman1 wrote:
> On 2017/03/27 16:56:20, caryclark wrote:
> > Sorry about dropping the ball on this. 
> > 
> > I'm a bit confused by the conversation up to this point, though.
> > Do you expect any GM to change?
> > We're still seeing changes in bleed_alpha_image_shader
> > This change may be closer to correct, though.
> > Do you have a GM or test that better shows the before / after?
> 
> This is intended to just make sure the code stays on a fastpath where it
should,
> while it currently falls off it... so it's a change related to performance
where
> it shouldn't ideally make a perceptual difference.
> 
> At least on the trybot and when I run it locally, it comes out fine. How do I
> need to run the test to make it fail?

It's possible that gold and trybots have bit-rotted with rietveld. We've moved
to using gerrit exclusively, so you may want to upload there in the future. 
skia-review.googlesource.com

To generate the diff locally, I ran this on Windows 10:

ninja -C out/release dm
./out/release/dm.exe -m bleed_alpha_image_shader -w ~/nomoz
git cl patch --rietveld 2089583002
ninja -C out/release dm
./out/release/dm.exe -m bleed_alpha_image_shader -w ~/moz
ninja -C out/release skdiff
./out/release/skdiff ~/moz ~/nomoz ~/diffmoz

Since this is a performance CL, there should be a noticeable change in some
nanobench result. Have you seen something encouraging there?

[lsalzman1, 2017-03-30 20:22:22]

On 2017/03/30 12:16:59, caryclark wrote:
> On 2017/03/29 21:22:00, lsalzman1 wrote:
> > On 2017/03/27 16:56:20, caryclark wrote:
> > > Sorry about dropping the ball on this. 
> > > 
> > > I'm a bit confused by the conversation up to this point, though.
> > > Do you expect any GM to change?
> > > We're still seeing changes in bleed_alpha_image_shader
> > > This change may be closer to correct, though.
> > > Do you have a GM or test that better shows the before / after?
> > 
> > This is intended to just make sure the code stays on a fastpath where it
> should,
> > while it currently falls off it... so it's a change related to performance
> where
> > it shouldn't ideally make a perceptual difference.
> > 
> > At least on the trybot and when I run it locally, it comes out fine. How do
I
> > need to run the test to make it fail?
> 
> It's possible that gold and trybots have bit-rotted with rietveld. We've moved
> to using gerrit exclusively, so you may want to upload there in the future. 
> http://skia-review.googlesource.com
> 
> To generate the diff locally, I ran this on Windows 10:
> 
> ninja -C out/release dm
> ./out/release/dm.exe -m bleed_alpha_image_shader -w ~/nomoz
> git cl patch --rietveld 2089583002
> ninja -C out/release dm
> ./out/release/dm.exe -m bleed_alpha_image_shader -w ~/moz
> ninja -C out/release skdiff
> ./out/release/skdiff ~/moz ~/nomoz ~/diffmoz
> 
> Since this is a performance CL, there should be a noticeable change in some
> nanobench result. Have you seen something encouraging there?

Where we noticed this was in our own internal profiling. Basically, any attempt
to use an offset with drawBitmapRect would cause us to fall onto the bitmap
shader path, rather than use the drawBitmap path that would internally try to
draw as a sprite (and thus eliminate some copies). In the end this caused enough
frustration that we decided to just not use this part of Skia, via some slightly
ugly workarounds, since it was so finicky about inputs because of this bug.

[caryclark, 2017-03-30 20:30:37]

Thanks for the additional explanation.

Does this CL remove the need for your workarounds, or is there more work to do?

[lsalzman1, 2017-03-31 18:21:02]

On 2017/03/30 20:30:37, caryclark wrote:
> Thanks for the additional explanation.
> 
> Does this CL remove the need for your workarounds, or is there more work to
do?

It would simplify some things on our end knowing that drawImageRect would be
smarter about dispatching to the fast-path where it can, yes.

[caryclark, 2017-03-31 18:28:59]

Since this does change some GM results, the change may need to be wrapped to
protect Chrome and Android tests from failing until their tests are
re-baselined. I'll also look for an existing nanobench test to verify that this
improves dispatching to the fast-path so this doesn't regress in the future.

[caryclark, 2017-03-31 18:47:54]

This is the result of running nanobench without, and then with, this CL.
The median column shows a slight improvement in 8888 and no change in gl.

curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  42/42  MB     1       1.45ms  1.47ms  1.47ms  1.51ms  1%      OOo.oo..o.     
8888    DrawLattice_Src100_Dst1000_Rects15
  49/49  MB     64      3.5us   78.5us  70.8us  79us    33%     .OOOOOOOOO     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$./out/release/nanobench.exe -m DrawLattice_Src100_Dst1000_Rects15
Timer overhead: 31.9ns
curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  40/40  MB     1       1.47ms  1.66ms  1.6ms   1.79ms  7%      .oOooo....     
8888    DrawLattice_Src100_Dst1000_Rects15
  41/41  MB     64      4.04us  76.9us  70us    78.4us  33%     .OOOOOOOOO     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$./out/release/nanobench.exe -m DrawLattice_Src100_Dst1000_Rects15
Timer overhead: 28.5ns
curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  40/40  MB     1       1.49ms  1.65ms  1.6ms   1.71ms  5%      o.oOOOO...     
8888    DrawLattice_Src100_Dst1000_Rects15
  46/46  MB     66      74.4us  77.7us  117us   475us   107%    .........O     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$./out/release/nanobench.exe -m DrawLattice_Src100_Dst1000_Rects15
Timer overhead: 29.3ns
curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  40/40  MB     1       1.48ms  1.6ms   1.58ms  1.68ms  4%      ooo....oOO     
8888    DrawLattice_Src100_Dst1000_Rects15
  46/46  MB     66      74.4us  78.2us  119us   490us   109%    .........O     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$./out/release/nanobench.exe -m DrawLattice_Src100_Dst1000_Rects15
Timer overhead: 28.2ns
curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  40/40  MB     1       1.49ms  1.56ms  1.58ms  1.71ms  5%      oo.....oOO     
8888    DrawLattice_Src100_Dst1000_Rects15
  46/46  MB     66      74.1us  78.6us  117us   467us   105%    .........O     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$./out/release/nanobench.exe -m DrawLattice_Src100_Dst1000_Rects15
Timer overhead: 29ns
curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  40/40  MB     1       1.43ms  1.49ms  1.5ms   1.68ms  5%      ..o...oo.O     
8888    DrawLattice_Src100_Dst1000_Rects15
  46/46  MB     66      75.1us  78.1us  118us   480us   108%    .........O     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$
/c/puregit$
/c/puregit$ninja -C out/release nanobench
ninja: Entering directory `out/release'
[1/3] compile ../../src/core/SkBitmapDevice.cpp
[2/3] link skia.lib
[3/3] link nanobench.exe
   Creating library ./nanobench.lib and object ./nanobench.exp
/c/puregit$./out/release/nanobench.exe -m DrawLattice_Src100_Dst1000_Rects15
Timer overhead: 28.9ns
curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  40/40  MB     1       1.36ms  1.46ms  1.47ms  1.71ms  7%      o.....oOoo     
8888    DrawLattice_Src100_Dst1000_Rects15
  46/46  MB     66      73.7us  78.2us  116us   465us   105%    .........O     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$./out/release/nanobench.exe -m DrawLattice_Src100_Dst1000_Rects15
Timer overhead: 28.5ns
curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  40/40  MB     1       1.36ms  1.39ms  1.39ms  1.43ms  1%      oo.O.ooooo     
8888    DrawLattice_Src100_Dst1000_Rects15
  46/46  MB     65      73.8us  78.9us  125us   541us   116%    .........O     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$./out/release/nanobench.exe -m DrawLattice_Src100_Dst1000_Rects15
Timer overhead: 31.6ns
curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  40/40  MB     1       1.37ms  1.41ms  1.42ms  1.48ms  3%      o.O.oo.oO.     
8888    DrawLattice_Src100_Dst1000_Rects15
  41/41  MB     64      3.58us  78.4us  70.8us  80.3us  33%     .OOOOOOOOO     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$./out/release/nanobench.exe -m DrawLattice_Src100_Dst1000_Rects15
Timer overhead: 29.5ns
curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  40/40  MB     1       1.37ms  1.39ms  1.4ms   1.44ms  2%      oO.O.oooO.     
8888    DrawLattice_Src100_Dst1000_Rects15
  42/42  MB     64      3.5us   79us    71.2us  79.7us  33%     .OOOOOOOOO     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$./out/release/nanobench.exe -m DrawLattice_Src100_Dst1000_Rects15
Timer overhead: 28.4ns
curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  40/40  MB     1       1.36ms  1.39ms  1.43ms  1.57ms  6%      .o.....O.O     
8888    DrawLattice_Src100_Dst1000_Rects15
  46/46  MB     66      73.3us  78.4us  117us   469us   106%    .........O     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$./out/release/nanobench.exe -m DrawLattice_Src100_Dst1000_Rects15
Timer overhead: 28.7ns
curr/maxrss     loops   min     median  mean    max     stddev  samples        
config  bench
No context was available matching config type and options.
  40/40  MB     1       1.36ms  1.39ms  1.39ms  1.46ms  2%      .o.Oooo.O.     
8888    DrawLattice_Src100_Dst1000_Rects15
  42/42  MB     64      3.49us  78.8us  70.9us  79.3us  33%     .OOOOOOOOO     
gl      DrawLattice_Src100_Dst1000_Rects15
/c/puregit$

[caryclark, 2017-03-31 20:08:34]

lgtm

Ran chrome layout tests with and without change and saw no meaningful
difference.

[caryclark, 2017-03-31 20:09:15]

The CQ bit was checked by caryclark@google.com

[commit-bot: I haz the power, 2017-03-31 20:09:23]

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-03-31 20:46:47]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490990955457160,
"parent_rev": "ec53c636b781830cb6146bb32106d4ee44651fcc", "commit_rev":
"ea9bc0c07b5dae78a9a449d7d7a07fc79262d41a"}

[commit-bot: I haz the power, 2017-03-31 20:46:52]

Description was changed from

==========
Fix the size check for the drawBitmap fast-path in
SkBitmapDevice::drawBitmapRect. It would fail when the source rectangle had a
non-zero offset, in which case it would compare the source rectangle with the
offset to the extracted bitmap size, which always fails. The only thing that
should matter is that the source rectangle and extract bitmap have the same
size, since the offset gets added onto the matrix.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2089583002
==========

to

==========
Fix the size check for the drawBitmap fast-path in
SkBitmapDevice::drawBitmapRect. It would fail when the source rectangle had a
non-zero offset, in which case it would compare the source rectangle with the
offset to the extracted bitmap size, which always fails. The only thing that
should matter is that the source rectangle and extract bitmap have the same
size, since the offset gets added onto the matrix.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2089583002

Review-Url: https://codereview.chromium.org/2089583002
Committed:
https://skia.googlesource.com/skia/+/ea9bc0c07b5dae78a9a449d7d7a07fc79262d41a
==========

[commit-bot: I haz the power, 2017-03-31 20:46:55]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/ea9bc0c07b5dae78a9a449d7d7a07fc79262d41a

[mtklein_C, 2017-04-01 01:36:29]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2787263005/ by mtklein@chromium.org.

The reason for reverting is: I agree the diffs are not meaningful, but
nevertheless their existence is blocking the roll into Chrome.

(https://storage.googleapis.com/chromium-layout-test-archives/linux_trusty_bli...).

[caryclark, 2017-04-03 11:40:07]

Please bracket your change in an #ifdef so that it can be checked-in with the
current behavior. Once baselines are relanded, the #ifdef can be removed.

[lsalzman1, 2017-04-04 18:48:58]

On 2017/04/03 11:40:07, caryclark wrote:
> Please bracket your change in an #ifdef so that it can be checked-in with the
> current behavior. Once baselines are relanded, the #ifdef can be removed.

Okay, I added a guard so that you must set the #define to get the new behavior.
Is this sufficient, or would it be better to have it go the other way with an
SK_SUPPORT_LEGACY_FOO-style define?

[caryclark, 2017-04-05 13:42:04]

The difference between the two styles is whether the fix should be on in Skia
and off in Chrome and Android, or off in all three.

The advantage to turning it on in Skia and subsequently rebaselining the Gold
images is that the code becomes active and any problems will more likely be
found earlier, and the fix will not bit-rot.

Your style (which is fine) will only benefit clients that your code explicitly.

[caryclark, 2017-04-05 13:42:35]

> Your style (which is fine) will only benefit clients that enable your code
explicitly.

[caryclark, 2017-04-05 13:43:23]

Description was changed from

==========
Fix the size check for the drawBitmap fast-path in
SkBitmapDevice::drawBitmapRect. It would fail when the source rectangle had a
non-zero offset, in which case it would compare the source rectangle with the
offset to the extracted bitmap size, which always fails. The only thing that
should matter is that the source rectangle and extract bitmap have the same
size, since the offset gets added onto the matrix.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2089583002

Review-Url: https://codereview.chromium.org/2089583002
Committed:
https://skia.googlesource.com/skia/+/ea9bc0c07b5dae78a9a449d7d7a07fc79262d41a
==========

to

==========
Fix the size check for the drawBitmap fast-path in
SkBitmapDevice::drawBitmapRect. It would fail when the source rectangle had a
non-zero offset, in which case it would compare the source rectangle with the
offset to the extracted bitmap size, which always fails. The only thing that
should matter is that the source rectangle and extract bitmap have the same
size, since the offset gets added onto the matrix.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2089583002

Review-Url: https://codereview.chromium.org/2089583002
Committed:
https://skia.googlesource.com/skia/+/ea9bc0c07b5dae78a9a449d7d7a07fc79262d41a
==========

[caryclark, 2017-04-05 16:52:38]

The CQ bit was checked by caryclark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 16:52:42]

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

[caryclark, 2017-04-05 16:59:47]

https://codereview.chromium.org/2089583002/diff/40001/src/core/SkBitmapDevice...
File src/core/SkBitmapDevice.cpp (right):

https://codereview.chromium.org/2089583002/diff/40001/src/core/SkBitmapDevice...
src/core/SkBitmapDevice.cpp:310: SkRect extractedBitmapBounds =
SkRect::MakeIWH(bitmapPtr->width(), bitmapPtr->height());
Can you change this back to

SkRect extractedBitmapBounds;
extractedBitmapBounds.isetWH(bitmapPtr->width(), bitmapPtr->height());	

That will make it clear to the casual observer that the old code path is
unchanged.

[commit-bot: I haz the power, 2017-04-05 17:22:39]

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

[commit-bot: I haz the power, 2017-04-05 17:22:40]

Dry run: This issue passed the CQ dry run.

[lsalzman1, 2017-04-05 19:14:30]

On 2017/04/05 16:59:47, caryclark wrote:
>
https://codereview.chromium.org/2089583002/diff/40001/src/core/SkBitmapDevice...
> File src/core/SkBitmapDevice.cpp (right):
> 
>
https://codereview.chromium.org/2089583002/diff/40001/src/core/SkBitmapDevice...
> src/core/SkBitmapDevice.cpp:310: SkRect extractedBitmapBounds =
> SkRect::MakeIWH(bitmapPtr->width(), bitmapPtr->height());
> Can you change this back to
> 
> SkRect extractedBitmapBounds;
> extractedBitmapBounds.isetWH(bitmapPtr->width(), bitmapPtr->height());	
> 
> That will make it clear to the casual observer that the old code path is
> unchanged.

Done.

[caryclark, 2017-04-05 19:16:30]

lgtm

[lsalzman1, 2017-04-06 04:12:12]

The CQ bit was checked by lsalzman@mozilla.com

[commit-bot: I haz the power, 2017-04-06 04:12:16]

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-04-06 04:42:54]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491451932333810,
"parent_rev": "45dcc0c42c4059c3b533b3e91d6c7fe67f6eefc7", "commit_rev":
"c0e52f44aac391da632d88d9702aea4f41cfe17a"}

[commit-bot: I haz the power, 2017-04-06 04:42:57]

Description was changed from

==========
Fix the size check for the drawBitmap fast-path in
SkBitmapDevice::drawBitmapRect. It would fail when the source rectangle had a
non-zero offset, in which case it would compare the source rectangle with the
offset to the extracted bitmap size, which always fails. The only thing that
should matter is that the source rectangle and extract bitmap have the same
size, since the offset gets added onto the matrix.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2089583002

Review-Url: https://codereview.chromium.org/2089583002
Committed:
https://skia.googlesource.com/skia/+/ea9bc0c07b5dae78a9a449d7d7a07fc79262d41a
==========

to

==========
Fix the size check for the drawBitmap fast-path in
SkBitmapDevice::drawBitmapRect. It would fail when the source rectangle had a
non-zero offset, in which case it would compare the source rectangle with the
offset to the extracted bitmap size, which always fails. The only thing that
should matter is that the source rectangle and extract bitmap have the same
size, since the offset gets added onto the matrix.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2089583002

Review-Url: https://codereview.chromium.org/2089583002
Committed:
https://skia.googlesource.com/skia/+/ea9bc0c07b5dae78a9a449d7d7a07fc79262d41a
Review-Url: https://codereview.chromium.org/2089583002
Committed:
https://skia.googlesource.com/skia/+/c0e52f44aac391da632d88d9702aea4f41cfe17a
==========

[commit-bot: I haz the power, 2017-04-06 04:42:59]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/c0e52f44aac391da632d88d9702aea4f41cfe17a