Fix non-anti-aliased frame update.

Fix non-anti-aliased frame update.

m_contentsCangeCommitted (now renamed to m_contentsChangeResolved) was
not getting set on non-anti-aliased DrawingBuffers, causing us to always
skip setting m_animationFrameInProgress to true. This in turn skips
calling glReadPixels() for CPU-composited frames.

BUG=707298, 608873
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2793653002
Cr-Commit-Position: refs/heads/master@{#461542}
Committed: https://chromium.googlesource.com/chromium/src/+/5f8b467781b1b08d611e1d05f31c10dee5538edb

[capn, 2017-03-31 20:04:32]

The CQ bit was checked by capn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 20:05:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[capn, 2017-03-31 20:05:42]

Description was changed from

==========
Fix non-anti-aliased frame update.

m_contentsCangeCommitted (now renamed to m_contentsChangeResolved) was
not getting set on non-anti-aliased DrawingBuffers, causing us to always
skip setting m_animationFrameInProgress to true. This in turn skips
calling glReadPixels() for CPU-composited frames.

BUG=707298,608873
==========

to

==========
Fix non-anti-aliased frame update.

m_contentsCangeCommitted (now renamed to m_contentsChangeResolved) was
not getting set on non-anti-aliased DrawingBuffers, causing us to always
skip setting m_animationFrameInProgress to true. This in turn skips
calling glReadPixels() for CPU-composited frames.

BUG=707298,608873
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[capn, 2017-03-31 20:15:04]

capn@chromium.org changed reviewers:
+ junov@chromium.org, kainino@chromium.org

[capn, 2017-03-31 20:16:29]

PTAL

[Justin Novosad, 2017-03-31 20:26:54]

I think a simple unit test in DrawingBufferTest.cpp would be appropriate.

https://codereview.chromium.org/2793653002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2793653002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1012: void
DrawingBuffer::resolve() {
Nit: In blink, we like to use the suffix "IfNeeded" for methods that only do
something under certain conditions.

[commit-bot: I haz the power, 2017-03-31 21:32:02]

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

[commit-bot: I haz the power, 2017-03-31 21:32:03]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2017-04-01 00:07:19]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2017-04-01 00:07:20]

Good find and fix Nicolas.

Could you add a test for this? Perhaps a new pixel test that runs only on
Windows right now, and which does a couple of updates like glClears? In
src/content/test/gpu/gpu_tests/pixel_test_pages.py there are some tests which
avoid the use of golden images by specifying rectangles and colors, like
GpuRasterizationPages, which will simplify things.

[capn, 2017-04-03 17:41:18]

On 2017/03/31 20:26:54, Justin Novosad wrote:
> I think a simple unit test in DrawingBufferTest.cpp would be appropriate.

Certainly, but this bug has reached Stable and so I'd like to get this landed
ASAP before looking into how to prevent it from happening again. Also, the patch
fixes my minimized repro (crbug.com/707298#c6), but the affected game fails to
load due to some other issue which appears to require an official build. So I'd
like to test Canary soon.

>
https://codereview.chromium.org/2793653002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:1012: void
> DrawingBuffer::resolve() {
> Nit: In blink, we like to use the suffix "IfNeeded" for methods that only do
> something under certain conditions.

Done.

[capn, 2017-04-03 17:44:20]

On 2017/04/01 00:07:20, Ken Russell wrote:
> Good find and fix Nicolas.
> 
> Could you add a test for this? Perhaps a new pixel test that runs only on
> Windows right now, and which does a couple of updates like glClears? In
> src/content/test/gpu/gpu_tests/pixel_test_pages.py there are some tests which
> avoid the use of golden images by specifying rectangles and colors, like
> GpuRasterizationPages, which will simplify things.

Yes, I'm definitely planning to add a test for this since it broke all
non-anti-aliased uses of SwiftShader in Stable, and possibly some other
scenarios as well. Thanks for the pointers! Note my comment above for why I'd
like to land this before looking into writing tests for it.

[Ken Russell (switch to Gerrit), 2017-04-03 18:31:42]

On 2017/04/03 17:44:20, capn wrote:
> On 2017/04/01 00:07:20, Ken Russell wrote:
> > Good find and fix Nicolas.
> > 
> > Could you add a test for this? Perhaps a new pixel test that runs only on
> > Windows right now, and which does a couple of updates like glClears? In
> > src/content/test/gpu/gpu_tests/pixel_test_pages.py there are some tests
which
> > avoid the use of golden images by specifying rectangles and colors, like
> > GpuRasterizationPages, which will simplify things.
> 
> Yes, I'm definitely planning to add a test for this since it broke all
> non-anti-aliased uses of SwiftShader in Stable, and possibly some other
> scenarios as well. Thanks for the pointers! Note my comment above for why I'd
> like to land this before looking into writing tests for it.

It's most responsible to take the time to write the tests now. See for example
https://codereview.chromium.org/2791813003/, another serious bug which reached
stable but for which tests were written first.

[Ken Russell (switch to Gerrit), 2017-04-03 18:32:28]

I'll lgtm to unblock but would push for new tests along with the fix.

Ready to help with any testing.

[capn, 2017-04-03 19:10:02]

The CQ bit was unchecked by capn@chromium.org

[capn, 2017-04-03 19:10:05]

The CQ bit was checked by capn@chromium.org

[commit-bot: I haz the power, 2017-04-03 19:10:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 21:16:10]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491246605213430,
"parent_rev": "6b2de0f362490dc4823e64976d9682c37a62c64d", "commit_rev":
"5f8b467781b1b08d611e1d05f31c10dee5538edb"}

[commit-bot: I haz the power, 2017-04-03 21:16:53]

Description was changed from

==========
Fix non-anti-aliased frame update.

m_contentsCangeCommitted (now renamed to m_contentsChangeResolved) was
not getting set on non-anti-aliased DrawingBuffers, causing us to always
skip setting m_animationFrameInProgress to true. This in turn skips
calling glReadPixels() for CPU-composited frames.

BUG=707298,608873
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix non-anti-aliased frame update.

m_contentsCangeCommitted (now renamed to m_contentsChangeResolved) was
not getting set on non-anti-aliased DrawingBuffers, causing us to always
skip setting m_animationFrameInProgress to true. This in turn skips
calling glReadPixels() for CPU-composited frames.

BUG=707298,608873
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2793653002
Cr-Commit-Position: refs/heads/master@{#461542}
Committed:
https://chromium.googlesource.com/chromium/src/+/5f8b467781b1b08d611e1d05f31c...
==========

[commit-bot: I haz the power, 2017-04-03 21:16:54]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5f8b467781b1b08d611e1d05f31c...