Make A8 readbacks work from non-A8 surfaces

Make A8 readbacks work from non-A8 surfaces

Expand testing of A8 readbacks.

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

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

[bsalomon, 2016-01-21 22:27:56]

Description was changed from

==========
start

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

to

==========
start

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

[bsalomon, 2016-01-23 18:28:57]

Description was changed from

==========
start

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

to

==========
Make A8 readbacks work from non-A8 surfaces

Expand testing of A8 readbacks.

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

[bsalomon, 2016-01-23 18:28:57]

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

[bsalomon, 2016-01-23 19:01:40]

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

[commit-bot: I haz the power, 2016-01-23 19:01:43]

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

[commit-bot: I haz the power, 2016-01-23 19:13:13]

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

[commit-bot: I haz the power, 2016-01-23 19:13:13]

Dry run: This issue passed the CQ dry run.

[egdaniel, 2016-01-25 21:28:24]

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

https://codereview.chromium.org/1613903005/diff/40001/src/gpu/gl/GrGLCaps.cpp...
src/gpu/gl/GrGLCaps.cpp:649: if (!this->isConfigRenderable(rtConfig, false)) {
what is the case where this is not assertable any more? Maybe add a comment why
that is

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

https://codereview.chromium.org/1613903005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2042: tempDrawInfo->fTempSurfaceDesc.fConfig =
readConfig;
so not 100% sure how the srgb coversion works, but do we always want to set both
configs to the read config regardless of which one was srgb?

https://codereview.chromium.org/1613903005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2143: this->readPixelsSupported(surface, tempConfig)) {
just for symmetry can we use tgt instead of surface here?

[bsalomon, 2016-01-25 21:45:17]

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

https://codereview.chromium.org/1613903005/diff/40001/src/gpu/gl/GrGLCaps.cpp...
src/gpu/gl/GrGLCaps.cpp:649: if (!this->isConfigRenderable(rtConfig, false)) {
On 2016/01/25 21:28:24, egdaniel wrote:
> what is the case where this is not assertable any more? Maybe add a comment
why
> that is

The old code was more brittle. It assumed that the caller has already checked
this. Now the implementation of this checks it so that the caller doesn't need
to call two caps functions.

Added comment.

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

https://codereview.chromium.org/1613903005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2042: tempDrawInfo->fTempSurfaceDesc.fConfig =
readConfig;
On 2016/01/25 21:28:24, egdaniel wrote:
> so not 100% sure how the srgb coversion works, but do we always want to set
both
> configs to the read config regardless of which one was srgb?

Yes, the conversion to or from sRGB happens during the draw. I added an extra
check here so that we will fail early if the readback from the readConfig would
fail. This could be made more complicated by added swizzling + srgb conversions,
but deferring on that for now.

https://codereview.chromium.org/1613903005/diff/40001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2143: this->readPixelsSupported(surface, tempConfig)) {
On 2016/01/25 21:28:24, egdaniel wrote:
> just for symmetry can we use tgt instead of surface here? 

Done, also renamed tgt to renderTarget.

[egdaniel, 2016-01-25 22:02:40]

lgtm with nits

https://codereview.chromium.org/1613903005/diff/60001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1613903005/diff/60001/src/gpu/gl/GrGLCaps.cpp...
src/gpu/gl/GrGLCaps.cpp:646: // If it's not possible to even have a render
target of rt config then read pixels is
make rt config rtConfig to match variable? Also of->with?

Well either "with rtConfig" or "with the rt config"

https://codereview.chromium.org/1613903005/diff/60001/tests/ReadWriteAlphaTes...
File tests/ReadWriteAlphaTest.cpp (right):

https://codereview.chromium.org/1613903005/diff/60001/tests/ReadWriteAlphaTes...
tests/ReadWriteAlphaTest.cpp:159: "Failed alpha readback from cfg %d. Expected:
0x%02x, Got: 0x%02x at "
100 chars

[bsalomon, 2016-01-25 22:21:03]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-01-25 22:21:03]

The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/1613903005/#ps80001
(title: "Address comments")

[commit-bot: I haz the power, 2016-01-25 22:21:07]

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

[bsalomon, 2016-01-25 22:21:10]

https://codereview.chromium.org/1613903005/diff/60001/src/gpu/gl/GrGLCaps.cpp
File src/gpu/gl/GrGLCaps.cpp (right):

https://codereview.chromium.org/1613903005/diff/60001/src/gpu/gl/GrGLCaps.cpp...
src/gpu/gl/GrGLCaps.cpp:646: // If it's not possible to even have a render
target of rt config then read pixels is
On 2016/01/25 22:02:39, egdaniel wrote:
> make rt config rtConfig to match variable? Also of->with?
> 
> Well either "with rtConfig" or "with the rt config"

Done.

https://codereview.chromium.org/1613903005/diff/60001/tests/ReadWriteAlphaTes...
File tests/ReadWriteAlphaTest.cpp (right):

https://codereview.chromium.org/1613903005/diff/60001/tests/ReadWriteAlphaTes...
tests/ReadWriteAlphaTest.cpp:159: "Failed alpha readback from cfg %d. Expected:
0x%02x, Got: 0x%02x at "
On 2016/01/25 22:02:39, egdaniel wrote:
> 100 chars

Done.

[commit-bot: I haz the power, 2016-01-25 22:33:29]

Description was changed from

==========
Make A8 readbacks work from non-A8 surfaces

Expand testing of A8 readbacks.

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

to

==========
Make A8 readbacks work from non-A8 surfaces

Expand testing of A8 readbacks.

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/+/e9573317d35d08254412eb407211f3607f8f74fb
==========

[commit-bot: I haz the power, 2016-01-25 22:33:30]

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