Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
241223004:
Enable GPU rasterization by default on Android (Closed)
|
Created:
6 years, 8 months ago by ajuma Modified:
5 years, 6 months ago CC:
chromium-reviews, piman+watch_chromium.org, jam, darin-cc_chromium.org, alokp Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable GPU rasterization by default on Android
This enables hybrid GPU rasterization on Android.
BUG=362779
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264735
Patch Set 1 #Patch Set 2 : --enable-bleeding-edge-rendering-fast-paths should not bypass blacklist #
Total comments: 2
Patch Set 3 : Whitelist N4, N5 #
Total comments: 4
Patch Set 4 : Also whitelist N7 and MotoX #
Total comments: 3
Messages
Total messages: 16 (0 generated)
This blacklists Imagination (because of crashes on Galaxy Nexus). Do we want to blacklist more broadly or should we leave further blacklist additions to follow-up CLs?
On 2014/04/17 20:11:06, ajuma wrote: > This blacklists Imagination (because of crashes on Galaxy Nexus). Do we want to > blacklist more broadly or should we leave further blacklist additions to > follow-up CLs? We talked about this here this morning. I would prefer to be more conservative for starters and whitelist a few devices: N4, N5, N7, MotoX Also, I think the enable fast paths should not bypass the blacklist.
On 2014/04/17 20:53:36, vangelis wrote: > We talked about this here this morning. I would prefer to be more conservative > for starters and whitelist a few devices: > > N4, N5, N7, MotoX Sounds good. zmo, do we have a list somewhere that maps these devices to the ids we need to use in the blacklist? > Also, I think the enable fast paths should not bypass the blacklist. Fixed.
you can use the machine_model in blacklist for this. Need to look at each phone's machine_model in about:gpu. There are a few examples in software_rendering_list_json.cc https://codereview.chromium.org/241223004/diff/10001/content/browser/gpu/comp... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/241223004/diff/10001/content/browser/gpu/comp... content/browser/gpu/compositor_util.cc:289: #endif remove this
I've added entries for N4 and N5. If someone with an N7 or a Moto X could look up the value of "Machine model" in about:gpu, that would be much appreciated! https://codereview.chromium.org/241223004/diff/10001/content/browser/gpu/comp... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/241223004/diff/10001/content/browser/gpu/comp... content/browser/gpu/compositor_util.cc:289: #endif On 2014/04/17 21:24:14, Zhenyao Mo wrote: > remove this Done.
N7 is just "Nexus 7" I think. MotoX have a few models, including "XT1049", "XT1050", "XT1052", "XT1053", "XT1055", "XT1056", "XT1058", "XT1060". https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... File gpu/config/software_rendering_list_json.cc (right): https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... gpu/config/software_rendering_list_json.cc:1146: }, remove "," https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... gpu/config/software_rendering_list_json.cc:1154: }, remove ","
On 2014/04/17 22:12:25, Zhenyao Mo wrote: > N7 is just "Nexus 7" I think. > > MotoX have a few models, including "XT1049", "XT1050", "XT1052", "XT1053", > "XT1055", "XT1056", "XT1058", "XT1060". > > https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... > File gpu/config/software_rendering_list_json.cc (right): > > https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... > gpu/config/software_rendering_list_json.cc:1146: }, > remove "," > > https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... > gpu/config/software_rendering_list_json.cc:1154: }, > remove "," If you want to land as soon as possible, just add a bunch of exceptions. Otherwise, I am changing the machine_model semantic to be a list, so you can just have one exception.
On 2014/04/17 22:15:29, Zhenyao Mo wrote: > On 2014/04/17 22:12:25, Zhenyao Mo wrote: > > N7 is just "Nexus 7" I think. > > > > MotoX have a few models, including "XT1049", "XT1050", "XT1052", "XT1053", > > "XT1055", "XT1056", "XT1058", "XT1060". > > > > > https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... > > File gpu/config/software_rendering_list_json.cc (right): > > > > > https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... > > gpu/config/software_rendering_list_json.cc:1146: }, > > remove "," > > > > > https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... > > gpu/config/software_rendering_list_json.cc:1154: }, > > remove "," > > If you want to land as soon as possible, just add a bunch of exceptions. > > Otherwise, I am changing the machine_model semantic to be a list, so you can > just have one exception. Never mind. Just go ahead with the current semantic and add one exception per model.
On 2014/04/17 22:20:59, Zhenyao Mo wrote: > Just go ahead with the current semantic and add one exception per > model. Done. https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... File gpu/config/software_rendering_list_json.cc (right): https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... gpu/config/software_rendering_list_json.cc:1146: }, On 2014/04/17 22:12:25, Zhenyao Mo wrote: > remove "," Done. https://codereview.chromium.org/241223004/diff/30001/gpu/config/software_rend... gpu/config/software_rendering_list_json.cc:1154: }, On 2014/04/17 22:12:25, Zhenyao Mo wrote: > remove "," Done.
https://codereview.chromium.org/241223004/diff/50001/content/browser/gpu/comp... File content/browser/gpu/compositor_util.cc (left): https://codereview.chromium.org/241223004/diff/50001/content/browser/gpu/comp... content/browser/gpu/compositor_util.cc:278: } I see this is removed instead of moving after blacklist. Is this what you want?
https://codereview.chromium.org/241223004/diff/50001/content/browser/gpu/comp... File content/browser/gpu/compositor_util.cc (left): https://codereview.chromium.org/241223004/diff/50001/content/browser/gpu/comp... content/browser/gpu/compositor_util.cc:278: } On 2014/04/17 22:54:53, Zhenyao Mo wrote: > I see this is removed instead of moving after blacklist. > > > Is this what you want? Since we're already returning true after the blacklist check, checking this condition after the blacklist check doesn't change the return value (we return true either way).
LGTM
The CQ bit was checked by ajuma@chromium.org
https://codereview.chromium.org/241223004/diff/50001/content/browser/gpu/comp... File content/browser/gpu/compositor_util.cc (left): https://codereview.chromium.org/241223004/diff/50001/content/browser/gpu/comp... content/browser/gpu/compositor_util.cc:278: } On 2014/04/17 22:57:17, ajuma wrote: > On 2014/04/17 22:54:53, Zhenyao Mo wrote: > > I see this is removed instead of moving after blacklist. > > > > > > Is this what you want? > > Since we're already returning true after the blacklist check, checking this > condition after the blacklist check doesn't change the return value (we return > true either way). The only difference is for a system that's blacklisted, you can use --enable-bleeding-edge-rendering-fast-paths to turn the feature one. Now you can't. But it's your call, as you can still turn it one with --enable-gpu-rasterization.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/241223004/50001
Message was sent while issue was closed.
Change committed as 264735 |
