Bug 82033 - Make chromium android port use hardware gpu path default.
Summary: Make chromium android port use hardware gpu path default.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hao Zheng
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-23 01:31 PDT by Hao Zheng
Modified: 2012-04-26 11:56 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.61 KB, patch)
2012-03-23 01:33 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff
Patch (2.38 KB, patch)
2012-03-26 23:51 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hao Zheng 2012-03-23 01:31:22 PDT
Make chromium android port use hardware gpu path default.
Comment 1 Hao Zheng 2012-03-23 01:33:00 PDT
Created attachment 133443 [details]
Patch
Comment 2 Peter Beverloo 2012-03-23 07:14:23 PDT
Comment on attachment 133443 [details]
Patch

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

Thanks! Some drive-by nits.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:1
> +#!/usr/bin/env python

Do we invoke chromium_android.py directly? I don't think this is necessary if it's just being imported elsewhere.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:137
> +        'chromium-gpu',

These names correspond to directories in LayoutTests/platform/. All the chromium-gpu-* directories got removed in r110517.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:148
> +        # The chromium-android port always uses the hardware GPU code path,

textual nit: "The Chromium port for Android always uses the hardware GPU path."

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:237
> +        # All tests run in hardware gpu path, so don't need to run virtual gpu tests.

textual nit: "All tests are being run in the hardware GPU path, so there is no need to run virtual GPU tests."
Comment 3 Adam Barth 2012-03-23 09:15:15 PDT
Comment on attachment 133443 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:149
> +        self._graphics_type = 'gpu'

Does graphics_type still exist?  It seems like it should be removed now that the GPU configuration no longer exists.
Comment 4 Dirk Pranke 2012-03-23 11:00:51 PDT
Comment on attachment 133443 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:149
>> +        self._graphics_type = 'gpu'
> 
> Does graphics_type still exist?  It seems like it should be removed now that the GPU configuration no longer exists.

No, it doesn't still exist.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:238
> +        return []

To be more future-proof, you might want to call the base class and then remove the gpu suites, in case other suites get added down the road.
Comment 5 Hao Zheng 2012-03-25 21:13:52 PDT
(In reply to comment #4)
> (From update of attachment 133443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133443&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:149
> >> +        self._graphics_type = 'gpu'
> > 
> > Does graphics_type still exist?  It seems like it should be removed now that the GPU configuration no longer exists.
> 
> No, it doesn't still exist.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:238
> > +        return []
> 
> To be more future-proof, you might want to call the base class and then remove the gpu suites, in case other suites get added down the road.

I guess Adam is right. It's gone. We fall behind trunk some way, and apparently I need to rework this change. But anyway, I'd like to hear your opinion first on issues we come across.

As we always enable hardware gpu in our final build, we only need to run tests once in both real and virtual suites. 2 choices:
- Run these tests in real suites. Return [] for virtual suites. But we can not inherit baselines in platform/chromium-*/platform/chromium/virtual/gpu/, and can not reuse expectations like
BUGWK63933 : platform/chromium/virtual/gpu/fast/canvas/canvas-webkitLineDash.html = FAIL .

- Run these tests in virtual suites. This seems more natural to me. I can use skipped_tests() to skip these tests in real suites?

Which way looks better to you?
Comment 6 Dirk Pranke 2012-03-25 22:28:51 PDT
(In reply to comment #5)
> As we always enable hardware gpu in our final build, we only need to run tests once in both real and virtual suites. 2 choices:
> - Run these tests in real suites. Return [] for virtual suites. But we can not inherit baselines in platform/chromium-*/platform/chromium/virtual/gpu/, and can not reuse expectations like
> BUGWK63933 : platform/chromium/virtual/gpu/fast/canvas/canvas-webkitLineDash.html = FAIL .
> 
> - Run these tests in virtual suites. This seems more natural to me. I can use skipped_tests() to skip these tests in real suites?
> 
> Which way looks better to you?

I think you are right that you should do whichever is likely to let you reuse more baselines and expectations. Running them as virtual suites -- which at least means they have the same "hardware accelerated" usage as the other ports -- seems perfectly reasonable to me.

Sorry for the inconvenience; it's unfortunate that you're trying to merge things at the same time we were reworking the gpu ports, but at least that's done and I don't foresee anything similar on the horizon that should affect you.
Comment 7 Hao Zheng 2012-03-26 23:51:48 PDT
Created attachment 133989 [details]
Patch
Comment 8 Hao Zheng 2012-03-26 23:54:49 PDT
(In reply to comment #2)
> (From update of attachment 133443 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133443&action=review
> 
> Thanks! Some drive-by nits.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:1
> > +#!/usr/bin/env python
> 
> Do we invoke chromium_android.py directly? I don't think this is necessary if it's just being imported elsewhere.
> 

All scripts under webkitpy have such head. I just don't want android port to be an exception.

> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:137
> > +        'chromium-gpu',
> 
> These names correspond to directories in LayoutTests/platform/. All the chromium-gpu-* directories got removed in r110517.
> 

Done.

> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:148
> > +        # The chromium-android port always uses the hardware GPU code path,
> 

Done.

> textual nit: "The Chromium port for Android always uses the hardware GPU path."
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:237
> > +        # All tests run in hardware gpu path, so don't need to run virtual gpu tests.
> 
> textual nit: "All tests are being run in the hardware GPU path, so there is no need to run virtual GPU tests."

Removed.
Comment 9 Hao Zheng 2012-03-27 00:02:57 PDT
(In reply to comment #7)
> Created an attachment (id=133989) [details]
> Patch

I use skipped_tests(), which is not used by chromium port, because I find adding a line like 'WONTFIX ANDROID SKIP : fast/canvas = PASS FAIL' is not enough. More specific rules like 'BUGWEBGL : fast/canvas/webgl/read-pixels-test.html = TIMEOUT' would override the general SKIP rule, so the tests are still run. I have to override again using specific platform rules like 'WONTFIX ANDROID SKIP : fast/canvas/webgl/read-pixels-test.html = TIMEOUT'. This is annoying because we have to add one SKIP rule for Android each time we add one rule for canvas tests.
Comment 10 Dirk Pranke 2012-03-27 13:07:28 PDT
Comment on attachment 133989 [details]
Patch

I think using skipped_tests in this case is fine.
Comment 11 WebKit Review Bot 2012-03-28 00:05:24 PDT
Comment on attachment 133989 [details]
Patch

Clearing flags on attachment: 133989

Committed r112368: <http://trac.webkit.org/changeset/112368>
Comment 12 WebKit Review Bot 2012-03-28 00:05:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Xianzhu Wang 2012-04-26 11:56:58 PDT
Comment on attachment 133989 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:1
> +#!/usr/bin/env python

Just FYI: I think abarth intentionally left this out in the first check-in. The magic is useless in a file that is not excutable itself.