Move draw on upload decision in GrGpu

Move draw on upload decision in GrGpu

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/0341b4427e5f037e3b501ed6e57dfdb7b40f150e

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

[bsalomon, 2015-07-27 19:50:36]

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

[robertphillips, 2015-07-28 13:15:23]

lgtm (as far as I can follow it) + nits & questions

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:340: 
premul -> performUPMToPMConversion ?

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:343: if (!GrPixelConfigIs8888(srcConfig)) {
// Our UPMToPM conversion only works on 8888. ?

Wouldn't 565 & 4444 also work ?

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

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:242: if (!this->onGetReadPixelsInfo(srcSurface, width, height,
rowBytes, readConfig, drawPreference,
tab this over to the '(' ?

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:272: if (!this->onGetWritePixelsInfo(dstSurface, width,
height, rowBytes, srcConfig, drawPreference,
match with '(' ?

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

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrGpu.h#newcode192
src/gpu/GrGpu.h:192: GrSurfaceDesc   fTempSurfaceDesc;
If set, ?

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrTest.cpp
File src/gpu/GrTest.cpp (right):

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrTest.cpp#newc...
src/gpu/GrTest.cpp:154: bool onGetWritePixelsInfo(GrSurface* dstSurface, int
width, int height, size_t rowBytes,
line up  ?

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

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:545: }
Do we not need a software fallback for this ?

https://codereview.chromium.org/1257073003/diff/40001/tests/WritePixelsTest.cpp
File tests/WritePixelsTest.cpp (right):

https://codereview.chromium.org/1257073003/diff/40001/tests/WritePixelsTest.c...
tests/WritePixelsTest.cpp:29: 
get_canvas_color ?

https://codereview.chromium.org/1257073003/diff/40001/tests/WritePixelsTest.c...
tests/WritePixelsTest.cpp:166: 
check_pixel ?

[bsalomon, 2015-07-28 14:57:59]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-07-28 14:57:59]

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/1257073003/#ps50001
(title: "Address comments")

[bsalomon, 2015-07-28 14:58:05]

The CQ bit was unchecked by bsalomon@google.com

[bsalomon, 2015-07-28 14:58:10]

The CQ bit was checked by bsalomon@google.com to run a CQ dry run

[bsalomon, 2015-07-28 14:58:18]

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:340: 
On 2015/07/28 13:15:23, robertphillips wrote:
> premul -> performUPMToPMConversion ?

Done, but somewhat shorter rename

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:343: if (!GrPixelConfigIs8888(srcConfig)) {
On 2015/07/28 13:15:22, robertphillips wrote:
> // Our UPMToPM conversion only works on 8888. ?
> 
> Wouldn't 565 & 4444 also work ?

565 has no alpha :)

It would work with 4444 but it just isn't tested whereas 8888 is well tested. We
could add it in a future change.

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

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:242: if (!this->onGetReadPixelsInfo(srcSurface, width, height,
rowBytes, readConfig, drawPreference,
On 2015/07/28 13:15:23, robertphillips wrote:
> tab this over to the '(' ?

Done.

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:272: if (!this->onGetWritePixelsInfo(dstSurface, width,
height, rowBytes, srcConfig, drawPreference,
On 2015/07/28 13:15:23, robertphillips wrote:
> match with '(' ?

Done.

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

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrGpu.h#newcode192
src/gpu/GrGpu.h:192: GrSurfaceDesc   fTempSurfaceDesc;
On 2015/07/28 13:15:23, robertphillips wrote:
> If set, ?

Done.

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrTest.cpp
File src/gpu/GrTest.cpp (right):

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/GrTest.cpp#newc...
src/gpu/GrTest.cpp:154: bool onGetWritePixelsInfo(GrSurface* dstSurface, int
width, int height, size_t rowBytes,
On 2015/07/28 13:15:23, robertphillips wrote:
> line up  ?

Done.

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

https://codereview.chromium.org/1257073003/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:545: }
On 2015/07/28 13:15:23, robertphillips wrote:
> Do we not need a software fallback for this ?

If none of the if, else if, else if conditions are met then it means that we
expect uploading bgra pixels to rgba or vice versa will just work.

I think are additional things to check in this function in general, but for now
I just wanted to come as close to preserving existing behavior as I could
(though, I'm sure there are some changes).

https://codereview.chromium.org/1257073003/diff/40001/tests/WritePixelsTest.cpp
File tests/WritePixelsTest.cpp (right):

https://codereview.chromium.org/1257073003/diff/40001/tests/WritePixelsTest.c...
tests/WritePixelsTest.cpp:29: 
On 2015/07/28 13:15:23, robertphillips wrote:
> get_canvas_color ?

Done.

https://codereview.chromium.org/1257073003/diff/40001/tests/WritePixelsTest.c...
tests/WritePixelsTest.cpp:166: 
On 2015/07/28 13:15:23, robertphillips wrote:
> check_pixel ?

Done.

[commit-bot: I haz the power, 2015-07-28 14:58:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257073003/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257073003/50001

[commit-bot: I haz the power, 2015-07-28 15:54:05]

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

[commit-bot: I haz the power, 2015-07-28 15:54:06]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2015-07-28 16:13:03]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2015-07-28 16:13:12]

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

[commit-bot: I haz the power, 2015-07-28 16:13:51]

Committed patchset #4 (id:50001) as
https://skia.googlesource.com/skia/+/0341b4427e5f037e3b501ed6e57dfdb7b40f150e

[bsalomon, 2015-07-28 19:23:06]

A revert of this CL (patchset #4 id:50001) has been created in
https://codereview.chromium.org/1260293004/ by bsalomon@google.com.

The reason for reverting is: breaking write pixels test on bots.

[bsalomon, 2015-07-28 20:18:30]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-07-28 20:18:31]

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/1257073003/#ps70001
(title: "fixed for sw conversion case")

[commit-bot: I haz the power, 2015-07-28 20:18:43]

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

[commit-bot: I haz the power, 2015-07-28 20:26:19]

Committed patchset #5 (id:70001) as
https://skia.googlesource.com/skia/+/f0674516cb18f6b13e34f404ff5793d9b9ebb56c