Add onDrawBitmapLattice(), avoid unnecessary bitmap->image copy

Add onDrawBitmapLattice(), avoid unnecessary bitmap->image copy

out/Release/nanobench --match Lattice --config gpu --ms 3000
3.42ms -> 17.2us

For reference, a loop over drawBitmapRects (which is what
Android currently does) is about 13us.

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

Committed: https://skia.googlesource.com/skia/+/168820625c35a8c19f66c661efcbce7a5e334837

[msarett, 2016-08-03 14:42:37]

Description was changed from

==========
Add onDrawBitmapLattice(), avoid unnecessary bitmap->image copy

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

to

==========
Add onDrawBitmapLattice(), avoid unnecessary bitmap->image copy

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

[msarett, 2016-08-03 14:51:56]

Patchset #1 (id:1) has been deleted

[msarett, 2016-08-03 14:59:26]

Patchset #1 (id:20001) has been deleted

[msarett, 2016-08-03 15:07:04]

Description was changed from

==========
Add onDrawBitmapLattice(), avoid unnecessary bitmap->image copy

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

to

==========
Add onDrawBitmapLattice(), avoid unnecessary bitmap->image copy

out/Release/nanobench --match Lattice --config gpu --ms 3000
3.42ms -> 17.2us

For reference, a loop over drawBitmapRects (which is what
Android does) is about 13 us.

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

[msarett, 2016-08-03 15:07:40]

Description was changed from

==========
Add onDrawBitmapLattice(), avoid unnecessary bitmap->image copy

out/Release/nanobench --match Lattice --config gpu --ms 3000
3.42ms -> 17.2us

For reference, a loop over drawBitmapRects (which is what
Android does) is about 13 us.

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

to

==========
Add onDrawBitmapLattice(), avoid unnecessary bitmap->image copy

out/Release/nanobench --match Lattice --config gpu --ms 3000
3.42ms -> 17.2us

For reference, a loop over drawBitmapRects (which is what
Android currently does) is about 13us.

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

[msarett, 2016-08-03 15:07:56]

msarett@google.com changed reviewers:
+ djsollen@google.com, mtklein@google.com, reed@google.com

[msarett, 2016-08-03 15:07:57]

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

https://codereview.chromium.org/2205273003/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:1996: void SkCanvas::drawImageLattice(const SkImage*
image, const Lattice& lattice, const SkRect& dst,
Just moved this.

https://codereview.chromium.org/2205273003/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:2533: void SkCanvas::onDrawImageLattice(const SkImage*
image, const Lattice& lattice, const SkRect& dst,
Also moved.

https://codereview.chromium.org/2205273003/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:2556: void SkCanvas::onDrawBitmapLattice(const SkBitmap&
bitmap, const Lattice& lattice,
This is new.  Could we also use this implementation for SkPictureRecord and
SkRecorder?  Or do they need to copy when converting to SkImage?

[mtklein, 2016-08-03 15:11:51]

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

https://codereview.chromium.org/2205273003/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:2556: void SkCanvas::onDrawBitmapLattice(const SkBitmap&
bitmap, const Lattice& lattice,
On 2016/08/03 15:07:57, msarett wrote:
> This is new.  Could we also use this implementation for SkPictureRecord and
> SkRecorder?  Or do they need to copy when converting to SkImage?

They need to copy if immutable (though I think SkPictureRecord will never
actually see a mutable bitmap today... they should all have been previously
snapped by SkRecorder).

https://codereview.chromium.org/2205273003/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:2558: sk_sp<SkImage> image = sk_sp<SkImage>((SkImage*) new
SkImage_Raster(bitmap, true));
I think we spell this,

   sk_sp<SkImage> image = SkMakeImageFromRasterBitmap(bitmap,
kNever_ForceCopyMode)


Are we sure we know this is safe here?  I thought we had to be down in the
raster device before we know it's not going to be retained somehow (e.g. by PDF
or GPU devices).

