Revise DrawAtlasBatch to get rid of one copy when generating the batch.

Revise DrawAtlasBatch to get rid of one copy when generating the batch.

This doesn't appear to have much affect on perf in SampleApp, but
it should avoid some copies and replaces a couple of STArrays
with one that just does a memcpy in the copy constructor.

Committed: https://skia.googlesource.com/skia/+/5a4d23508c3ca2c6cbca4e725ce65c0227b3b9ac

[jvanverth1, 2015-08-11 14:49:49]

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

[jvanverth1, 2015-08-11 14:49:50]

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
File src/gpu/batches/GrDrawAtlasBatch.cpp (right):

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:156: return false;
Not sure if this should be here or not.

[robertphillips, 2015-08-11 15:01:07]

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
File src/gpu/batches/GrDrawAtlasBatch.cpp (right):

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:156: return false;
I think we should just let it batch or add a big comment saying "We don't think
this will ever batch in the real world so we just ignore the possibility here".

[jvanverth1, 2015-08-11 15:27:43]

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
File src/gpu/batches/GrDrawAtlasBatch.cpp (right):

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:156: return false;
On 2015/08/11 15:01:06, robertphillips wrote:
> I think we should just let it batch or add a big comment saying "We don't
think
> this will ever batch in the real world so we just ignore the possibility
here".
> 

Re-enabled it.

[robertphillips, 2015-08-11 15:32:06]

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
File src/gpu/batches/GrDrawAtlasBatch.cpp (right):

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:86: // figure out stride and offsets
// Order within the vertex is: position [color] texture ?

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:91: size_t texOffset = sizeof(SkPoint);
stride -> vertexStride ?

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:109: SkRect bounds;
Hmmm ... isn't this going to force the bounds to always include the origin?

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:117:
*(reinterpret_cast<SkPoint*>(currVertex)) = quad[0];
Hmmm ... could we have a second separate loop if we have colors? It is kind of
odd to keep copying over the same color just to overwrite it.

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:188: 
Do we need to pass in the width & height of the texture so we can randomize the
anchor points too?

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:211: random->nextRangeScalar(kMinCoord,
kMaxCoord));
Do we need a sort call here ?

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:214: 
Seems like 'count' should be an int.

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:230: 
(spriteCount) on xforms, texRects & colors ?

https://codereview.chromium.org/1279343004/diff/20001/src/gpu/batches/GrDrawA...
File src/gpu/batches/GrDrawAtlasBatch.cpp (right):

https://codereview.chromium.org/1279343004/diff/20001/src/gpu/batches/GrDrawA...
src/gpu/batches/GrDrawAtlasBatch.cpp:77: int spriteCount, const SkRSXform*
xforms, const SkRect* rects,
Don't we assume these are premul?

[jvanverth1, 2015-08-11 20:22:55]

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
File src/gpu/batches/GrDrawAtlasBatch.cpp (right):

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:86: // figure out stride and offsets
On 2015/08/11 15:32:05, robertphillips wrote:
> // Order within the vertex is: position [color] texture ?

Done.

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:91: size_t texOffset = sizeof(SkPoint);
On 2015/08/11 15:32:05, robertphillips wrote:
> stride -> vertexStride ?

Done.

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:109: SkRect bounds;
On 2015/08/11 15:32:05, robertphillips wrote:
> Hmmm ... isn't this going to force the bounds to always include the origin?

Done.

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:117:
*(reinterpret_cast<SkPoint*>(currVertex)) = quad[0];
On 2015/08/11 15:32:05, robertphillips wrote:
> Hmmm ... could we have a second separate loop if we have colors? It is kind of
> odd to keep copying over the same color just to overwrite it.

Talked to mtlklein and revised to make it cleaner. No perf difference that I can
see.

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:188: 
On 2015/08/11 15:32:05, robertphillips wrote:
> Do we need to pass in the width & height of the texture so we can randomize
the
> anchor points too?

Added random anchor points. It doesn't affect the texture coordinates, so
texture size shouldn't matter.

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:211: random->nextRangeScalar(kMinCoord,
kMaxCoord));
On 2015/08/11 15:32:05, robertphillips wrote:
> Do we need a sort call here ?

Done.

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:214: 
On 2015/08/11 15:32:05, robertphillips wrote:
> Seems like 'count' should be an int.

Done.

https://codereview.chromium.org/1279343004/diff/1/src/gpu/batches/GrDrawAtlas...
src/gpu/batches/GrDrawAtlasBatch.cpp:230: 
On 2015/08/11 15:32:05, robertphillips wrote:
> (spriteCount) on xforms, texRects & colors ?

Done.

https://codereview.chromium.org/1279343004/diff/20001/src/gpu/batches/GrDrawA...
File src/gpu/batches/GrDrawAtlasBatch.cpp (right):

https://codereview.chromium.org/1279343004/diff/20001/src/gpu/batches/GrDrawA...
src/gpu/batches/GrDrawAtlasBatch.cpp:77: int spriteCount, const SkRSXform*
xforms, const SkRect* rects,
On 2015/08/11 15:32:06, robertphillips wrote:
> Don't we assume these are premul?

Added the GrColor conversion back in.

[robertphillips, 2015-08-12 12:34:11]

lgtm + some nits

https://codereview.chromium.org/1279343004/diff/40001/src/gpu/batches/GrDrawA...
File src/gpu/batches/GrDrawAtlasBatch.cpp (right):

https://codereview.chromium.org/1279343004/diff/40001/src/gpu/batches/GrDrawA...
src/gpu/batches/GrDrawAtlasBatch.cpp:90: size_t vertexStride =
2*sizeof(SkPoint);
SkToBool(colors) ?

https://codereview.chromium.org/1279343004/diff/40001/src/gpu/batches/GrDrawA...
src/gpu/batches/GrDrawAtlasBatch.cpp:108: SkPoint quad[4];
const SkRect& currRect = ... ?

[jvanverth1, 2015-08-12 14:08:42]

https://codereview.chromium.org/1279343004/diff/40001/src/gpu/batches/GrDrawA...
File src/gpu/batches/GrDrawAtlasBatch.cpp (right):

https://codereview.chromium.org/1279343004/diff/40001/src/gpu/batches/GrDrawA...
src/gpu/batches/GrDrawAtlasBatch.cpp:90: size_t vertexStride =
2*sizeof(SkPoint);
On 2015/08/12 12:34:11, robertphillips wrote:
> SkToBool(colors) ?

Done.

https://codereview.chromium.org/1279343004/diff/40001/src/gpu/batches/GrDrawA...
src/gpu/batches/GrDrawAtlasBatch.cpp:108: SkPoint quad[4];
On 2015/08/12 12:34:11, robertphillips wrote:
> const SkRect& currRect = ... ?

Done.

[jvanverth1, 2015-08-12 14:08:45]

The CQ bit was checked by jvanverth@google.com

[jvanverth1, 2015-08-12 14:08:46]

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/1279343004/#ps60001
(title: "Address nits")

[commit-bot: I haz the power, 2015-08-12 14:08:56]

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

[commit-bot: I haz the power, 2015-08-12 14:12:06]

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

[commit-bot: I haz the power, 2015-08-12 14:12:07]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[jvanverth1, 2015-08-12 15:07:14]

The CQ bit was checked by jvanverth@google.com

[jvanverth1, 2015-08-12 15:07:15]

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/1279343004/#ps100001
(title: "Picky picky Windows")

[commit-bot: I haz the power, 2015-08-12 15:07:27]

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

[commit-bot: I haz the power, 2015-08-12 15:15:35]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/5a4d23508c3ca2c6cbca4e725ce65c0227b3b9ac