Separate out natively-texture image/bmp draws from cached-as-texture image/bmp draws

Separate out natively-texture image/bmp draws from cached-as-texture image/bmp draws

This makes texture-backed images and bitmaps down a new code path. It adds a pinch point via the texture adjuster that will be used to handle copied necessary for different texture targets. It also fixes bugs in the existing code exhibited by recent updates to the bleed GM. The plan is to move the the sw/generator-backed imgs/bmps on to this code path with future changes.

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

[bsalomon, 2015-11-05 18:47:00]

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

[joshualitt, 2015-11-05 19:53:12]

first pass, Robert should really handle the meat.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:298: bool canApplyAA = width >=0 ?
viewMatrix.rectStaysRect() :
view_matrix_ok_for_aa_stroke_rect?  Though I guess if this is only used once,
its not a big deal

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:346: GrDrawBatch* batch =
GrAAFillRectBatch::CreateWithLocalRect(
Is there a reason not to use SkAutoTUnref here?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:354: paint.getColor(),
extra spaces

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:376: this->drawBatch(&pipelineBuilder, batch);
Its outside the scope of this CL but it'd be nice to not have GrBatch spill out
into GrDrawContext.

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

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1758: clippedDstRect, *draw.fMatrix, srcToDstMatrix,
fClip, paint, constraint);
can this be made a helper? or do we want it to be separate because of the whole
image / bitmap thing?  Could maybe have a lambda to call to create the
imageTextureAdjuster if it succeeds.

[bsalomon, 2015-11-05 20:10:37]

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:298: bool canApplyAA = width >=0 ?
viewMatrix.rectStaysRect() :
On 2015/11/05 19:53:12, joshualitt wrote:
> view_matrix_ok_for_aa_stroke_rect?  Though I guess if this is only used once,
> its not a big deal

I only pulled the other one out in order to share it with additional callers. If
you think it adds clarity I can do it for the stroke case as well.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:346: GrDrawBatch* batch =
GrAAFillRectBatch::CreateWithLocalRect(
On 2015/11/05 19:53:12, joshualitt wrote:
> Is there a reason not to use SkAutoTUnref here?

I already had to branch on nullptr

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:354: paint.getColor(),
On 2015/11/05 19:53:12, joshualitt wrote:
> extra spaces

will fix

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:376: this->drawBatch(&pipelineBuilder, batch);
On 2015/11/05 19:53:12, joshualitt wrote:
> Its outside the scope of this CL but it'd be nice to not have GrBatch spill
out
> into GrDrawContext.

You mean not have GrDrawContext create GrBatches at all?

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

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1758: clippedDstRect, *draw.fMatrix, srcToDstMatrix,
fClip, paint, constraint);
On 2015/11/05 19:53:12, joshualitt wrote:
> can this be made a helper? or do we want it to be separate because of the
whole
> image / bitmap thing?  Could maybe have a lambda to call to create the
> imageTextureAdjuster if it succeeds.

I'm not sure I understand what you're asking/suggesting.