[mtklein, 2016-08-03 15:12:19]

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

https://codereview.chromium.org/2205273003/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:2556: void SkCanvas::onDrawBitmapLattice(const SkBitmap&
bitmap, const Lattice& lattice,
On 2016/08/03 15:11:51, mtklein wrote:
> On 2016/08/03 15:07:57, msarett wrote:
> > This is new.  Could we also use this implementation for SkPictureRecord and
> > SkRecorder?  Or do they need to copy when converting to SkImage?
> 
> They need to copy if immutable (though I think SkPictureRecord will never
> actually see a mutable bitmap today... they should all have been previously
> snapped by SkRecorder).

They need to copy if *mutable...

[reed1, 2016-08-03 15:21:06]

also agree to use SkMakeRasterImage... instead of needing to expose image_raster
header

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

https://codereview.chromium.org/2205273003/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:2558: sk_sp<SkImage> image = sk_sp<SkImage>((SkImage*) new
SkImage_Raster(bitmap, true));
On 2016/08/03 15:11:51, mtklein wrote:
> I think we spell this,
> 
>    sk_sp<SkImage> image = SkMakeImageFromRasterBitmap(bitmap,
> kNever_ForceCopyMode)
> 
> 
> Are we sure we know this is safe here?  I thought we had to be down in the
> raster device before we know it's not going to be retained somehow (e.g. by
PDF
> or GPU devices).

I had suggested this point in the pipeline to commit to a volatile image, but
perhaps you're right. Not sure about pdf, but gpu is allowed to not look at the
image pixels until later. Maybe we need to push down further to the device level
virtual (i.e. add drawBitmapLattice to device), at least when the bitmap is not
immutable...

[msarett, 2016-08-03 15:49:23]

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

https://codereview.chromium.org/2205273003/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:2556: void SkCanvas::onDrawBitmapLattice(const SkBitmap&
bitmap, const Lattice& lattice,
On 2016/08/03 15:12:19, mtklein wrote:
> On 2016/08/03 15:11:51, mtklein wrote:
> > On 2016/08/03 15:07:57, msarett wrote:
> > > This is new.  Could we also use this implementation for SkPictureRecord
and
> > > SkRecorder?  Or do they need to copy when converting to SkImage?
> > 
> > They need to copy if immutable (though I think SkPictureRecord will never
> > actually see a mutable bitmap today... they should all have been previously
> > snapped by SkRecorder).
> 
> They need to copy if *mutable...

Gotcha, I think the Picture/Record impls are as they should be then.

https://codereview.chromium.org/2205273003/diff/40001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:2558: sk_sp<SkImage> image = sk_sp<SkImage>((SkImage*) new
SkImage_Raster(bitmap, true));
On 2016/08/03 15:21:05, reed1 wrote:
> On 2016/08/03 15:11:51, mtklein wrote:
> > I think we spell this,
> > 
> >    sk_sp<SkImage> image = SkMakeImageFromRasterBitmap(bitmap,
> > kNever_ForceCopyMode)
> > 
> > 
> > Are we sure we know this is safe here?  I thought we had to be down in the
> > raster device before we know it's not going to be retained somehow (e.g. by
> PDF
> > or GPU devices).
> 
> I had suggested this point in the pipeline to commit to a volatile image, but
> perhaps you're right. Not sure about pdf, but gpu is allowed to not look at
the
> image pixels until later. Maybe we need to push down further to the device
level
> virtual (i.e. add drawBitmapLattice to device), at least when the bitmap is
not
> immutable...

Changed to use SkMakeImageFromRasterBitmap.  Pushing bitmap virtual down to
device level.

https://codereview.chromium.org/2205273003/diff/60001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/2205273003/diff/60001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:200: sk_sp<SkImage> image =
SkMakeImageFromRasterBitmap(bitmap, kNever_ForceCopyMode);
So now we convert image->bitmap here.  No need to worry about pdf since that
will be caught by pdf canvas.

