SkCanvas::drawImage is the new way for drawing an SkImage to a Canvas

SkCanvas::drawImage is the new way for drawing a SkImage to a Canvas

BUG=skia:2947
Committed: https://skia.googlesource.com/skia/+/432789972c1e1f8a66165c75a250dba1853efa08

Committed: https://skia.googlesource.com/skia/+/b5fae93d72c7b6480f83fd8a7b534cd1fdfcd49a

[Rémi Piotaix, 2014-09-17 22:59:06]

piotaixr@chromium.org changed reviewers:
+ junov@chromium.org, reed@google.com

[Rémi Piotaix, 2014-09-17 22:59:56]

PTAL

Chromium compiles sucessfully with this patch applied to Skia.

[reed1, 2014-09-18 18:25:27]

reed@google.com changed reviewers:
+ bsalomon@google.com

[reed1, 2014-09-18 18:25:28]

I like the direction of this.

Lets rename the (now private) SkImage::draw to something like
SkImage::drawIntoCanvas or something that is more different from "draw". Note we
recently renamed SkPicture::draw to SkPicture::playback, and it made reading
that code much clearer.

Still don't understand why we need drawBitmapFromSkImage yet.

[Rémi Piotaix, 2014-09-18 18:41:08]

On 2014/09/18 18:25:28, reed1 wrote:
> I like the direction of this.
> 
> Lets rename the (now private) SkImage::draw to something like
> SkImage::drawIntoCanvas or something that is more different from "draw". Note
we
> recently renamed SkPicture::draw to SkPicture::playback, and it made reading
> that code much clearer.
> 
> Still don't understand why we need drawBitmapFromSkImage yet.

We need a way to tell the SkCanvas that the SkBitmap it is going to draw is
guaranteed immutable.

[Justin Novosad, 2014-09-18 18:47:31]

On 2014/09/18 18:41:08, Rémi Piotaix wrote:
> On 2014/09/18 18:25:28, reed1 wrote:
> > I like the direction of this.
> > 
> > Lets rename the (now private) SkImage::draw to something like
> > SkImage::drawIntoCanvas or something that is more different from "draw".
Note
> we
> > recently renamed SkPicture::draw to SkPicture::playback, and it made reading
> > that code much clearer.
> > 
> > Still don't understand why we need drawBitmapFromSkImage yet.
> 
> We need a way to tell the SkCanvas that the SkBitmap it is going to draw is
> guaranteed immutable.

(...) even though it is not marked as immutable.

This function basically comes with the contract that the calling code will hold
a ref on the associated SkImage until the Bitmap is consumed.

[Rémi Piotaix, 2014-09-18 18:57:17]

On 2014/09/18 18:25:28, reed1 wrote:
> I like the direction of this.
> 
> Lets rename the (now private) SkImage::draw to something like
> SkImage::drawIntoCanvas or something that is more different from "draw". Note
we
> recently renamed SkPicture::draw to SkPicture::playback, and it made reading
> that code much clearer.

Ok, lets go for drawToCanvas (i think that the word 'draw' is the good one).

For the methods that draw 'rectToRect', what is the good name ? I saw 'draw' in
SkImage, 'drawRect' and 'drawRectToRect'...

[Rémi Piotaix, 2014-09-19 21:13:49]

New patch: don't depend on https://codereview.chromium.org/577063004/ anymore.

What is the best name for the method currently named
'drawBitmapRectToRectFromSkImage'? 
- drawBitmapFromSkImage(SkBitmap, SkRect, SkRect, SkPaint)
- drawBitmapRectFromSkImage(...)
- drawBitmapRectToRectFromSkImage(...)

Should i rename the SkImage::drawIntoCanvas(SkCanvas, SkRect, SkRect, SkPaint)
to add 'Rect' (or 'RectToRect') in its name?

[reed1, 2014-09-19 21:20:30]

On 2014/09/19 21:13:49, Rémi Piotaix wrote:
> New patch: don't depend on https://codereview.chromium.org/577063004/ anymore.
> 
> What is the best name for the method currently named
> 'drawBitmapRectToRectFromSkImage'? 
> - drawBitmapFromSkImage(SkBitmap, SkRect, SkRect, SkPaint)
> - drawBitmapRectFromSkImage(...)
> - drawBitmapRectToRectFromSkImage(...)
> 
> Should i rename the SkImage::drawIntoCanvas(SkCanvas, SkRect, SkRect, SkPaint)
> to add 'Rect' (or 'RectToRect') in its name?

Actually, I like just drawFooRect, and think we should rename SkCanvas methods
at some point (e.g. drawBitmapRectToRect).

[Rémi Piotaix, 2014-09-22 15:44:08]

On 2014/09/19 21:20:30, reed1 wrote:
> Actually, I like just drawFooRect, and think we should rename SkCanvas methods
> at some point (e.g. drawBitmapRectToRect).

Done :)

