use drawRect when drawing a bitmap with anti-aliasing on a non-msaa target

use drawRect when drawing a bitmap with anti-aliasing on a non-msaa target

If the dest isn't pixel aligned, or if a non 90 degree rotation is
present, we need to use drawRect() for drawing anti-aliased bitmaps on
non-msaa targets or the edges won't be anti-aliased as intended.

Note: If the bitmap size is bigger than max texture size we fall back to drawBitmapCommon.

Original-Author: Henry Song <henrysong@samsung.com>;

Committed: https://skia.googlesource.com/skia/+/367e1867b2d6901e3327d0707738d2bc7d13826e

[derekf, 2014-10-21 19:44:50]

derekf@osg.samsung.com changed reviewers:
+ robertphillips@google.com

[robertphillips, 2014-10-22 18:34:20]

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

[bsalomon, 2014-10-22 18:47:03]

https://codereview.chromium.org/649313003/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/649313003/diff/1/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1056: fContext->getMatrix().mapRect(&rect, srcRect);
Why does it matter if the src rect is aligned?

Also, this seems like it will break large bitmaps (> max texture size) that
currently get tiled. For those we need to apply AA just to the exterior edges of
the tiles.

There is a real problem here, that we don't antialias the edges of draw rect
calls. That definitely needs to be fixed but I think the tiling issue will
require a somewhat more complex solution.

[derekf, 2014-11-04 20:11:10]

Patchset #2 (id:20001) has been deleted

[derekf, 2014-11-04 20:15:03]

On 2014/10/22 18:47:03, bsalomon wrote:
> https://codereview.chromium.org/649313003/diff/1/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
https://codereview.chromium.org/649313003/diff/1/src/gpu/SkGpuDevice.cpp#newc...
> src/gpu/SkGpuDevice.cpp:1056: fContext->getMatrix().mapRect(&rect, srcRect);
> Why does it matter if the src rect is aligned?

Heh, good point.

> Also, this seems like it will break large bitmaps (> max texture size) that
> currently get tiled. For those we need to apply AA just to the exterior edges
of
> the tiles.

Hmm, it indeed does break.

> There is a real problem here, that we don't antialias the edges of draw rect
> calls. That definitely needs to be fixed but I think the tiling issue will
> require a somewhat more complex solution.

Are you interested in a partial solution (patch set 2) that falls back to
un-antialiased if the size > max texture size?

I'm not sure how to write the complete fix.

[derekf, 2014-11-20 16:34:14]

Patchset #3 (id:60001) has been deleted

[derekf, 2014-11-20 16:39:14]

PTAL..

Patch set 2 accidentally lacked the dest rect checks.

(As with patch set 2 large bitmaps are still rendered as they were previously,
without anti-aliased edges)

[bsalomon, 2014-11-26 14:22:48]

Hey Derek, sorry for taking so long on this. It slipped under my radar. It seems
ok to me. But can you do a before/after run and update
expectations/gm/ignored-tests.txt for tests that will have changed pixels as a
result?

My procedure for doing that is:

./gyp_skia
git checkout master
ninja -C out/Release dm
out/Release/dm --config gpu msaa4 --notests -w master_gm_out
git checkout <branch with change>
ninja -C out/Release dm
out/Release/dm --config gpu msaa4 --notests -w change_gm_out
mkdir cmp
ninja -C out/Release skdiff
out/Release/skdiff master_gm_out change_gm_out cmp

then open cmp/index.html to see the changes. If the changes look ok add the test
names to the file mentioned above.

https://codereview.chromium.org/649313003/diff/80001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/649313003/diff/80001/src/gpu/SkGpuDevice.cpp#...
src/gpu/SkGpuDevice.cpp:1064: // if the render target is not msaa and draw is
antialiased,
Can we update the comment to say something like "The tiled bitmap code path
doesn't currently support antialiased edges." ?


I just want to be clear that this is work around and not a fundamental
limitation (We could in the future make the tiled bitmap code do antialiasing on
the outer tile edges).

[derekf, 2014-12-01 21:53:17]

