Fix the way we patch up the matrix for scaled images that aren't

Fix the way we patch up the matrix for scaled images that aren't
clamp/clamp

BUG=skia:2904
TBR=reed

Committed: https://skia.googlesource.com/skia/+/535e3b2025d3244c2ca48982d14d1d31f1d23c1f

[humper, 2014-10-23 20:34:11]

PTAL -- this turned out to be kind of a (very) stupid bug, although I still
don't fully understand why we need that postIDiv; left a TODO for reed@ to add
some much-needed comments to that line :)

I've verified that the SKP submitted with bug 2904 now renders correctly, and
included a GM that, while it doesn't exactly reproduce the failure in that bug,
did exhibit pretty wonky behavior in SampleApp if you use the up/down arrows to
adjust the scale.  The GM also works with this patch.

[reed1, 2014-10-24 14:21:13]

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

[reed1, 2014-10-24 14:21:14]

/*
 * Most of the scanline procs deal with "unit" texture coordinates, as this
makes it easy
 * to perform tiling modes (repeat = (x & 0xFFFF)). To generate those, we divide
the matrix
 * by its dimensions here.
 *
 * We don't do this if we're either trivial (can ignore the matrix) or clamping
in both X and Y
 * since clamping to width,height is just as easy as to 0xFFFF.
 */

[reed1, 2014-10-24 14:27:38]

https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp
File gm/tiledscaledbitmap.cpp (right):

https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp#new...
gm/tiledscaledbitmap.cpp:45: SkString resourcePath =
GetResourcePath(fFilename.c_str());
I think it is just as easy (and slightly more portable/tweakable) to just
generate the bitmap in code. That makes it trivial to create variations of the
test as desired in the future (e.g. images w/ or w/o per-pixel alpha, images in
different bit-depths, images that are small, large, pow-of-2, etc.).

Many of our GMs already do this (grep for make_bitmap or make_bm)

e.g.

static SkImage* make_image(...) {
    SkAutoTUnref<SkSurface> surface(SkSurface::NewRaster(info));
    SkPaint paint;
    surface->getCanvas()->drawCircle(..., paint);
    return surface->newImageSnapshot();
}

static SkBitmap make_bitmap(...) {
    SkBitmap bm;
    bm.allocPixels(info);
    SkCanvas canvas(bm);
    SkPaint paint;
    canvas.drawCircle(..., paint);
    return bm;
}

https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp#new...
gm/tiledscaledbitmap.cpp:90: static GM* MyFactory(void*) { return new
TiledScaledBitmapGM("circle.png"); }
nit: DEF_GM( return SkNEW_ARGS(TiledScaledBitmapGM, (...) ); )

[humper, 2014-10-24 15:05:30]

On 2014/10/24 14:27:38, reed1 wrote:
> https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp
> File gm/tiledscaledbitmap.cpp (right):
> 
>
https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp#new...
> gm/tiledscaledbitmap.cpp:45: SkString resourcePath =
> GetResourcePath(fFilename.c_str());
> I think it is just as easy (and slightly more portable/tweakable) to just
> generate the bitmap in code. That makes it trivial to create variations of the
> test as desired in the future (e.g. images w/ or w/o per-pixel alpha, images
in
> different bit-depths, images that are small, large, pow-of-2, etc.).
> 
> Many of our GMs already do this (grep for make_bitmap or make_bm)
> 
> e.g.
> 
> static SkImage* make_image(...) {
>     SkAutoTUnref<SkSurface> surface(SkSurface::NewRaster(info));
>     SkPaint paint;
>     surface->getCanvas()->drawCircle(..., paint);
>     return surface->newImageSnapshot();
> }
> 
> static SkBitmap make_bitmap(...) {
>     SkBitmap bm;
>     bm.allocPixels(info);
>     SkCanvas canvas(bm);
>     SkPaint paint;
>     canvas.drawCircle(..., paint);
>     return bm;
> }
> 
>
https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp#new...
> gm/tiledscaledbitmap.cpp:90: static GM* MyFactory(void*) { return new
> TiledScaledBitmapGM("circle.png"); }
> nit: DEF_GM( return SkNEW_ARGS(TiledScaledBitmapGM, (...) ); )