[Rémi Piotaix, 2014-09-22 19:02:28]

SkImage::draw() removed.
public SkImage_Base::draw[Rect]() added that calls SkCanvas::drawBitmap()
(overidden in SkImage_Raster/Gpu)

SkCanvas::drawImage() calls SkImage_Base::draw() by default, to be overidden in
SkCanvas subclasses.

[reed1, 2014-09-22 19:26:06]

There are an awful lot of renaming going on in this CL, in addition to the new
functionality. That makes it much harder to focus on the diff. Can we just see
the change that adds drawImage[Rect] to SkCanvas? e.g. renaming the image_base
methods is completely separable.

[Rémi Piotaix, 2014-09-22 19:48:20]

On 2014/09/22 19:26:06, reed1 wrote:
> There are an awful lot of renaming going on in this CL, in addition to the new
> functionality. That makes it much harder to focus on the diff. Can we just see
> the change that adds drawImage[Rect] to SkCanvas? e.g. renaming the image_base
> methods is completely separable.

Done.

[reed1, 2014-09-22 21:00:29]

1. Will you also be updating all of the canvas subclasses to know about the new
virtuals drawImage? (e.g. picture recording, pipe, NWay, etc) ? This is, I
think, the heart of this CL.

2. I think you can simplify this CL even more, by just moving the SkImage draw
methods to private, and declaring SkCanvas a friend. You may not need to edit
any of the src/image files if you do that.

[Rémi Piotaix, 2014-09-22 21:07:03]

On 2014/09/22 21:00:29, reed1 wrote:
> 1. Will you also be updating all of the canvas subclasses to know about the
new
> virtuals drawImage? (e.g. picture recording, pipe, NWay, etc) ? This is, I
> think, the heart of this CL.

I will, but in an other CL. If we do nothing, the current behavior is unchanged,
right?

> 2. I think you can simplify this CL even more, by just moving the SkImage draw
> methods to private, and declaring SkCanvas a friend. You may not need to edit
> any of the src/image files if you do that.

Ok, i'll split that in 2 CL, then.

[Rémi Piotaix, 2014-09-22 21:23:43]

On 2014/09/22 21:07:03, Rémi Piotaix wrote:
> On 2014/09/22 21:00:29, reed1 wrote:
> > 1. Will you also be updating all of the canvas subclasses to know about the
> new
> > virtuals drawImage? (e.g. picture recording, pipe, NWay, etc) ? This is, I
> > think, the heart of this CL.
> 
> I will, but in an other CL. If we do nothing, the current behavior is
unchanged,
> right?
> 
> > 2. I think you can simplify this CL even more, by just moving the SkImage
draw
> > methods to private, and declaring SkCanvas a friend. You may not need to
edit
> > any of the src/image files if you do that.
> 
> Ok, i'll split that in 2 CL, then.

Done

[reed1, 2014-09-23 12:29:44]

lgtm w/ question about including SkImage.h instead of SkImage_Base.h

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

https://codereview.chromium.org/583453002/diff/140001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:15: #include "SkImage_Base.h"
Can we just include SkImage.h ?

[Rémi Piotaix, 2014-09-23 19:47:24]

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

https://codereview.chromium.org/583453002/diff/140001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:15: #include "SkImage_Base.h"
On 2014/09/23 12:29:44, reed1 wrote:
> Can we just include SkImage.h ?

Done.

[Rémi Piotaix, 2014-09-23 19:47:31]

The CQ bit was checked by piotaixr@chromium.org

[commit-bot: I haz the power, 2014-09-23 21:01:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/583453002/160001

[commit-bot: I haz the power, 2014-09-23 21:10:55]

Committed patchset #9 (id:160001) as 432789972c1e1f8a66165c75a250dba1853efa08

[borenet, 2014-09-24 18:02:21]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/598133002/ by borenet@google.com.

The reason for reverting is: Broke ChromiumOS Ozone builder:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

Reverting to unblock DEPS roll..

[Rémi Piotaix, 2014-09-24 18:47:28]

PTAL-Again :)

SkImage::draw is now public (temporarily) to not break the ChromiumOs Ozone
builder.

[Justin Novosad, 2014-09-24 18:55:38]

On 2014/09/24 18:47:28, Rémi Piotaix wrote:
> PTAL-Again :)
> 
> SkImage::draw is now public (temporarily) to not break the ChromiumOs Ozone
> builder.

lgtm

[reed1, 2014-09-24 19:09:20]

lets land a fix to dri so we can make it private again asap

[Rémi Piotaix, 2014-09-24 19:55:00]

The CQ bit was checked by piotaixr@chromium.org

[commit-bot: I haz the power, 2014-09-24 19:55:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/583453002/180001

[commit-bot: I haz the power, 2014-09-24 20:03:33]

Committed patchset #10 (id:180001) as b5fae93d72c7b6480f83fd8a7b534cd1fdfcd49a