Bug 72402 - [chromium] Merge chromium-gpu layout test configurations into non-gpu versions
Summary: [chromium] Merge chromium-gpu layout test configurations into non-gpu versions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on: 72444
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-15 11:36 PST by Adrienne Walker
Modified: 2011-11-16 01:25 PST (History)
12 users (show)

See Also:


Attachments
work in progress (12.83 KB, patch)
2011-11-15 13:52 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (15.60 KB, patch)
2011-11-15 15:37 PST, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-11-15 11:36:45 PST
Chromium GPU tests have their own separate buildbot step and set of expectations.  This causes extra complexity, gardening pain, configuration overhead, and on-going confusion about who's responsible for them.  These tests should not be special.

There's no reason why these tests can't be part of the layout_tests step.  Arguably, we could just not disable hardware acceleration for all layout tests and let the GPU tests trigger hardware acceleration as-needed.

The only real sticking point is the canvas tests which are run in both hardware and software mode.  senorblanco argues that we could just run these in hardware, which would sidestep the problem entirely.
Comment 1 James Robinson 2011-11-15 12:42:48 PST
Completely agree.  I think the best way to tackle this is piece-by-piece.  The easiest thing to tackle is the compositing/webgl/3d animation tests - they should just run as normal layout tests using osmesa as appropriate.  I'll take a look at doing this today.

We can leave canvas 2d in a split configuration for now, with the assumption that longer-term skia could be responsible for testing these different paths.
Comment 2 James Robinson 2011-11-15 13:01:34 PST
I think I can just make DRT always enable compositing (with the normal compositing triggers) and then just run the compositing/ directories through the normal DRT path.  Our normal compositing triggers should not trip on any tests outside of these directories.
Comment 3 James Robinson 2011-11-15 13:10:09 PST
Hm, another potential issue is media/ - currently we run that through the compositing and non-compositing paths separately and have separate baselines for the two.  If we think running these tests two times is valuable then I could rig up in the same way as canvas 2d to run without the compositor on the main layout test run and then run with the compositor triggered by media in webkit_gpu_tests.
Comment 4 Adrienne Walker 2011-11-15 13:44:00 PST
Nutty idea: could we add a feature to DRT such that it mirrors a given directory, but with a variation like a js file forcibly injected into each one? Then we could have a virtual mirror of media/ called media-gpu/, and containing nothing but expectations.  With this, we could still run tests in both configs, wouldn't have to manually duplicate them, and could eliminate the separate buildbot step.  On the other hand, this forces different complexity into DRT and gardening tools.  But, maybe this feature would be useful in cases other than GPU land?
Comment 5 James Robinson 2011-11-15 13:52:12 PST
Created attachment 115236 [details]
work in progress
Comment 6 James Robinson 2011-11-15 13:58:29 PST
We currently have 562 files (expectations and tests) in LayoutTests/platform/chromium*gpu*/compositing/*.  I plan to do the following in order to land this:

1.) Copy all existing compositing expectations in gpu directories to the non-gpu-specific directory.  For example, everything in platform/chromium-gpu-win/compositing/ will be copied to platform/chromium-win/

2.) Land the code change to DumpRenderTree and our test scripts to run the compositing tests on the normal layout test run and not in the GPU run

3.) Remove the compositing expectations from platform/chromium-*gpu*/

4.) Remove the SKIP from these lines in test_expectations.txt:

WONTFIX SKIP CPU CPU-CG : compositing = PASS TIMEOUT FAIL
WONTFIX SKIP CPU CPU-CG : platform/chromium/compositing = PASS TIMEOUT FAIL
WONTFIX SKIP CPU CPU-CG : animations/3d = PASS TIMEOUT FAIL

to start running the tests.

5.) Garden the set of failures from these lines down to zero

6.) Remove the = FAIL lines for compositing/ and animations/3d from test_expectations.txt

7.) Profit
Comment 7 Ojan Vafai 2011-11-15 14:06:19 PST
6a) Remove run-webkit-tests support for the special -gpu configurations
6b) Remove all knowledge of the -gpu bots from the flakiness dashboard
Comment 8 James Robinson 2011-11-15 14:08:18 PST
(In reply to comment #7)
> 6a) Remove run-webkit-tests support for the special -gpu configurations
> 6b) Remove all knowledge of the -gpu bots from the flakiness dashboard