I agree with you that this should probably make its own image, especially since
the image itself isn't particularly
important.

Somewhere on my todo list is to make a set of helper routines for ALL GMs to be
able to generate various kinds of
bitmaps; would it be reasonable to land this GM with the enclosed PNG with the
understanding that when such
helper routines arrive I'll go through all existing GMs and clean up either any
bitmap creations or checked-in
images that could easily be created on the fly?

[humper, 2014-10-24 15:09:34]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-10-24 15:09:50]

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

[humper, 2014-10-24 15:10:02]

https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp
File gm/tiledscaledbitmap.cpp (right):

https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp#new...
gm/tiledscaledbitmap.cpp:90: static GM* MyFactory(void*) { return new
TiledScaledBitmapGM("circle.png"); }
On 2014/10/24 14:27:38, reed1 wrote:
> nit: DEF_GM( return SkNEW_ARGS(TiledScaledBitmapGM, (...) ); )

Done.

[commit-bot: I haz the power, 2014-10-24 15:21:22]

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

[commit-bot: I haz the power, 2014-10-24 15:21:23]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[reed1, 2014-10-24 15:33:30]

On 2014/10/24 15:05:30, humper wrote:
> On 2014/10/24 14:27:38, reed1 wrote:
> > https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp
> > File gm/tiledscaledbitmap.cpp (right):
> > 
> >
>
https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp#new...
> > gm/tiledscaledbitmap.cpp:45: SkString resourcePath =
> > GetResourcePath(fFilename.c_str());
> > I think it is just as easy (and slightly more portable/tweakable) to just
> > generate the bitmap in code. That makes it trivial to create variations of
the
> > test as desired in the future (e.g. images w/ or w/o per-pixel alpha, images
> in
> > different bit-depths, images that are small, large, pow-of-2, etc.).
> > 
> > Many of our GMs already do this (grep for make_bitmap or make_bm)
> > 
> > e.g.
> > 
> > static SkImage* make_image(...) {
> >     SkAutoTUnref<SkSurface> surface(SkSurface::NewRaster(info));
> >     SkPaint paint;
> >     surface->getCanvas()->drawCircle(..., paint);
> >     return surface->newImageSnapshot();
> > }
> > 
> > static SkBitmap make_bitmap(...) {
> >     SkBitmap bm;
> >     bm.allocPixels(info);
> >     SkCanvas canvas(bm);
> >     SkPaint paint;
> >     canvas.drawCircle(..., paint);
> >     return bm;
> > }
> > 
> >
>
https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp#new...
> > gm/tiledscaledbitmap.cpp:90: static GM* MyFactory(void*) { return new
> > TiledScaledBitmapGM("circle.png"); }
> > nit: DEF_GM( return SkNEW_ARGS(TiledScaledBitmapGM, (...) ); )
> 
> I agree with you that this should probably make its own image, especially
since
> the image itself isn't particularly
> important.
> 
> Somewhere on my todo list is to make a set of helper routines for ALL GMs to
be
> able to generate various kinds of
> bitmaps; would it be reasonable to land this GM with the enclosed PNG with the
> understanding that when such
> helper routines arrive I'll go through all existing GMs and clean up either
any
> bitmap creations or checked-in
> images that could easily be created on the fly?

Lets do it inline for this new GM, and then create a separate task/cl for
centralizing this sort of thing.

[humper, 2014-10-27 12:46:18]