But this implementation still might not be safe for gpu?  That's strange because
this optimization is targeted for gpu.  Regardless, we can now override this
from SkGpuDevice.

[reed1, 2016-08-15 20:34:44]

https://codereview.chromium.org/2205273003/diff/60001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/2205273003/diff/60001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:200: sk_sp<SkImage> image =
SkMakeImageFromRasterBitmap(bitmap, kNever_ForceCopyMode);
Since this trick with kNever is only safe for Raster, perhaps we should have a
loop here just like you have for drawImageLattice.

https://codereview.chromium.org/2205273003/diff/60001/src/pdf/SkPDFCanvas.cpp
File src/pdf/SkPDFCanvas.cpp (right):

https://codereview.chromium.org/2205273003/diff/60001/src/pdf/SkPDFCanvas.cpp...
src/pdf/SkPDFCanvas.cpp:99: void SkPDFCanvas::onDrawBitmapLattice(const
SkBitmap& bitmap,
can remove this if we put this loop into BaseDevice

[msarett, 2016-08-15 20:43:03]

https://codereview.chromium.org/2205273003/diff/60001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

https://codereview.chromium.org/2205273003/diff/60001/src/core/SkDevice.cpp#n...
src/core/SkDevice.cpp:200: sk_sp<SkImage> image =
SkMakeImageFromRasterBitmap(bitmap, kNever_ForceCopyMode);
On 2016/08/15 20:34:43, reed1 wrote:
> Since this trick with kNever is only safe for Raster, perhaps we should have a
> loop here just like you have for drawImageLattice.

sgtm, that seems like the right think to do.

I'm hoping to follow-up by overriding these on SkGPUDevice.

https://codereview.chromium.org/2205273003/diff/60001/src/pdf/SkPDFCanvas.cpp
File src/pdf/SkPDFCanvas.cpp (right):

https://codereview.chromium.org/2205273003/diff/60001/src/pdf/SkPDFCanvas.cpp...
src/pdf/SkPDFCanvas.cpp:99: void SkPDFCanvas::onDrawBitmapLattice(const
SkBitmap& bitmap,
On 2016/08/15 20:34:43, reed1 wrote:
> can remove this if we put this loop into BaseDevice

Don't think so.  SkPDFDevice asserts(false) in onDrawImageRect() and
onDrawBitmapRect().  We need to override before we hit device level.

[msarett, 2016-08-15 20:43:15]

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

[commit-bot: I haz the power, 2016-08-15 20:43:18]

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-08-15 20:44:50]

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

[commit-bot: I haz the power, 2016-08-15 20:44:50]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
  skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[msarett, 2016-08-15 21:37:40]

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

[commit-bot: I haz the power, 2016-08-15 21:37:53]

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-08-15 21:48:02]

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

[commit-bot: I haz the power, 2016-08-15 21:48:03]

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

[msarett, 2016-08-15 22:05:41]

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

[commit-bot: I haz the power, 2016-08-15 22:05:52]

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

[msarett, 2016-08-15 22:17:15]

Added overrides for SkLiteRecorder

[commit-bot: I haz the power, 2016-08-15 22:26:30]

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

[commit-bot: I haz the power, 2016-08-15 22:26:31]

Dry run: This issue passed the CQ dry run.

[mtklein, 2016-08-15 23:14:59]

The Lite stuff lgtm.

[reed1, 2016-08-16 11:28:47]

lgtm w/ minor suggestions/questions

https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp
File src/core/SkLiteDL.cpp (right):

https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp#...
src/core/SkLiteDL.cpp:629: int xs = lattice.fXCount, ys = lattice.fYCount;
minor nit: possibly just call drawImageLattice,so we don't have to duplicate the
logic for recording it.

e.g.

this->drawImageLattice(SkImage::MakeFromBitmap(bm), ...);