Can't do those until the canvas 2d and media tests are resolved. I'm only planning to move the compositing/ tests over right now since we only ever want to run those in one configuration (with compositing enabled via osmesa software rasterizer).  For canvas 2d and media we'll need to do a bit more work.
Comment 9 Dirk Pranke 2011-11-15 14:12:30 PST
(In reply to comment #4)
> Nutty idea: could we add a feature to DRT such that it mirrors a given directory, but with a variation like a js file forcibly injected into each one? Then we could have a virtual mirror of media/ called media-gpu/, and containing nothing but expectations.  With this, we could still run tests in both configs, wouldn't have to manually duplicate them, and could eliminate the separate buildbot step.  On the other hand, this forces different complexity into DRT and gardening tools.  But, maybe this feature would be useful in cases other than GPU land?

We'd have to modify NRWT to know how to mirror the trees so that it "found" both sets of tests, but that is presumably doable.

Also, your idea would I think require us to have 2x the number of expectations. I don't know how many of our expectations are duplicated today, or whether this would be much of an issue. I am curious how many of these tests could be expressed as reference tests. I also wonder if we could figure out how to express CPU/GPU as a reference test (i.e., the CPU's version should match the GPU's version)?

[ If we didn't duplicate the expectations, we'd have to define a fallback path and modifiers for what happens when you get differing results, which would put us back to where we are today. ]
Comment 10 WebKit Review Bot 2011-11-15 14:49:14 PST
Comment on attachment 115236 [details]
work in progress

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

New failing tests:
fast/media/mq-transform-03.html
transitions/default-timing-function.html
animations/missing-values-first-keyframe.html
fast/repaint/block-selection-gap-in-composited-layer.html
fast/media/mq-transform-02.html
animations/opacity-transform-animation.html
animations/missing-values-last-keyframe.html
Comment 12 James Robinson 2011-11-15 15:23:35 PST
(In reply to comment #10)
> (From update of attachment 115236 [details])
> Attachment 115236 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10372697
> 
> New failing tests:
> fast/media/mq-transform-03.html
> transitions/default-timing-function.html
> animations/missing-values-first-keyframe.html
> fast/repaint/block-selection-gap-in-composited-layer.html
> fast/media/mq-transform-02.html
> animations/opacity-transform-animation.html
> animations/missing-values-last-keyframe.html

These are most likely failing due to the compositing trigger for animating a non-3d transform. Currently they run with compositing disabled on the main layout test run. With this patch, they would run with compositing enabled.  I think we should just rebaseline with the compositing expectations, since that's the mode we run in actual chromium.
Comment 13 James Robinson 2011-11-15 15:37:05 PST
Created attachment 115263 [details]
Patch
Comment 14 WebKit Review Bot 2011-11-15 15:39:23 PST
Attachment 115263 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

ERROR: FAILURES FOR <lucid, x86_64, release, cpu> in LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:2735 Path does not exist. plugins/invalidate_rect.html
LayoutTests/platform/chromium/test_expectations.txt:2735:  Path does not exist. plugins/invalidate_rect.html  [test/expectations] [2]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Dirk Pranke 2011-11-15 15:47:21 PST
Comment on attachment 115263 [details]
Patch

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

> Tools/ChangeLog:10
> +        and video are still controlled by an explicit trigger so that they are true when platform=chromium-gpu and false

Nit: maybe say "media" instead of video (or both) for people that aren't aware that the two are synonymous?

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:431
> +            'accelerated_video': '--enable-accelerated-video',

I think you need to change --accelerated-compositing to --accelerated-video in run_webkit_tests.py as well (line220).
Comment 16 James Robinson 2011-11-15 16:02:46 PST
Committed r100355: <http://trac.webkit.org/changeset/100355>
Comment 17 James Robinson 2011-11-15 16:03:11 PST
Comment on attachment 115263 [details]
Patch

Clearing flags, this patch landed.
Comment 18 James Robinson 2011-11-15 16:03:46 PST
(In reply to comment #16)
> Committed r100355: <http://trac.webkit.org/changeset/100355>

This completes step 2 from comment #6.  I'm leaving this bug open to cover the rest of the steps there.
Comment 19 James Robinson 2011-11-15 16:25:11 PST
Committed r100360: <http://trac.webkit.org/changeset/100360>
Comment 20 James Robinson 2011-11-15 16:25:59 PST
http://trac.webkit.org/changeset/100360 handles steps 3 and 4.
Comment 21 James Robinson 2011-11-15 23:48:55 PST
Committed r100415: <http://trac.webkit.org/changeset/100415>
Comment 22 James Robinson 2011-11-16 01:25:07 PST
OK, the compositing tests are now running on the main webkit_tests run only and I've gardened down to 16 failures, which seems small enough to handle by more specialized bugs.  I'm going to mark this bug resolved.

Open issues are how to handle the canvas 2d and media tests.  Right now, those tests are still using webkit_gpu_tests.  I'll open up bugs for those two categories tomorrow to discuss options.