On 2014/11/26 14:22:48, bsalomon wrote:
> Hey Derek, sorry for taking so long on this. It slipped under my radar. It
seems
> ok to me. But can you do a before/after run and update
> expectations/gm/ignored-tests.txt for tests that will have changed pixels as a
> result?
> 
> My procedure for doing that is:
> 
> ./gyp_skia
> git checkout master
> ninja -C out/Release dm
> out/Release/dm --config gpu msaa4 --notests -w master_gm_out
> git checkout <branch with change>
> ninja -C out/Release dm
> out/Release/dm --config gpu msaa4 --notests -w change_gm_out
> mkdir cmp
> ninja -C out/Release skdiff
> out/Release/skdiff master_gm_out change_gm_out cmp
> 
> then open cmp/index.html to see the changes. If the changes look ok add the
test
> names to the file mentioned above.
> 
> https://codereview.chromium.org/649313003/diff/80001/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
https://codereview.chromium.org/649313003/diff/80001/src/gpu/SkGpuDevice.cpp#...
> src/gpu/SkGpuDevice.cpp:1064: // if the render target is not msaa and draw is
> antialiased,
> Can we update the comment to say something like "The tiled bitmap code path
> doesn't currently support antialiased edges." ?
> 
> 
> I just want to be clear that this is work around and not a fundamental
> limitation (We could in the future make the tiled bitmap code do antialiasing
on
> the outer tile edges).

PTAL

I've changed the comment and added an ignore for the one test it changed.
The test (drawbitmapmatrix) changed as expected, with anti-aliasing along the
edges of (most) of the bitmaps drawn.

The one drawn with a setSinCos() matrix isn't anti-aliased, but I'm wondering if
that's a bug somewhere else?  The matrix is being configured with setSinCos(.5,
2).  I thought the values passed to setSinCos() were supposed to be in the range
-1..1? (as sin and cos values are?)  Is this just a broken matrix that draws a
rotated rectangle but confuses all the machinery that decides if AA is necessary
or not?


Thanks for the workflow btw, very helpful. :)

[bsalomon, 2014-12-01 21:56:03]

On 2014/12/01 21:53:17, derekf wrote:
> On 2014/11/26 14:22:48, bsalomon wrote:
> > Hey Derek, sorry for taking so long on this. It slipped under my radar. It
> seems
> > ok to me. But can you do a before/after run and update
> > expectations/gm/ignored-tests.txt for tests that will have changed pixels as
a
> > result?
> > 
> > My procedure for doing that is:
> > 
> > ./gyp_skia
> > git checkout master
> > ninja -C out/Release dm
> > out/Release/dm --config gpu msaa4 --notests -w master_gm_out
> > git checkout <branch with change>
> > ninja -C out/Release dm
> > out/Release/dm --config gpu msaa4 --notests -w change_gm_out
> > mkdir cmp
> > ninja -C out/Release skdiff
> > out/Release/skdiff master_gm_out change_gm_out cmp
> > 
> > then open cmp/index.html to see the changes. If the changes look ok add the
> test
> > names to the file mentioned above.
> > 
> > https://codereview.chromium.org/649313003/diff/80001/src/gpu/SkGpuDevice.cpp
> > File src/gpu/SkGpuDevice.cpp (right):
> > 
> >
>
https://codereview.chromium.org/649313003/diff/80001/src/gpu/SkGpuDevice.cpp#...
> > src/gpu/SkGpuDevice.cpp:1064: // if the render target is not msaa and draw
is
> > antialiased,
> > Can we update the comment to say something like "The tiled bitmap code path
> > doesn't currently support antialiased edges." ?
> > 
> > 
> > I just want to be clear that this is work around and not a fundamental
> > limitation (We could in the future make the tiled bitmap code do
antialiasing
> on
> > the outer tile edges).
> 
> PTAL
> 
> I've changed the comment and added an ignore for the one test it changed.
> The test (drawbitmapmatrix) changed as expected, with anti-aliasing along the
> edges of (most) of the bitmaps drawn.
> 
> The one drawn with a setSinCos() matrix isn't anti-aliased, but I'm wondering
if
> that's a bug somewhere else?  The matrix is being configured with
setSinCos(.5,
> 2).  I thought the values passed to setSinCos() were supposed to be in the
range
> -1..1? (as sin and cos values are?)  Is this just a broken matrix that draws a
> rotated rectangle but confuses all the machinery that decides if AA is
necessary
> or not?
> 

That is definitely a weird way to call setSinCos() but it creates a valid matrix
and everything *should* work from there...

> 
> Thanks for the workflow btw, very helpful. :)

[bsalomon, 2014-12-02 17:57:31]

lgtm

[derekf, 2014-12-02 18:05:15]

On 2014/12/02 17:57:31, bsalomon wrote:
> lgtm

Thanks...  One last extremely verbose question before I click the commit
button... :)

I've since been able to draw a simple rectangle with AA turned on that isn't
anti-aliased by using the same matrix operations.

I just took drawbitmapmatrix, removed all the bitmap stuff and replaced it with:
SkRect rect = SkRect::MakeXYWH(0, 0, 64, 64);
SkMatrix matrix;
paint.setAntiAlias(true);

canvas->translate(SkIntToScalar(100), 0);
matrix.reset();
matrix.setSinCos(SK_ScalarHalf, SkIntToScalar(2));
canvas->concat(matrix);
canvas->drawRect(rect, paint);