https://codereview.chromium.org/2205273003/diff/120001/src/pdf/SkPDFCanvas.cpp
File src/pdf/SkPDFCanvas.cpp (right):

https://codereview.chromium.org/2205273003/diff/120001/src/pdf/SkPDFCanvas.cp...
src/pdf/SkPDFCanvas.cpp:99: void SkPDFCanvas::onDrawBitmapLattice(const
SkBitmap& bitmap,
General question for PDF canvas (not strictly related to this CL)

- can it fold bitmaps->images at this level, to reduce the number of internal
methods? (as pictures do)?

[mtklein, 2016-08-16 12:36:05]

https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp
File src/core/SkLiteDL.cpp (right):

https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp#...
src/core/SkLiteDL.cpp:629: int xs = lattice.fXCount, ys = lattice.fYCount;
On 2016/08/16 11:28:47, reed1 wrote:
> minor nit: possibly just call drawImageLattice,so we don't have to duplicate
the
> logic for recording it.
> 
> e.g.
> 
> this->drawImageLattice(SkImage::MakeFromBitmap(bm), ...);

That can be made correct but will wobble the ref count more than needed. 
Instead of created at 1 and staying that way, it'll go up to 2 then back down to
1.

[reed1, 2016-08-16 13:39:02]

On 2016/08/16 12:36:05, mtklein wrote:
> https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp
> File src/core/SkLiteDL.cpp (right):
> 
>
https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp#...
> src/core/SkLiteDL.cpp:629: int xs = lattice.fXCount, ys = lattice.fYCount;
> On 2016/08/16 11:28:47, reed1 wrote:
> > minor nit: possibly just call drawImageLattice,so we don't have to duplicate
> the
> > logic for recording it.
> > 
> > e.g.
> > 
> > this->drawImageLattice(SkImage::MakeFromBitmap(bm), ...);
> 
> That can be made correct but will wobble the ref count more than needed. 
> Instead of created at 1 and staying that way, it'll go up to 2 then back down
to
> 1.

We just finished mallocing a new image object. Perhaps we won't be able to
measure that wobble. Not huge either way, but duplicating the logic/code might
not be worth that gain (would have to measure to know).

[mtklein, 2016-08-16 13:48:24]

On 2016/08/16 13:39:02, reed1 wrote:
> On 2016/08/16 12:36:05, mtklein wrote:
> > https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp
> > File src/core/SkLiteDL.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp#...
> > src/core/SkLiteDL.cpp:629: int xs = lattice.fXCount, ys = lattice.fYCount;
> > On 2016/08/16 11:28:47, reed1 wrote:
> > > minor nit: possibly just call drawImageLattice,so we don't have to
duplicate
> > the
> > > logic for recording it.
> > > 
> > > e.g.
> > > 
> > > this->drawImageLattice(SkImage::MakeFromBitmap(bm), ...);
> > 
> > That can be made correct but will wobble the ref count more than needed. 
> > Instead of created at 1 and staying that way, it'll go up to 2 then back
down
> to
> > 1.
> 
> We just finished mallocing a new image object. Perhaps we won't be able to
> measure that wobble. Not huge either way, but duplicating the logic/code might
> not be worth that gain (would have to measure to know).

I also don't particularly care about the performance of recording drawBitmap*. 
I'd just like this code to be consistent.  If we do make this change, please
update all 4 drawBitmap*() methods to proxy into their corresponding
drawImage*().

[reed1, 2016-08-16 13:57:21]

On 2016/08/16 13:48:24, mtklein wrote:
> On 2016/08/16 13:39:02, reed1 wrote:
> > On 2016/08/16 12:36:05, mtklein wrote:
> > >
https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp
> > > File src/core/SkLiteDL.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp#...
> > > src/core/SkLiteDL.cpp:629: int xs = lattice.fXCount, ys = lattice.fYCount;
> > > On 2016/08/16 11:28:47, reed1 wrote:
> > > > minor nit: possibly just call drawImageLattice,so we don't have to
> duplicate
> > > the
> > > > logic for recording it.
> > > > 
> > > > e.g.
> > > > 
> > > > this->drawImageLattice(SkImage::MakeFromBitmap(bm), ...);
> > > 
> > > That can be made correct but will wobble the ref count more than needed. 
> > > Instead of created at 1 and staying that way, it'll go up to 2 then back
> down
> > to
> > > 1.
> > 
> > We just finished mallocing a new image object. Perhaps we won't be able to
> > measure that wobble. Not huge either way, but duplicating the logic/code
might
> > not be worth that gain (would have to measure to know).
> 
> I also don't particularly care about the performance of recording drawBitmap*.

> I'd just like this code to be consistent.  If we do make this change, please
> update all 4 drawBitmap*() methods to proxy into their corresponding
> drawImage*().

I'm also fine exploring this idea (code sharing) after the current CL.

[msarett, 2016-08-16 14:45:36]

msarett@google.com changed reviewers:
+ halcanary@google.com

[msarett, 2016-08-16 14:45:37]

https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp
File src/core/SkLiteDL.cpp (right):

https://codereview.chromium.org/2205273003/diff/120001/src/core/SkLiteDL.cpp#...
src/core/SkLiteDL.cpp:629: int xs = lattice.fXCount, ys = lattice.fYCount;
On 2016/08/16 12:36:05, mtklein wrote:
> On 2016/08/16 11:28:47, reed1 wrote:
> > minor nit: possibly just call drawImageLattice,so we don't have to duplicate
> the
> > logic for recording it.
> > 
> > e.g.
> > 
> > this->drawImageLattice(SkImage::MakeFromBitmap(bm), ...);
> 
> That can be made correct but will wobble the ref count more than needed. 
> Instead of created at 1 and staying that way, it'll go up to 2 then back down
to
> 1.

Will leave as is in this CL.

[msarett, 2016-08-16 14:48:00]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-08-16 14:48:06]

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

