Fix render state restore bug in DirectRenderer

Fix render state restore bug in DirectRenderer

When processing multiple copy requests, DirectRenderer must restore
state between requests. Due to a change from an index based for loop
to an iterator based loop, a check within the loop for the first
iteration had gotten out of date. This caused DirectRenderer to miss
a required state restore in some cases. This change restores the
intended behavior.

The software renderer had a bug where calling UseRenderPass multiple
times would result in a crash. The bug addressed in this CL made this a
more common occurrance. A fix to the software-renderer issue was
committed in CL (899183003). This change doesn't revert the software
renderer fix as it is still possible to hit the software renderer bug
even with this bug fixed. See the additional unit test for a case where
the software renderer fix is still necessary even with this patch.

R=danakj@chromium.org

Committed: https://crrev.com/eaf9258c49f76a997f690a63cb7e3091a09990b0
Cr-Commit-Position: refs/heads/master@{#315412}

[danakj, 2015-02-09 18:01:17]

https://codereview.chromium.org/904033008/diff/1/cc/trees/layer_tree_host_pix...
File cc/trees/layer_tree_host_pixeltest_readback.cc (right):

https://codereview.chromium.org/904033008/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_readback.cc:1147: class
LayerTreeHostReadbackNonFirstNonRootRenderPassPixelTest
Maybe we can rename this a bit more generally since we're using it for multiple
tests now. Like LayerTreeHostReadbackGLAndSoftwareTest?

(Offhand, it might be nice to apply this pattern to other tests in this file too
when they currently duplicate the test code)

https://codereview.chromium.org/904033008/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_readback.cc:1175:
MultipleReadbackNonRootOrFirstLayer) {
The test has multiple requests on blue which becomes the first renderpass drawn
in the renderer, so this isnt quite right for the name.
MultipleReadbacksOnLayer?

https://codereview.chromium.org/904033008/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_readback.cc:1188:
blue->RequestCopyOfOutput(CopyOutputRequest::CreateBitmapRequest(
I think we can simplify this test a bit, by just putting one extra request like
this on background and removing blue? That should still test that codepath
right?

https://codereview.chromium.org/904033008/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_readback.cc:1193: GetParam() ? PIXEL_TEST_GL
: PIXEL_TEST_SOFTWARE, READBACK_DEFAULT,
Hm, it occured to me if only one version of this test failed, i'd have no idea
if /0 and /1 (the test suffixes) meant GL or Software.. Do you think we can
change the param type from bool to this enum type directly? Then we can see the
order clearly in the definition of the TEST_P?

[ericrk, 2015-02-09 19:46:15]

Made suggested comments.
Also refactored the unit tests in layer_tree_host_pixeltest_readback.cc to be
parameterized. This removes a lot of duplicated code.

https://codereview.chromium.org/904033008/diff/1/cc/trees/layer_tree_host_pix...
File cc/trees/layer_tree_host_pixeltest_readback.cc (right):

https://codereview.chromium.org/904033008/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_readback.cc:1147: class
LayerTreeHostReadbackNonFirstNonRootRenderPassPixelTest
On 2015/02/09 18:01:17, danakj wrote:
> Maybe we can rename this a bit more generally since we're using it for
multiple
> tests now. Like LayerTreeHostReadbackGLAndSoftwareTest?
> 
> (Offhand, it might be nice to apply this pattern to other tests in this file
too
> when they currently duplicate the test code)

Updated the tests to use parameters. Now all of them run three configs -
Software, GL+Bitmap, GL+Default. Software+Default and Software+Bitmap appear to
be the same, so I always do Software+Default.

https://codereview.chromium.org/904033008/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_readback.cc:1175:
MultipleReadbackNonRootOrFirstLayer) {
On 2015/02/09 18:01:17, danakj wrote:
> The test has multiple requests on blue which becomes the first renderpass
drawn
> in the renderer, so this isnt quite right for the name.
> MultipleReadbacksOnLayer?

Done.

https://codereview.chromium.org/904033008/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_readback.cc:1188:
blue->RequestCopyOfOutput(CopyOutputRequest::CreateBitmapRequest(
On 2015/02/09 18:01:17, danakj wrote:
> I think we can simplify this test a bit, by just putting one extra request
like
> this on background and removing blue? That should still test that codepath
> right?

Done.

https://codereview.chromium.org/904033008/diff/1/cc/trees/layer_tree_host_pix...
cc/trees/layer_tree_host_pixeltest_readback.cc:1193: GetParam() ? PIXEL_TEST_GL
: PIXEL_TEST_SOFTWARE, READBACK_DEFAULT,
On 2015/02/09 18:01:17, danakj wrote:
> Hm, it occured to me if only one version of this test failed, i'd have no idea
> if /0 and /1 (the test suffixes) meant GL or Software.. Do you think we can
> change the param type from bool to this enum type directly? Then we can see
the
> order clearly in the definition of the TEST_P?

Good call - done.

[danakj, 2015-02-09 20:02:00]

LGTM

https://codereview.chromium.org/904033008/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_readback.cc (right):

https://codereview.chromium.org/904033008/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:428: // This test has 2 copy
requests on the background layer.For every copy
nit: space missing

https://codereview.chromium.org/904033008/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:436:
background->RequestCopyOfOutput(CopyOutputRequest::CreateBitmapRequest(
You just need one of these because one will come from passing background as the
target to RunReadbackTestWithReadbackTarget

[ericrk, 2015-02-09 21:09:22]

New patchsets have been uploaded after l-g-t-m from danakj@chromium.org

[ericrk, 2015-02-09 21:11:19]

https://codereview.chromium.org/904033008/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_readback.cc (right):

https://codereview.chromium.org/904033008/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:428: // This test has 2 copy
requests on the background layer.For every copy
On 2015/02/09 20:01:59, danakj wrote:
> nit: space missing

Done.

https://codereview.chromium.org/904033008/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:436:
background->RequestCopyOfOutput(CopyOutputRequest::CreateBitmapRequest(
On 2015/02/09 20:01:59, danakj wrote:
> You just need one of these because one will come from passing background as
the
> target to RunReadbackTestWithReadbackTarget

Ah, yeah - removed one of these.

[ericrk, 2015-02-09 21:15:18]

The CQ bit was checked by ericrk@chromium.org

[danakj, 2015-02-09 21:15:54]

LGTM

[commit-bot: I haz the power, 2015-02-09 21:16:11]

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

[commit-bot: I haz the power, 2015-02-09 22:37:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-09 22:38:20]

Patchset 3 (id:??) landed as
https://crrev.com/eaf9258c49f76a997f690a63cb7e3091a09990b0
Cr-Commit-Position: refs/heads/master@{#315412}