And it spits out a roughly 30 degree rotated, 2x-ish scaled black rectangle with
no anti-aliasing.

Change the 2 to a 2.1 and it's anti-aliased.  translate by a non-integer value
and it's anti-aliased.

This seems to be because the last clause in apply_aa_to_rect in
src/gpu/GrContext.cpp is seeing devBoundRect be a rectangle with all integer
co-ordinates.  As this is a rotated rectangle, I don't understand why the axis
aligned bounding rectangle is even being tested, so I'm backing slowly away
assuming I'm missing something here...

But that leaves me with (finally, the question):
Should I be filing a bug for that?

(or am I just really misunderstanding something)

[bsalomon, 2014-12-02 18:07:49]

On 2014/12/02 18:05:15, derekf wrote:
> On 2014/12/02 17:57:31, bsalomon wrote:
> > lgtm
> 
> Thanks...  One last extremely verbose question before I click the commit
> button... :)
> 
> I've since been able to draw a simple rectangle with AA turned on that isn't
> anti-aliased by using the same matrix operations.
> 
> I just took drawbitmapmatrix, removed all the bitmap stuff and replaced it
with:
> SkRect rect = SkRect::MakeXYWH(0, 0, 64, 64);
> SkMatrix matrix;
> paint.setAntiAlias(true);
> 
> canvas->translate(SkIntToScalar(100), 0);
> matrix.reset();
> matrix.setSinCos(SK_ScalarHalf, SkIntToScalar(2));
> canvas->concat(matrix);
> canvas->drawRect(rect, paint);
> 
> And it spits out a roughly 30 degree rotated, 2x-ish scaled black rectangle
with
> no anti-aliasing.
> 
> Change the 2 to a 2.1 and it's anti-aliased.  translate by a non-integer value
> and it's anti-aliased.
> 
> This seems to be because the last clause in apply_aa_to_rect in
> src/gpu/GrContext.cpp is seeing devBoundRect be a rectangle with all integer
> co-ordinates.  As this is a rotated rectangle, I don't understand why the axis
> aligned bounding rectangle is even being tested, so I'm backing slowly away
> assuming I'm missing something here...
> 
> But that leaves me with (finally, the question):
> Should I be filing a bug for that?
> 
> (or am I just really misunderstanding something)

Ugh, yeah' that is totally wrong. Can you file a bug about that?

[derekf, 2014-12-02 18:49:36]

On 2014/12/02 18:07:49, bsalomon wrote:
> On 2014/12/02 18:05:15, derekf wrote:
> > On 2014/12/02 17:57:31, bsalomon wrote:
> > > lgtm
> > 
> > Thanks...  One last extremely verbose question before I click the commit
> > button... :)
> > 
> > I've since been able to draw a simple rectangle with AA turned on that isn't
> > anti-aliased by using the same matrix operations.
> > 
> > I just took drawbitmapmatrix, removed all the bitmap stuff and replaced it
> with:
> > SkRect rect = SkRect::MakeXYWH(0, 0, 64, 64);
> > SkMatrix matrix;
> > paint.setAntiAlias(true);
> > 
> > canvas->translate(SkIntToScalar(100), 0);
> > matrix.reset();
> > matrix.setSinCos(SK_ScalarHalf, SkIntToScalar(2));
> > canvas->concat(matrix);
> > canvas->drawRect(rect, paint);
> > 
> > And it spits out a roughly 30 degree rotated, 2x-ish scaled black rectangle
> with
> > no anti-aliasing.
> > 
> > Change the 2 to a 2.1 and it's anti-aliased.  translate by a non-integer
value
> > and it's anti-aliased.
> > 
> > This seems to be because the last clause in apply_aa_to_rect in
> > src/gpu/GrContext.cpp is seeing devBoundRect be a rectangle with all integer
> > co-ordinates.  As this is a rotated rectangle, I don't understand why the
axis
> > aligned bounding rectangle is even being tested, so I'm backing slowly away
> > assuming I'm missing something here...
> > 
> > But that leaves me with (finally, the question):
> > Should I be filing a bug for that?
> > 
> > (or am I just really misunderstanding something)
> 
> Ugh, yeah' that is totally wrong. Can you file a bug about that?

We're testing a patch to fix it right now, I'll open an issue shortly.

Thanks

[derekf, 2014-12-02 18:49:44]

The CQ bit was checked by derekf@osg.samsung.com

[commit-bot: I haz the power, 2014-12-02 18:51:45]

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

[commit-bot: I haz the power, 2014-12-02 19:02:09]

Committed patchset #4 (id:100001) as
https://skia.googlesource.com/skia/+/367e1867b2d6901e3327d0707738d2bc7d13826e