[hal.canary, 2016-08-16 14:57:33]

https://codereview.chromium.org/2205273003/diff/120001/src/pdf/SkPDFCanvas.cpp
File src/pdf/SkPDFCanvas.cpp (right):

https://codereview.chromium.org/2205273003/diff/120001/src/pdf/SkPDFCanvas.cp...
src/pdf/SkPDFCanvas.cpp:99: void SkPDFCanvas::onDrawBitmapLattice(const
SkBitmap& bitmap,
On 2016/08/16 11:28:47, reed1 wrote:
> General question for PDF canvas (not strictly related to this CL)
> 
> - can it fold bitmaps->images at this level, to reduce the number of internal
> methods? (as pictures do)?

No!

You need to keep the bitmaps until we get to the de-duplication stage.  As long
as out clients are using bitmaps, we need to keep de-duping them.

[hal.canary, 2016-08-16 14:58:41]

PDF: LGTM

[commit-bot: I haz the power, 2016-08-16 16:31:11]

Description was changed from

==========
Add onDrawBitmapLattice(), avoid unnecessary bitmap->image copy

out/Release/nanobench --match Lattice --config gpu --ms 3000
3.42ms -> 17.2us

For reference, a loop over drawBitmapRects (which is what
Android currently does) is about 13us.

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

to

==========
Add onDrawBitmapLattice(), avoid unnecessary bitmap->image copy

out/Release/nanobench --match Lattice --config gpu --ms 3000
3.42ms -> 17.2us

For reference, a loop over drawBitmapRects (which is what
Android currently does) is about 13us.

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

Committed:
https://skia.googlesource.com/skia/+/168820625c35a8c19f66c661efcbce7a5e334837
==========

[commit-bot: I haz the power, 2016-08-16 16:31:12]

Committed patchset #5 (id:120001) as
https://skia.googlesource.com/skia/+/168820625c35a8c19f66c661efcbce7a5e334837