Move some parts of onReadPixels up to GrGpu readPixels.

Move some parts of onReadPixels up to GrGpu readPixels.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/6d901da0ec4b10010eca05114ab8f9564bd04af3

[egdaniel, 2015-07-30 18:00:48]

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

[egdaniel, 2015-07-30 18:00:49]

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

https://codereview.chromium.org/1270583003/diff/20001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:297: // We cannot read pixels into a compressed buffer
It can be argued this one is dependent on the backend, but I don't see a reason
we can't just say we don't support reading into a compressed buffer in general.

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

https://codereview.chromium.org/1270583003/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1812: // resolve the render target if necessary
I also wouldn't be against a virtual on Gpu that is like
resolveRT(GrRenderTarget). Then I would just move this up as well (obviously
only resolving if the surface is a RT)

[bsalomon, 2015-07-30 18:27:23]

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

https://codereview.chromium.org/1270583003/diff/20001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:297: // We cannot read pixels into a compressed buffer
On 2015/07/30 18:00:49, egdaniel wrote:
> It can be argued this one is dependent on the backend, but I don't see a
reason
> we can't just say we don't support reading into a compressed buffer in
general.

If we're going to do this then we should also move the corresponding code in
onGetReadPixelsInfo to getReadPixelsInfo.

https://codereview.chromium.org/1270583003/diff/20001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:311: if (0 == rowBytes) {
AdjustReadPixelParams already does this (sets rowBytes to tight if it's zero).
So this can be removed.

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

https://codereview.chromium.org/1270583003/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1797: size_t tightRowBytes) {
Personally I find the tightRowBytes param is more clutter than is justified by
the cost of recomputing in the subclass (and I think the computation in
GrGpu::readPixels isn't really necessary in that function).

https://codereview.chromium.org/1270583003/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1812: // resolve the render target if necessary
On 2015/07/30 18:00:49, egdaniel wrote:
> I also wouldn't be against a virtual on Gpu that is like
> resolveRT(GrRenderTarget). Then I would just move this up as well (obviously
> only resolving if the surface is a RT)

This is doing more than just resolving (that is handled by the call to
onResolveRenderTarget below). It is ensuring that the correct FBO is bound for
reading. If we want to resolve in the base class we can just call
GrGpu::resolveRenderTarget(). It'd preclude any implementation that could
downsample and blit to CPU accessible memory in one step (though I know of no
such thing). However, we will wind up revisiting this in any event when we
reorganize the GrSurface family of objects.

[egdaniel, 2015-07-30 18:47:35]

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

https://codereview.chromium.org/1270583003/diff/20001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:297: // We cannot read pixels into a compressed buffer
On 2015/07/30 18:27:22, bsalomon wrote:
> On 2015/07/30 18:00:49, egdaniel wrote:
> > It can be argued this one is dependent on the backend, but I don't see a
> reason
> > we can't just say we don't support reading into a compressed buffer in
> general.
> 
> If we're going to do this then we should also move the corresponding code in
> onGetReadPixelsInfo to getReadPixelsInfo.

Done.

https://codereview.chromium.org/1270583003/diff/20001/src/gpu/GrGpu.cpp#newco...
src/gpu/GrGpu.cpp:311: if (0 == rowBytes) {
On 2015/07/30 18:27:22, bsalomon wrote:
> AdjustReadPixelParams already does this (sets rowBytes to tight if it's zero).
> So this can be removed.

Done.

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

https://codereview.chromium.org/1270583003/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1797: size_t tightRowBytes) {
On 2015/07/30 18:27:22, bsalomon wrote:
> Personally I find the tightRowBytes param is more clutter than is justified by
> the cost of recomputing in the subclass (and I think the computation in
> GrGpu::readPixels isn't really necessary in that function).

Done.

https://codereview.chromium.org/1270583003/diff/20001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:1812: // resolve the render target if necessary
On 2015/07/30 18:27:22, bsalomon wrote:
> On 2015/07/30 18:00:49, egdaniel wrote:
> > I also wouldn't be against a virtual on Gpu that is like
> > resolveRT(GrRenderTarget). Then I would just move this up as well (obviously
> > only resolving if the surface is a RT)
> 
> This is doing more than just resolving (that is handled by the call to
> onResolveRenderTarget below). It is ensuring that the correct FBO is bound for
> reading. If we want to resolve in the base class we can just call
> GrGpu::resolveRenderTarget(). It'd preclude any implementation that could
> downsample and blit to CPU accessible memory in one step (though I know of no
> such thing). However, we will wind up revisiting this in any event when we
> reorganize the GrSurface family of objects.

Sounds good

[bsalomon, 2015-07-30 18:49:07]

lgtm

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

https://codereview.chromium.org/1270583003/diff/40001/src/gpu/gl/GrGLGpu.h#ne...
src/gpu/gl/GrGLGpu.h:146: size_t tightRowBytes) override;
rm this?

[egdaniel, 2015-07-30 18:52:38]

The CQ bit was checked by egdaniel@google.com

[egdaniel, 2015-07-30 18:52:38]

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/1270583003/#ps60001
(title: "nit")

[commit-bot: I haz the power, 2015-07-30 18:52:47]

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

[commit-bot: I haz the power, 2015-07-30 19:02:19]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/6d901da0ec4b10010eca05114ab8f9564bd04af3