Bug 94254 – [chromium] Do not allow infinite pending frames in CCFrameRateController

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Brian Anderson]

[chromium] Do not allow infinite pendign frames in CCFrameRateController

[Brian Anderson]

Created attachment 158915 [details]
Patch

[Brian Anderson]

Created attachment 158922 [details]
Patch

[Brian Anderson]

Comment on attachment 158922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158922&action=review

Not sure how swap acks are supposed to work for testing.  Should we be seeing them during tests?  They don't show up currently, so I had to add fake swap acks.  The tests seem to rely on being able to submit infinite frames otherwise.

> Source/WebKit/chromium/tests/CCThreadedTest.cpp:155
> +    // run until after the lifetime of its client.

Is there a more automatic way to cancel tasks that outlive the objects it references?

> Source/WebKit/chromium/tests/CCThreadedTest.cpp:182
> +        CCProxy::implThread()->postDelayedTask(adoptPtr(swapAck), 15);

Do we want realistic swap ack delays for testing?  Or should we make this delay shorter/zero?

[James Robinson]

Nat / John - whatcha think?

[John Bates]

Comment on attachment 158922 [details]
Patch

Maybe there should be an assert(maxFramesPending > 0) in CCFrameRateController::setMaxFramesPending?

[Brian Anderson]

Created attachment 159507 [details]
Patch

[Brian Anderson]

Rebased.

Added assert(maxFramesPending > 0) back in to CCFrameRateController::setMaxFramesPending.  (It used to be part of a previous patch.)

Questions from comment #3 still apply.

[Nat Duca]

Comment on attachment 158922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158922&action=review

>> Source/WebKit/chromium/tests/CCThreadedTest.cpp:182
>> +        CCProxy::implThread()->postDelayedTask(adoptPtr(swapAck), 15);
> 
> Do we want realistic swap ack delays for testing?  Or should we make this delay shorter/zero?

How about using a CCTimer and a counter of numPending. Any time it ticks, decrement. If numPending still greater than 0, post another tick. Then you dont need a deque or cancel logic.

15 seems fine.

[WebKit Review Bot]

Comment on attachment 159507 [details]
Patch

Attachment 159507 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13551033

New failing tests:
platform/chromium/virtual/threaded/compositing/visibility/visibility-composited-transforms.html

[WebKit Review Bot]

Created attachment 159548 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Brian Anderson]

Created attachment 159575 [details]
Patch

[Nat Duca]

Comment on attachment 159575 [details]
Patch

Nice, lgtm

[James Robinson]

Comment on attachment 159575 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 159575 [details]
Patch

Attachment 159575 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13547248

New failing tests:
platform/chromium/virtual/threaded/compositing/visibility/visibility-composited-transforms.html

[WebKit Review Bot]

Created attachment 159585 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Brian Anderson]

How do I run this new failing test locally?
platform/chromium/virtual/threaded/compositing/visibility/visibility-composited-transforms.html

[Adrienne Walker]

(In reply to comment #16)
> How do I run this new failing test locally?
> platform/chromium/virtual/threaded/compositing/visibility/visibility-composited-transforms.html

Hmm, it looks like that test timed out.

That test is LayoutTests/compositing/visibility/visibility-composited-transforms.html run with threaded compositing turned on.  I added them as a smoke test for threaded compositing since the normal layout tests only exercise the single-threaded mode.

On Linux, to run this locally, build the DumpRenderTree target and then run 'new-run-webkit-tests --chromium --debug platform/chromium/virtual/threaded/compositing/visibility/visibility-composited-transforms.html'.

[Brian Anderson]

I could not reproduce the visibility-composited-transforms.html timeout/error, either on Windows or Linux, although I do get an unexpected EOF on stdout.

How can I re-run cr-linux on the bots?

Here's the output I get on my local machine:
"
$ new-run-webkit-tests --chromium --debug platform/chromium/virtual/threaded/compositing/visibility/visibility-composited-transforms.html
Using port 'chromium-linux-x86_64'
Test configuration: <lucid, x86_64, debug>
Placing test results in None
Baseline search path: chromium-linux -> chromium-win -> chromium -> mac -> generic
Using Debug build
Pixel tests enabled
Regular timeout: 12000, slow test timeout: 60000
Command line: /mnt/code/chrome0/src/out/Debug/DumpRenderTree -

Found 1 test; running 1, skipping 0.
Running 1 DumpRenderTree over 1 shard.

unexpected EOF of stdout                                                                             
The test ran as expected.
"

[Adrienne Walker]

(In reply to comment #18)
> I could not reproduce the visibility-composited-transforms.html timeout/error, either on Windows or Linux, although I do get an unexpected EOF on stdout.
> 
> How can I re-run cr-linux on the bots?

You could upload another patch and click the ews button.  Alternatively, you could also send off a try job to the linux_layout bot.

[Brian Anderson]

Created attachment 159792 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159792 [details]
Patch

Attachment 159792 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13569039

New failing tests:
platform/chromium/virtual/threaded/compositing/visibility/visibility-image-layers-dynamic.html
platform/chromium/virtual/threaded/compositing/visibility/visibility-composited-transforms.html

[WebKit Review Bot]

Created attachment 159818 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Eric Seidel (no email)]

Comment on attachment 158922 [details]
Patch

Cleared review? from obsolete attachment 158922 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Brian Anderson]

Created attachment 161388 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161388 [details]
Patch

Attachment 161388 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13682777

New failing tests:
platform/chromium/virtual/threaded/compositing/visibility/visibility-simple-canvas2d-layer.html
platform/chromium/virtual/threaded/compositing/webgl/webgl-no-alpha.html
platform/chromium/virtual/threaded/compositing/visibility/visibility-image-layers-dynamic.html

[Brian Anderson]

Created attachment 161734 [details]
Patch

[Brian Anderson]

The latest patch should eliminate the flakiness. The basic problem was that the implementations of WGC3D used by tests didn't support swap acks.  Instead of faking swap acks in the tests like previous patches, I went ahead and changed CCFrameRateController to not track number of pending frames if swap acks are not supported.

[James Robinson]

Comment on attachment 161734 [details]
Patch

R=me

[Brian Anderson]

Comment on attachment 161734 [details]
Patch

This one should be ready to go.

[WebKit Review Bot]

Comment on attachment 161734 [details]
Patch

Clearing flags on attachment: 161734

Committed r127476: <http://trac.webkit.org/changeset/127476>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.