On 2014/10/24 15:33:30, reed1 wrote:
> On 2014/10/24 15:05:30, humper wrote:
> > On 2014/10/24 14:27:38, reed1 wrote:
> > > https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp
> > > File gm/tiledscaledbitmap.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp#new...
> > > gm/tiledscaledbitmap.cpp:45: SkString resourcePath =
> > > GetResourcePath(fFilename.c_str());
> > > I think it is just as easy (and slightly more portable/tweakable) to just
> > > generate the bitmap in code. That makes it trivial to create variations of
> the
> > > test as desired in the future (e.g. images w/ or w/o per-pixel alpha,
images
> > in
> > > different bit-depths, images that are small, large, pow-of-2, etc.).
> > > 
> > > Many of our GMs already do this (grep for make_bitmap or make_bm)
> > > 
> > > e.g.
> > > 
> > > static SkImage* make_image(...) {
> > >     SkAutoTUnref<SkSurface> surface(SkSurface::NewRaster(info));
> > >     SkPaint paint;
> > >     surface->getCanvas()->drawCircle(..., paint);
> > >     return surface->newImageSnapshot();
> > > }
> > > 
> > > static SkBitmap make_bitmap(...) {
> > >     SkBitmap bm;
> > >     bm.allocPixels(info);
> > >     SkCanvas canvas(bm);
> > >     SkPaint paint;
> > >     canvas.drawCircle(..., paint);
> > >     return bm;
> > > }
> > > 
> > >
> >
>
https://codereview.chromium.org/675823002/diff/1/gm/tiledscaledbitmap.cpp#new...
> > > gm/tiledscaledbitmap.cpp:90: static GM* MyFactory(void*) { return new
> > > TiledScaledBitmapGM("circle.png"); }
> > > nit: DEF_GM( return SkNEW_ARGS(TiledScaledBitmapGM, (...) ); )
> > 
> > I agree with you that this should probably make its own image, especially
> since
> > the image itself isn't particularly
> > important.
> > 
> > Somewhere on my todo list is to make a set of helper routines for ALL GMs to
> be
> > able to generate various kinds of
> > bitmaps; would it be reasonable to land this GM with the enclosed PNG with
the
> > understanding that when such
> > helper routines arrive I'll go through all existing GMs and clean up either
> any
> > bitmap creations or checked-in
> > images that could easily be created on the fly?
> 
> Lets do it inline for this new GM, and then create a separate task/cl for
> centralizing this sort of thing.

Okay, done.  There's already a bug assigned to me for fixing this across all
GMs.

[humper, 2014-10-27 12:46:23]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-10-27 12:46:44]

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

[commit-bot: I haz the power, 2014-10-27 12:50:24]

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

[commit-bot: I haz the power, 2014-10-27 12:50:25]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[humper, 2014-10-27 12:53:32]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-10-27 12:53:54]

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

[commit-bot: I haz the power, 2014-10-27 12:59:25]

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

[commit-bot: I haz the power, 2014-10-27 12:59:26]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[humper, 2014-10-27 17:22:45]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-10-27 17:23:26]

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

[commit-bot: I haz the power, 2014-10-27 17:32:11]

Committed patchset #5 (id:80001) as 535e3b2025d3244c2ca48982d14d1d31f1d23c1f

[tfarina, 2014-10-30 15:30:16]

https://codereview.chromium.org/675823002/diff/80001/gm/tiledscaledbitmap.cpp
File gm/tiledscaledbitmap.cpp (right):

https://codereview.chromium.org/675823002/diff/80001/gm/tiledscaledbitmap.cpp...
gm/tiledscaledbitmap.cpp:10: #include "Resources.h"
looks like it is unused after make_bm().

https://codereview.chromium.org/675823002/diff/80001/gm/tiledscaledbitmap.cpp...
gm/tiledscaledbitmap.cpp:34: virtual SkString onShortName() {
SK_OVERRIDE?

https://codereview.chromium.org/675823002/diff/80001/gm/tiledscaledbitmap.cpp...
gm/tiledscaledbitmap.cpp:38: virtual SkISize onISize() {
SK_OVERRIDE?

https://codereview.chromium.org/675823002/diff/80001/gm/tiledscaledbitmap.cpp...
gm/tiledscaledbitmap.cpp:61: virtual void onDraw(SkCanvas* canvas) {
SK_OVERRIDE?