More framework support for TransferBuffers

More framework support for TransferBuffers

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1534123003

Committed: https://skia.googlesource.com/skia/+/17aa047066eaaa56637c4c2b93c8c4c374127dbf

[jvanverth1, 2015-12-18 15:10:48]

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

[jvanverth1, 2015-12-18 15:10:48]

This is probably safe to land as no one is using TransferBuffers at the moment.
Tested it with the upcoming patch -- it works but with synchronization issues.

[bsalomon, 2015-12-18 15:24:38]

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

https://codereview.chromium.org/1534123003/diff/1/src/gpu/GrGpu.cpp#newcode343
src/gpu/GrGpu.cpp:343: if (!buffer) {
|| !surface?

https://codereview.chromium.org/1534123003/diff/1/src/gpu/GrGpu.cpp#newcode350
src/gpu/GrGpu.cpp:350: fStats.incTextureUploads();
Worth tracking transfers differently from uploads? incTransfersToTexture()?

https://codereview.chromium.org/1534123003/diff/1/src/gpu/GrResourceProvider.h
File src/gpu/GrResourceProvider.h (right):

https://codereview.chromium.org/1534123003/diff/1/src/gpu/GrResourceProvider....
src/gpu/GrResourceProvider.h:115: enum TransferType {
We could put this in GrTypesPriv.h if you anticipate having a bunch of redefs of
this basic idea.

https://codereview.chromium.org/1534123003/diff/1/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1534123003/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:659: GR_GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM ==
glBuffer->bufferType());
align?

https://codereview.chromium.org/1534123003/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:662: bool result = onWritePixels(surface, left, top,
width, height, config,
this->

https://codereview.chromium.org/1534123003/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:666: GL_CALL(BindBuffer(glBuffer->bufferType(), 0));
should we lazily unbind when we see a call to writePixels?

https://codereview.chromium.org/1534123003/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:687: //*** for now    SkASSERT(data || isNewTexture);
b/c data might be an offset? Maybe we should rename the param dataOrOffset?

Or is there a slightly bigger rewrite we should do that passes the
transferBuffer or nullptr here (and does the bind here)? Not sure whether it's
better or not, just tossing out stuff.

[jvanverth1, 2016-01-04 21:18:40]

Description was changed from

==========
More framework support for TransferBuffers

BUG=skia:
==========

to

==========
More framework support for TransferBuffers

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[jvanverth1, 2016-01-04 21:24:09]

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

https://codereview.chromium.org/1534123003/diff/1/src/gpu/GrGpu.cpp#newcode343
src/gpu/GrGpu.cpp:343: if (!buffer) {
On 2015/12/18 15:24:38, bsalomon wrote:
> || !surface?

Done.

https://codereview.chromium.org/1534123003/diff/1/src/gpu/GrGpu.cpp#newcode350
src/gpu/GrGpu.cpp:350: fStats.incTextureUploads();
On 2015/12/18 15:24:38, bsalomon wrote:
> Worth tracking transfers differently from uploads? incTransfersToTexture()?

Done.

https://codereview.chromium.org/1534123003/diff/1/src/gpu/GrResourceProvider.h
File src/gpu/GrResourceProvider.h (right):

https://codereview.chromium.org/1534123003/diff/1/src/gpu/GrResourceProvider....
src/gpu/GrResourceProvider.h:115: enum TransferType {
On 2015/12/18 15:24:38, bsalomon wrote:
> We could put this in GrTypesPriv.h if you anticipate having a bunch of redefs
of
> this basic idea.

Done.

https://codereview.chromium.org/1534123003/diff/1/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1534123003/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:659: GR_GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM ==
glBuffer->bufferType());
On 2015/12/18 15:24:38, bsalomon wrote:
> align?

Done.

https://codereview.chromium.org/1534123003/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:662: bool result = onWritePixels(surface, left, top,
width, height, config,
On 2015/12/18 15:24:38, bsalomon wrote:
> this->

Done.

https://codereview.chromium.org/1534123003/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:666: GL_CALL(BindBuffer(glBuffer->bufferType(), 0));
On 2015/12/18 15:24:38, bsalomon wrote:
> should we lazily unbind when we see a call to writePixels?

I'm not sure we need an unbind -- removed.

https://codereview.chromium.org/1534123003/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:687: //*** for now    SkASSERT(data || isNewTexture);
On 2015/12/18 15:24:38, bsalomon wrote:
> b/c data might be an offset? Maybe we should rename the param dataOrOffset?
> 
> Or is there a slightly bigger rewrite we should do that passes the
> transferBuffer or nullptr here (and does the bind here)? Not sure whether it's
> better or not, just tossing out stuff.

Rewrote this to take an upload type -- some of the things we do for regular
uploads aren't possible for transfers (e.g. handling bottom-origin textures by
swapping rows).

[bsalomon, 2016-01-05 14:01:18]

looks good, minor stuff

https://codereview.chromium.org/1534123003/diff/20001/include/gpu/GrTypesPriv.h
File include/gpu/GrTypesPriv.h (right):

https://codereview.chromium.org/1534123003/diff/20001/include/gpu/GrTypesPriv...
include/gpu/GrTypesPriv.h:271: * Indicates the transfer direction for a transfer
buffer
nit the single *s should be one space out to align with the first * in /**

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:353: if (!buffer) {
wondering whether this should be an assert since GrTB is a internal type

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/GrGpu.h#newcode265
src/gpu/GrGpu.h:265: *
spaces

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:642: if (!glTex) {
If there is a simple way to share this preamble of checks with onUploadPixels
that'd be nice.

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/gl/GrGLGpu.h#ne...
src/gpu/gl/GrGLGpu.h:293: kDefault_UploadType,
Maybe brief comments here about what these are (particularly default).

[jvanverth1, 2016-01-05 17:21:28]

https://codereview.chromium.org/1534123003/diff/20001/include/gpu/GrTypesPriv.h
File include/gpu/GrTypesPriv.h (right):

https://codereview.chromium.org/1534123003/diff/20001/include/gpu/GrTypesPriv...
include/gpu/GrTypesPriv.h:271: * Indicates the transfer direction for a transfer
buffer
On 2016/01/05 14:01:17, bsalomon wrote:
> nit the single *s should be one space out to align with the first * in /**

Done.

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:353: if (!buffer) {
On 2016/01/05 14:01:17, bsalomon wrote:
> wondering whether this should be an assert since GrTB is a internal type

Done.

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/GrGpu.h#newcode265
src/gpu/GrGpu.h:265: *
On 2016/01/05 14:01:17, bsalomon wrote:
> spaces

Done.

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:642: if (!glTex) {
On 2016/01/05 14:01:17, bsalomon wrote:
> If there is a simple way to share this preamble of checks with onUploadPixels
> that'd be nice.

Done.

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/1534123003/diff/20001/src/gpu/gl/GrGLGpu.h#ne...
src/gpu/gl/GrGLGpu.h:293: kDefault_UploadType,
On 2016/01/05 14:01:17, bsalomon wrote:
> Maybe brief comments here about what these are (particularly default).

Done.

[bsalomon, 2016-01-05 18:14:11]

lgtm

[jvanverth1, 2016-01-05 18:23:33]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2016-01-05 18:23:45]

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

[commit-bot: I haz the power, 2016-01-05 18:41:29]

Description was changed from

==========
More framework support for TransferBuffers

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
More framework support for TransferBuffers

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/17aa047066eaaa56637c4c2b93c8c4c374127dbf
==========

[commit-bot: I haz the power, 2016-01-05 18:41:30]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/17aa047066eaaa56637c4c2b93c8c4c374127dbf