[robertphillips, 2015-11-05 20:13:24]

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp
File src/gpu/GrBlurUtils.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:171: pathPtr = tmpPath.get();
do we need to set volatility here ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:178: pathPtr = strokedPath;
here ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:185: pathIsMutable = true;
here ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:200: if
(maskFilter->canFilterMaskGPU(SkRRect::MakeRect(devPathPtr->getBounds()),
tab these guys left?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:211: if
(maskFilter->directFilterMaskGPU(context->textureProvider(),
tab these guys left too ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:232: if (maskFilter->filterMaskGPU(mask, viewMatrix,
maskRect,
on prior line ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:250: maskFilter, clipBounds, paint, style);
rm this "return;" and the \n after it ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:326: } else {
What happens if tmpPath was already activated by the preceding "prePathMatrix"
code path?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:329: pathPtr = tmpPath.get();
volatile ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:345: view_matrix_ok_for_aa_fill_rect(viewMatrix)) {
SkAutoTUnref<GrDrawBatch> batch( ...
?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrImageIDTextur...
File src/gpu/GrImageIDTextureAdjuster.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrImageIDTextur...
src/gpu/GrImageIDTextureAdjuster.cpp:28: GrMakeKeyFromImageID(&baseKey,
fBmp->getGenerationID(),
align ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
File src/gpu/GrTextureParamsAdjuster.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:184: static DomainMode
determine_domain_mode(
tab these over one to avoid lining up with the code ...

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:191: SkRect* domainRect) {
... or maybe a '\n' here ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:235: 
// We will inset the domain half a pixel b.c. ...

?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:236: static const SkScalar kDomainInset =
0.5f;
can -> to ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:247: // constraint rect, and we're allowed
to filter across the constraint rect boundary. So
from -> to ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:284: }
What happens to the sides that are still out at the 'setLargest' values ?

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
File src/gpu/GrTextureParamsAdjuster.h (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.h:99: *                                        
clipped to the content area already.
filterConstraint ?
Could use a less cryptic explanation ...

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.h:104: *                                        
width, just the raw coords).
// An in/out param that is ...
// On input it ... on output it ...

?

[robertphillips, 2015-11-05 20:49:39]

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/SkGpuDevice_dra...
File src/gpu/SkGpuDevice_drawTexture.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/SkGpuDevice_dra...
src/gpu/SkGpuDevice_drawTexture.cpp:31: const SkPaint& paint) {
'\n' ?

[robertphillips, 2015-11-05 20:53:14]

provisional lgtm if you want it. I'll take another look tomorrow morning.

[bsalomon, 2015-11-06 15:24:26]

Updated, moved common code from draw(Image|Bitmap)[Rect] to a new
drawTextureAdjuster function.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp
File src/gpu/GrBlurUtils.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:171: pathPtr = tmpPath.get();
On 2015/11/05 20:13:23, robertphillips wrote:
> do we need to set volatility here ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:178: pathPtr = strokedPath;
On 2015/11/05 20:13:23, robertphillips wrote:
> here ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:185: pathIsMutable = true;
On 2015/11/05 20:13:23, robertphillips wrote:
> here ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:200: if
(maskFilter->canFilterMaskGPU(SkRRect::MakeRect(devPathPtr->getBounds()),
On 2015/11/05 20:13:23, robertphillips wrote:
> tab these guys left?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:211: if
(maskFilter->directFilterMaskGPU(context->textureProvider(),
On 2015/11/05 20:13:24, robertphillips wrote:
> tab these guys left too ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:232: if (maskFilter->filterMaskGPU(mask, viewMatrix,
maskRect,
On 2015/11/05 20:13:23, robertphillips wrote:
> on prior line ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:250: maskFilter, clipBounds, paint, style);
On 2015/11/05 20:13:24, robertphillips wrote:
> rm this "return;" and the \n after it ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:326: } else {
On 2015/11/05 20:13:23, robertphillips wrote:
> What happens if tmpPath was already activated by the preceding "prePathMatrix"
> code path?

Done, good catch

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrBlurUtils.cpp...
src/gpu/GrBlurUtils.cpp:329: pathPtr = tmpPath.get();
On 2015/11/05 20:13:23, robertphillips wrote:
> volatile ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:345: view_matrix_ok_for_aa_fill_rect(viewMatrix)) {
On 2015/11/05 20:13:24, robertphillips wrote:
> SkAutoTUnref<GrDrawBatch> batch( ...
> ?

I relent

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:354: paint.getColor(),
On 2015/11/05 20:10:37, bsalomon wrote:
> On 2015/11/05 19:53:12, joshualitt wrote:
> > extra spaces
> 
> will fix

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrImageIDTextur...
File src/gpu/GrImageIDTextureAdjuster.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrImageIDTextur...
src/gpu/GrImageIDTextureAdjuster.cpp:28: GrMakeKeyFromImageID(&baseKey,
fBmp->getGenerationID(),
On 2015/11/05 20:13:24, robertphillips wrote:
> align ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
File src/gpu/GrTextureParamsAdjuster.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:184: static DomainMode
determine_domain_mode(
On 2015/11/05 20:13:24, robertphillips wrote:
> tab these over one to avoid lining up with the code ...

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:191: SkRect* domainRect) {
On 2015/11/05 20:13:24, robertphillips wrote:
> ... or maybe a '\n' here ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:235: 
On 2015/11/05 20:13:24, robertphillips wrote:
> // We will inset the domain half a pixel b.c. ...
> 
> ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:236: static const SkScalar kDomainInset =
0.5f;
On 2015/11/05 20:13:24, robertphillips wrote:
> can -> to ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:247: // constraint rect, and we're allowed
to filter across the constraint rect boundary. So
On 2015/11/05 20:13:24, robertphillips wrote:
> from -> to ?

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:284: }
On 2015/11/05 20:13:24, robertphillips wrote:
> What happens to the sides that are still out at the 'setLargest' values ?

They'd still be really large (in abs value). This is tested. In practice fTop
and fLeft of the content area are always 0.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
File src/gpu/GrTextureParamsAdjuster.h (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.h:99: *                                        
clipped to the content area already.
On 2015/11/05 20:13:24, robertphillips wrote:
> filterConstraint ?
> Could use a less cryptic explanation ...

It has a

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.h:104: *                                        
width, just the raw coords).
On 2015/11/05 20:13:24, robertphillips wrote:
> // An in/out param that is ...
> // On input it ... on output it ...
> 
> ?

It's not in/out. Made a real comment.

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

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1758: clippedDstRect, *draw.fMatrix, srcToDstMatrix,
fClip, paint, constraint);
On 2015/11/05 20:10:37, bsalomon wrote:
> On 2015/11/05 19:53:12, joshualitt wrote:
> > can this be made a helper? or do we want it to be separate because of the
> whole
> > image / bitmap thing?  Could maybe have a lambda to call to create the
> > imageTextureAdjuster if it succeeds.
> 
> I'm not sure I understand what you're asking/suggesting.

Done.

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/SkGpuDevice_dra...
File src/gpu/SkGpuDevice_drawTexture.cpp (right):

https://codereview.chromium.org/1424313010/diff/60001/src/gpu/SkGpuDevice_dra...
src/gpu/SkGpuDevice_drawTexture.cpp:31: const SkPaint& paint) {
On 2015/11/05 20:49:39, robertphillips wrote:
> '\n' ?

Done.

[robertphillips, 2015-11-06 20:54:52]

lgtm + mainly dox suggestions

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrBlurUtils.cpp
File src/gpu/GrBlurUtils.cpp (right):

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrBlurUtils.cp...
src/gpu/GrBlurUtils.cpp:328: if (!strokeInfo.isDashed() && pathEffect &&
pathEffect->filterPath(tmpPath2.init(),
tab this over ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrBlurUtils.h
File src/gpu/GrBlurUtils.h (right):

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrBlurUtils.h#...
src/gpu/GrBlurUtils.h:18: class SkMatrix;
Move up a line ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
File src/gpu/GrTextureParamsAdjuster.cpp (right):

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.cpp:195: GrTextureAdjuster::FilterConstraint
filterConstraint,
...LimitedTo... (i.e., add "To") ?
Contraint -> Constraint ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.cpp:234: case
GrTextureParams::kBilerp_FilterMode:
Wouldn't this be 0.5f for half filter width ? Isn't this more filter margin ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.cpp:274: }
texW-1 ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.cpp:279: }
texH ?
texH-1 ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.cpp:338: if (kDomain_DomainMode == domainMode) {
// All mipmapped textures should be tight (and not have a domain)
?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
File src/gpu/GrTextureParamsAdjuster.h (right):

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.h:85: 
Can any of these new entry points be const ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.h:99: *                                        
clipped to the content area already.
iang -> aint ?
is -> needs to be ?

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

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1507: const SkPaint& paint, SkCanvas::SrcRectConstraint
constraint) {
What is 'bm' here for ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1510: CHECK_SHOULD_DRAW(origDraw);
v.low priority: Should we make the other sites use 'alphaOnly' too (instead of
'alphaBitmap') ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1683: GrImageTextureAdjuster adjuster(as_IB(image));
xtra ' ' ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1714: }
mv 'bm' down to here ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
File src/gpu/SkGpuDevice_drawTexture.cpp (right):

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
src/gpu/SkGpuDevice_drawTexture.cpp:25: 
// This could use some documentation

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
src/gpu/SkGpuDevice_drawTexture.cpp:51: 
Can 'adjuster' be const ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
src/gpu/SkGpuDevice_drawTexture.cpp:66:
adjuster->getContentArea(&contentIBounds);
Can 'contentBounds' be const ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
src/gpu/SkGpuDevice_drawTexture.cpp:69: if (srcRect) {
Do we need 'tempDstRect'? Can we just point at 'contentBounds' ?

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
src/gpu/SkGpuDevice_drawTexture.cpp:154: // same happens for any mask filter
that extends the bounds rendered in the dst.
// TODO: not all mask filters expand ?

[bsalomon, 2015-11-09 19:35:23]

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrBlurUtils.cpp
File src/gpu/GrBlurUtils.cpp (right):

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrBlurUtils.cp...
src/gpu/GrBlurUtils.cpp:328: if (!strokeInfo.isDashed() && pathEffect &&
pathEffect->filterPath(tmpPath2.init(),
On 2015/11/06 20:54:51, robertphillips wrote:
> tab this over ?

Done.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrBlurUtils.h
File src/gpu/GrBlurUtils.h (right):

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrBlurUtils.h#...
src/gpu/GrBlurUtils.h:18: class SkMatrix;
On 2015/11/06 20:54:51, robertphillips wrote:
> Move up a line ?

Done.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
File src/gpu/GrTextureParamsAdjuster.cpp (right):

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.cpp:195: GrTextureAdjuster::FilterConstraint
filterConstraint,
On 2015/11/06 20:54:51, robertphillips wrote:
> ...LimitedTo... (i.e., add "To") ?
> Contraint -> Constraint ?

Done.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.cpp:234: case
GrTextureParams::kBilerp_FilterMode:
On 2015/11/06 20:54:51, robertphillips wrote:
> Wouldn't this be 0.5f for half filter width ? Isn't this more filter margin ?

It's supposed to really be half-width, fixed the values.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.cpp:274: }
On 2015/11/06 20:54:51, robertphillips wrote:
> texW-1 ?

no, if fRight is equal to texW we don't need a domain, otherwise we may.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.cpp:279: }
On 2015/11/06 20:54:51, robertphillips wrote:
> texH ?
> texH-1 ?

texH

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.cpp:338: if (kDomain_DomainMode == domainMode) {
On 2015/11/06 20:54:51, robertphillips wrote:
> // All mipmapped textures should be tight (and not have a domain)
> ?

Done.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
File src/gpu/GrTextureParamsAdjuster.h (right):

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.h:85: 
On 2015/11/06 20:54:51, robertphillips wrote:
> Can any of these new entry points be const ?

Done.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/GrTextureParam...
src/gpu/GrTextureParamsAdjuster.h:99: *                                        
clipped to the content area already.
On 2015/11/06 20:54:51, robertphillips wrote:
> iang -> aint ?
> is -> needs to be ?

Done.

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

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1507: const SkPaint& paint, SkCanvas::SrcRectConstraint
constraint) {
On 2015/11/06 20:54:51, robertphillips wrote:
> What is 'bm' here for ?

Done.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1510: CHECK_SHOULD_DRAW(origDraw);
On 2015/11/06 20:54:51, robertphillips wrote:
> v.low priority: Should we make the other sites use 'alphaOnly' too (instead of
> 'alphaBitmap') ?

Done.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1683: GrImageTextureAdjuster adjuster(as_IB(image));
On 2015/11/06 20:54:51, robertphillips wrote:
> xtra ' ' ?

Done.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1714: }
On 2015/11/06 20:54:51, robertphillips wrote:
> mv 'bm' down to here ?

Done.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
File src/gpu/SkGpuDevice_drawTexture.cpp (right):

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
src/gpu/SkGpuDevice_drawTexture.cpp:25: 
On 2015/11/06 20:54:51, robertphillips wrote:
> // This could use some documentation

Done.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
src/gpu/SkGpuDevice_drawTexture.cpp:51: 
On 2015/11/06 20:54:51, robertphillips wrote:
> Can 'adjuster' be const ?
I don't think it should be. It means a subclass could never be stateful (e.g.
remember some of it's decisions from the first time createFragmentProcessor is
called).

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
src/gpu/SkGpuDevice_drawTexture.cpp:66:
adjuster->getContentArea(&contentIBounds);
On 2015/11/06 20:54:51, robertphillips wrote:
> Can 'contentBounds' be const ?

sure

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
src/gpu/SkGpuDevice_drawTexture.cpp:69: if (srcRect) {
On 2015/11/06 20:54:52, robertphillips wrote:
> Do we need 'tempDstRect'? Can we just point at 'contentBounds' ?

Done.

https://codereview.chromium.org/1424313010/diff/180001/src/gpu/SkGpuDevice_dr...
src/gpu/SkGpuDevice_drawTexture.cpp:154: // same happens for any mask filter
that extends the bounds rendered in the dst.
On 2015/11/06 20:54:52, robertphillips wrote:
> // TODO: not all mask filters expand ?

Done.

[bsalomon, 2015-11-09 19:35:28]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-11-09 19:35:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1424313010/#ps220001
(title: "address more comments")

[commit-bot: I haz the power, 2015-11-09 19:35:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424313010/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424313010/220001

[commit-bot: I haz the power, 2015-11-09 19:37:40]

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

[commit-bot: I haz the power, 2015-11-09 19:37:41]

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

[bsalomon, 2015-11-09 19:43:43]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-11-09 19:43:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1424313010/#ps240001
(title: "update")

[commit-bot: I haz the power, 2015-11-09 19:43:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424313010/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424313010/240001

[commit-bot: I haz the power, 2015-11-09 19:56:00]

Committed patchset #13 (id:240001) as
https://skia.googlesource.com/skia/+/c55271f2551533b37043aa2e37f754832a43073c