Add SkImage->draw() call with src and dst rects.

Add SkImage->draw() call with src and dst rects.

Committed: http://code.google.com/p/skia/source/detail?r=10237

Committed: http://code.google.com/p/skia/source/detail?r=10274

[arbesfeld, 2013-07-18 19:32:28]

PTAL. This patch is needed for https://codereview.chromium.org/19705006/

[Justin Novosad, 2013-07-18 21:54:37]

https://codereview.chromium.org/19729007/diff/1/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/19729007/diff/1/include/core/SkImage.h#newcode85
include/core/SkImage.h:85: void draw(SkCanvas*, SkRect src, SkRect dst, const
SkPaint*);
Use "const SkRect&" for src and dst

https://codereview.chromium.org/19729007/diff/1/src/image/SkImagePriv.cpp
File src/image/SkImagePriv.cpp (right):

https://codereview.chromium.org/19729007/diff/1/src/image/SkImagePriv.cpp#new...
src/image/SkImagePriv.cpp:145: void SkImagePrivDrawPicture(SkCanvas* canvas,
SkPicture* picture,
You need to write a good test for this function.  I<m not sure we need this
override to be implemented. You could leave it empty with something like: 
SkASSERT(0); // Not implemented

https://codereview.chromium.org/19729007/diff/1/src/image/SkImagePriv.cpp#new...
src/image/SkImagePriv.cpp:147: int saveCount = canvas->getSaveCount();
No need for this call. This value is returned by saveLayer

https://codereview.chromium.org/19729007/diff/1/src/image/SkImagePriv.cpp#new...
src/image/SkImagePriv.cpp:149: canvas->clipRect(src);
Bug: This clip does not get undone.  Also I think it needs to be applied just
before the call to drawPictures.

https://codereview.chromium.org/19729007/diff/1/src/image/SkImagePriv.cpp#new...
src/image/SkImagePriv.cpp:151: canvas->translate(dst.x(), dst.y());
This is not the right transform to map src to dst.

https://codereview.chromium.org/19729007/diff/1/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/19729007/diff/1/src/image/SkImage_Codec.cpp#n...
src/image/SkImage_Codec.cpp:52: void SkImage_Codec::onDraw(SkCanvas* canvas,
SkRect src, SkRect dst, const SkPaint* paint) {
Test or leave unimplemented

https://codereview.chromium.org/19729007/diff/1/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/19729007/diff/1/src/image/SkImage_Gpu.cpp#new...
src/image/SkImage_Gpu.cpp:65: void SkImage_Gpu::onDraw(SkCanvas* canvas, SkRect
src, SkRect dst,
Write a test that exercises this path

https://codereview.chromium.org/19729007/diff/1/src/image/SkImage_Raster.cpp
File src/image/SkImage_Raster.cpp (right):

https://codereview.chromium.org/19729007/diff/1/src/image/SkImage_Raster.cpp#...
src/image/SkImage_Raster.cpp:113: void SkImage_Raster::onDraw(SkCanvas* canvas,
SkRect src, SkRect dst, const SkPaint* paint) {
Test please.

[arbesfeld, 2013-07-18 22:39:45]

Any recommendation for what to call the class? NativeImageSkia is already
taken...

This is kind of tricky to test in Skia. Would it suffice to show that both the
gpu and raster paths are explored with LayoutTests in Blink?

[reed1, 2013-07-19 14:03:45]

Primary comments:

1. clearly document the expected behavior in SkImage.h
2. write one or more gms to demonstrate this (exercising all backends if
possible)
3. see if you can imagine a unittest (may be harder to justify)

https://codereview.chromium.org/19729007/diff/1/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/19729007/diff/1/include/core/SkImage.h#newcode85
include/core/SkImage.h:85: void draw(SkCanvas*, SkRect src, SkRect dst, const
SkPaint*);
On 2013/07/18 21:54:37, junov wrote:
> Use "const SkRect&" for src and dst

How about

const SkRect* src
const SkRect& dst

So the src rect can be optional

https://codereview.chromium.org/19729007/diff/1/src/image/SkImagePriv.cpp
File src/image/SkImagePriv.cpp (right):

https://codereview.chromium.org/19729007/diff/1/src/image/SkImagePriv.cpp#new...
src/image/SkImagePriv.cpp:150: canvas->saveLayer(&dst, paint);
Why saveLayer instead of just save?

https://codereview.chromium.org/19729007/diff/1/src/image/SkImagePriv.cpp#new...
src/image/SkImagePriv.cpp:151: canvas->translate(dst.x(), dst.y());
On 2013/07/18 21:54:37, junov wrote:
> This is not the right transform to map src to dst. 

SkMatrix has a mapRectToRect helper function you can use, which you could then
concat() onto the canvas.

[arbesfeld, 2013-07-19 19:15:53]

>
https://codereview.chromium.org/19729007/diff/1/src/image/SkImagePriv.cpp#new...
> src/image/SkImagePriv.cpp:147: int saveCount = canvas->getSaveCount();
> No need for this call. This value is returned by saveLayer

Either way I will have to define the variable, so I thought it was cleaner to
also initialize it up here. This follows the structure of the other
SkImagePrivDrawPicture as well.

I made the other changes that were asked for. Since the test requires
re-baselining, I think I will need someone with commit privileges to commit for
me.

[reed1, 2013-07-19 19:32:29]

https://codereview.chromium.org/19729007/diff/14001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/19729007/diff/14001/include/core/SkImage.h#ne...
include/core/SkImage.h:89: *  filled with transparent black pixels.
// See SkCanvas::drawBitmapRectToRect for similar behavior.

https://codereview.chromium.org/19729007/diff/14001/src/image/SkImage_Base.h
File src/image/SkImage_Base.h (right):

https://codereview.chromium.org/19729007/diff/14001/src/image/SkImage_Base.h#...
src/image/SkImage_Base.h:18: virtual void onDraw(SkCanvas*, const SkRect* src,
const SkRect& dst, const SkPaint*) = 0;
Bad practice to overload a virtual with diff parameters (too easy to have one
shadow the other). Please create a new name, e.g. onDrawRectToRect or something
like that.

https://codereview.chromium.org/19729007/diff/14001/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/19729007/diff/14001/src/image/SkImage_Codec.c...
src/image/SkImage_Codec.cpp:53: SkASSERT(0); // Not implemented
Seems like we can easily implement this, following what was done just above in
onDraw(x,y)

[arbesfeld, 2013-07-19 20:15:29]

I made those changes. The SkImage_Codec test is only configured for "mike's"
machine (see drawJpeg in image.cpp).

[reed1, 2013-07-19 20:28:52]

lgtm w/ various nits about efficiency and exceeding 80/100 cols

https://codereview.chromium.org/19729007/diff/25001/src/image/SkImagePriv.cpp
File src/image/SkImagePriv.cpp (right):

https://codereview.chromium.org/19729007/diff/25001/src/image/SkImagePriv.cpp...
src/image/SkImagePriv.cpp:152: pictureBounds.set(0, 0,
minor, but might move the initialization of pictureBounds inside the else where
it is used, to avoid the cost of the int-to-float conversion when not needed.

https://codereview.chromium.org/19729007/diff/25001/src/image/SkImage_Base.h
File src/image/SkImage_Base.h (right):

https://codereview.chromium.org/19729007/diff/25001/src/image/SkImage_Base.h#...
src/image/SkImage_Base.h:18: virtual void onDrawRectToRect(SkCanvas*, const
SkRect* src, const SkRect& dst, const SkPaint*) = 0;
nit: exceed 80/100 cols. This file started at 80, so should try to keep it there
if possible.

https://codereview.chromium.org/19729007/diff/25001/src/image/SkImage_Codec.cpp
File src/image/SkImage_Codec.cpp (right):

https://codereview.chromium.org/19729007/diff/25001/src/image/SkImage_Codec.c...
src/image/SkImage_Codec.cpp:52: void SkImage_Codec::onDrawRectToRect(SkCanvas*
canvas, const SkRect* src, const SkRect& dst, const SkPaint* paint) {
exceeding 80/100 cols

[commit-bot: I haz the power, 2013-07-22 15:41:02]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/arbesfeld@chromium.org/19729007/36011

[commit-bot: I haz the power, 2013-07-22 16:16:41]

Change committed as 10237

[reed1, 2013-07-22 17:04:12]

https://chromiumcodereview.appspot.com/19729007/diff/36011/gm/image.cpp
File gm/image.cpp (right):

https://chromiumcodereview.appspot.com/19729007/diff/36011/gm/image.cpp#newco...
gm/image.cpp:43: src->set(0, 0, image->width(), image->height());
Yikes! We are calling set() on src when we know that src is NULL

https://chromiumcodereview.appspot.com/19729007/diff/36011/gm/image.cpp#newco...
gm/image.cpp:44: dst.set(0, 0, size.width(), size.height());
This, and the call above, are generating warnings of implicit int->float
conversions.

Either use SkIntToScalar() for the parameters, or you can use iset() which
accepts int parameters.

[arbesfeld, 2013-07-22 17:16:39]

On 2013/07/22 17:04:12, reed1 wrote:
> https://chromiumcodereview.appspot.com/19729007/diff/36011/gm/image.cpp
> File gm/image.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/19729007/diff/36011/gm/image.cpp#newco...
> gm/image.cpp:43: src->set(0, 0, image->width(), image->height());
> Yikes! We are calling set() on src when we know that src is NULL
> 
>
https://chromiumcodereview.appspot.com/19729007/diff/36011/gm/image.cpp#newco...
> gm/image.cpp:44: dst.set(0, 0, size.width(), size.height());
> This, and the call above, are generating warnings of implicit int->float
> conversions.
> 
> Either use SkIntToScalar() for the parameters, or you can use iset() which
> accepts int parameters.

Sorry about that. I just fixed these in patch set 10. Since this was already
committed, do I have to open a new issue? Also, do we need to re-baseline the
surface-image test?

[reed1, 2013-07-22 17:21:51]

If you need to rebaseline, that can/must happen *after* the code lands, and the
bots turn red

https://codereview.chromium.org/19729007/diff/44001/gm/image.cpp
File gm/image.cpp (right):

https://codereview.chromium.org/19729007/diff/44001/gm/image.cpp#newcode45
gm/image.cpp:45: canvas->scale(size.width() * 1.0f / image->width(),
Do you need this canvas scale in the case where we pass a dst rect?

https://codereview.chromium.org/19729007/diff/44001/gm/image.cpp#newcode48
gm/image.cpp:48: image->draw(canvas, NULL, dst, NULL);
Hmmm, now you're not exercising passing a non-null src rect. Its easy enough to
just place one on the stack, and pass its address.

e.g. your old version, but make src a SkRect (not a ptr), and pass &src

[arbesfeld, 2013-07-22 17:38:42]

Okay, fixed.

[reed1, 2013-07-22 18:19:49]

can you attach or email a screen-shot of the new gm?

https://codereview.chromium.org/19729007/diff/50001/gm/image.cpp
File gm/image.cpp (right):

https://codereview.chromium.org/19729007/diff/50001/gm/image.cpp#newcode38
gm/image.cpp:38: SkImage* image = SkImage::NewEncodedData(data);
SkAutoTUnref<SkImage>

https://codereview.chromium.org/19729007/diff/50001/gm/image.cpp#newcode49
gm/image.cpp:49: image->draw(canvas, 0, 0, NULL);
Does this call draw on-top-of the previous call, or is it translated somehow?

https://codereview.chromium.org/19729007/diff/50001/gm/image.cpp#newcode50
gm/image.cpp:50: image->unref();
shouldn't need this with SkAutoTUnref

[arbesfeld, 2013-07-22 18:40:29]

That particular draw call (drawJpeg) has this comment: 
    
    // TODO: Make this draw a file that is checked in, so it can
    // be exercised on machines other than mike's. Will require a
    // rebaseline.

So I haven't been able to see the results on my own machine. I emailed a
screenshot of the baselines.

[reed1, 2013-07-22 19:01:35]

if we're not testing the jpeg function (due to non-portable image location),
lets not modify it (since we can't test the modifications). Other than that,
lgtm

[commit-bot: I haz the power, 2013-07-23 13:34:14]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/arbesfeld@chromium.org/19729007/26011

[commit-bot: I haz the power, 2013-07-23 15:52:20]

Change committed as 10274