Add transfer buffer support.

Add transfer buffer support.

BUG=skia:4604
Committed: https://skia.googlesource.com/skia/+/fa498fe12239988578465d0dee69f0d5645bb361

Committed: https://skia.googlesource.com/skia/+/73063dc517f424ad5660db0fbc5fe6fcc13f77f7

[jvanverth1, 2015-12-02 16:23:57]

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

[jvanverth1, 2015-12-02 16:23:57]

This is the initial pass. I still have to add support for the Chromium buffers,
and CPU-backed buffers as a fallback, then set up the font cache. But it was
already seeming complicated enough.

[jvanverth1, 2015-12-02 16:33:36]

jvanverth@google.com changed reviewers:
+ egdaniel@google.com

[bsalomon, 2015-12-02 16:38:31]

Do we need CPU fallback? Wondering if we should just have a caps bit,
transferBufferSupport().

Let's definitely make adding support for a chrome variation a separate task (and
confirm with reveman@ that it is worth trying).

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

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/GrGpu.h#newcod...
src/gpu/GrGpu.h:136: * @param toGpu     true if used to transfer from the cpu to
the gpu
Maybe an enum instead of a bool for clarity at the call sites? something like

enum {
kCpuToGpu_TransferType
kGpuToGpu_TransferType
};

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/GrTransferBuff...
File src/gpu/GrTransferBuffer.h (right):

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/GrTransferBuff...
src/gpu/GrTransferBuffer.h:20: * The previous content of the buffer is
invalidated. It is an error
Looks like these comments came from geometrybuffer... wondering if it makes
sense to drop language around no map support since these wouldn't be useful at
all without mapping IIUC.

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/GrTransferBuff...
src/gpu/GrTransferBuffer.h:62: * Updates the buffer data.
Do we want this at all or should we just assume mapability?

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/gl/GrGLBufferI...
File src/gpu/gl/GrGLBufferImpl.h (right):

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/gl/GrGLBufferI...
src/gpu/gl/GrGLBufferImpl.h:22: enum Usage {
Like this change

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

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1421: /* Why DYNAMIC_DRAW here and STREAM_DRAW elsewhere?
*/
Probably no good reason... we should be consistent. I think our usage pattern is
really stream_draw.

Maybe we should have a function in GrBufferImpl.h that decides what the
"dynamic" bool means in GL?

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1645: } else if (GR_GL_ELEMENT_ARRAY_BUFFER == type) {
you don't need equivalent here for pbo?

[jvanverth1, 2015-12-02 18:05:07]

Agreed on CPU support -- I was thinking of wrapping it in one class because
that's what Android does. As far as Chromium, this is for the extension in the
command buffer, which I'd assume we'd want to use if it's there.

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

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/GrGpu.h#newcod...
src/gpu/GrGpu.h:136: * @param toGpu     true if used to transfer from the cpu to
the gpu
On 2015/12/02 16:38:31, bsalomon wrote:
> Maybe an enum instead of a bool for clarity at the call sites? something like
> 
> enum {
> kCpuToGpu_TransferType
> kGpuToGpu_TransferType
> };
> 

Done.

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/GrTransferBuff...
File src/gpu/GrTransferBuffer.h (right):

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/GrTransferBuff...
src/gpu/GrTransferBuffer.h:20: * The previous content of the buffer is
invalidated. It is an error
On 2015/12/02 16:38:31, bsalomon wrote:
> Looks like these comments came from geometrybuffer... wondering if it makes
> sense to drop language around no map support since these wouldn't be useful at
> all without mapping IIUC.

Done.

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/GrTransferBuff...
src/gpu/GrTransferBuffer.h:62: * Updates the buffer data.
On 2015/12/02 16:38:31, bsalomon wrote:
> Do we want this at all or should we just assume mapability?

BufferData is supported by PBOs, as far as I can tell. But I don't think we'd
use it. Removed.

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/gl/GrGLBufferI...
File src/gpu/gl/GrGLBufferImpl.h (right):

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/gl/GrGLBufferI...
src/gpu/gl/GrGLBufferImpl.h:22: enum Usage {
On 2015/12/02 16:38:31, bsalomon wrote:
> Like this change

Acknowledged.

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

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1421: /* Why DYNAMIC_DRAW here and STREAM_DRAW elsewhere?
*/
On 2015/12/02 16:38:31, bsalomon wrote:
> Probably no good reason... we should be consistent. I think our usage pattern
is
> really stream_draw.
> 
> Maybe we should have a function in GrBufferImpl.h that decides what the
> "dynamic" bool means in GL?

It looks like it's always stream_draw -- I've changed it assuming that.

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1645: } else if (GR_GL_ELEMENT_ARRAY_BUFFER == type) {
On 2015/12/02 16:38:31, bsalomon wrote:
> you don't need equivalent here for pbo?

It's used to update the geometry state, which I don't think we need to track for
the PBOs.

[bsalomon, 2015-12-02 18:39:22]

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

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1421: /* Why DYNAMIC_DRAW here and STREAM_DRAW elsewhere?
*/
On 2015/12/02 18:05:07, jvanverth1 wrote:
> On 2015/12/02 16:38:31, bsalomon wrote:
> > Probably no good reason... we should be consistent. I think our usage
pattern
> is
> > really stream_draw.
> > 
> > Maybe we should have a function in GrBufferImpl.h that decides what the
> > "dynamic" bool means in GL?
> 
> It looks like it's always stream_draw -- I've changed it assuming that.

Looks the same as before. I remember the history here. We used to use
DYNAMIC_DRAW, then Chromium added a workaround for Mali/IMG devices where
STREAM_DRAW would trigger client arrays on the service side and we changed from
DYNAMIC_DRAW to STREAM_DRAW. This spot was missed in that conversion.

[jvanverth1, 2015-12-02 18:41:59]

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

https://codereview.chromium.org/1490473003/diff/100001/src/gpu/gl/GrGLGpu.cpp...
src/gpu/gl/GrGLGpu.cpp:1421: /* Why DYNAMIC_DRAW here and STREAM_DRAW elsewhere?
*/
On 2015/12/02 18:39:22, bsalomon wrote:
> On 2015/12/02 18:05:07, jvanverth1 wrote:
> > On 2015/12/02 16:38:31, bsalomon wrote:
> > > Probably no good reason... we should be consistent. I think our usage
> pattern
> > is
> > > really stream_draw.
> > > 
> > > Maybe we should have a function in GrBufferImpl.h that decides what the
> > > "dynamic" bool means in GL?
> > 
> > It looks like it's always stream_draw -- I've changed it assuming that.
> 
> Looks the same as before. I remember the history here. We used to use
> DYNAMIC_DRAW, then Chromium added a workaround for Mali/IMG devices where
> STREAM_DRAW would trigger client arrays on the service side and we changed
from
> DYNAMIC_DRAW to STREAM_DRAW. This spot was missed in that conversion.

I apparently hadn't saved that file when I uploaded. See the next patch.

[bsalomon, 2015-12-02 18:58:51]

lgtm

[jvanverth1, 2015-12-02 20:30:20]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2015-12-02 20:30:36]

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

[commit-bot: I haz the power, 2015-12-02 20:31:33]

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

[commit-bot: I haz the power, 2015-12-02 20:31:34]

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

[jvanverth1, 2015-12-02 20:52:23]

The CQ bit was checked by jvanverth@google.com

[jvanverth1, 2015-12-02 20:52:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1490473003/#ps160001
(title: "Remove unnecessary assert")

[commit-bot: I haz the power, 2015-12-02 20:52:30]

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

[commit-bot: I haz the power, 2015-12-02 21:06:39]

Description was changed from

==========
Add transfer buffer support.

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

to

==========
Add transfer buffer support.

BUG=skia:4604

Committed:
https://skia.googlesource.com/skia/+/fa498fe12239988578465d0dee69f0d5645bb361
==========

[commit-bot: I haz the power, 2015-12-02 21:06:40]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/fa498fe12239988578465d0dee69f0d5645bb361

[egdaniel, 2015-12-03 14:32:06]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1496843003/ by egdaniel@google.com.

The reason for reverting is: speculative revert for deps roll failures.

[jvanverth1, 2015-12-03 14:53:16]

Description was changed from

==========
Add transfer buffer support.

BUG=skia:4604

Committed:
https://skia.googlesource.com/skia/+/fa498fe12239988578465d0dee69f0d5645bb361
==========

to

==========
Add transfer buffer support.

BUG=skia:4604

Committed:
https://skia.googlesource.com/skia/+/fa498fe12239988578465d0dee69f0d5645bb361
==========

[jvanverth1, 2015-12-03 15:11:28]

On 2015/12/03 14:32:06, egdaniel wrote:
> A revert of this CL (patchset #9 id:160001) has been created in
> https://codereview.chromium.org/1496843003/ by mailto:egdaniel@google.com.
> 
> The reason for reverting is: speculative revert for deps roll failures.

Fix is in. PTAL.

[egdaniel, 2015-12-03 15:19:38]

lgtm

[bsalomon, 2015-12-03 16:03:01]

On 2015/12/03 15:11:28, jvanverth1 wrote:
> On 2015/12/03 14:32:06, egdaniel wrote:
> > A revert of this CL (patchset #9 id:160001) has been created in
> > https://codereview.chromium.org/1496843003/ by mailto:egdaniel@google.com.
> > 
> > The reason for reverting is: speculative revert for deps roll failures.
> 
> Fix is in. PTAL.

lgtm

[jvanverth1, 2015-12-03 16:05:53]

The CQ bit was checked by jvanverth@google.com

[commit-bot: I haz the power, 2015-12-03 16:06:04]

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

[commit-bot: I haz the power, 2015-12-03 17:15:50]

Description was changed from

==========
Add transfer buffer support.

BUG=skia:4604

Committed:
https://skia.googlesource.com/skia/+/fa498fe12239988578465d0dee69f0d5645bb361
==========

to

==========
Add transfer buffer support.

BUG=skia:4604

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

Committed:
https://skia.googlesource.com/skia/+/73063dc517f424ad5660db0fbc5fe6fcc13f77f7
==========

[commit-bot: I haz the power, 2015-12-03 17:15:50]

Committed patchset #10 (id:180001) as
https://skia.googlesource.com/skia/+/73063dc517f424ad5660db0fbc5fe6fcc13f77f7