Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22144008:
cc: Make 1/4th default low res scale. (Closed)
|
Created:
7 years, 4 months ago by vmpstr Modified:
7 years, 2 months ago Reviewers:
aelias_OOO_until_Jul13, nduca, piman, brianderson, enne (OOO), Tom Hudson, epenner, brianderson_google CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, jam, apatrick_chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptioncc: Make 1/4th default low res scale.
This patch makes the default low res scale factor be 0.25 * high res
scale factor. The reasoning is that 0.125 looks too blurry. With low
quality low res tiles in play, 0.25 should still incur a performance
penalty, but it shouldn't be too bad, and look a bit better.
BUG=268203
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226371
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #Patch Set 3 : rebase#2 #
Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/22144008/diff/1/cc/layers/picture_layer_impl_... File cc/layers/picture_layer_impl_unittest.cc (right): https://codereview.chromium.org/22144008/diff/1/cc/layers/picture_layer_impl_... cc/layers/picture_layer_impl_unittest.cc:680: contents_scale = 2.f; This change is here since with the new low res contents scale of 0.25, it just happens to look like going 4 -> 1 creates a new tiling, where in reality the old high res becomes the new low res (with no new tiling created) https://codereview.chromium.org/22144008/diff/1/cc/trees/layer_tree_settings.cc File cc/trees/layer_tree_settings.cc (right): https://codereview.chromium.org/22144008/diff/1/cc/trees/layer_tree_settings.... cc/trees/layer_tree_settings.cc:39: minimum_contents_scale(0.0625f), I'm not sure if this should be changed as well. It was 0.5 * low contents scale, but I'm not sure if it should stay 0.0625 or be changed to 0.125.
aelias@, do you know how I would be able to get some performance numbers that this change makes on various android devices? I only have an N10, which isn't affected by this change...
+brian and tom and alex who helped with the initial discussions. Thoughts on making 1/4th default now that we have low quality mode on?
On 2013/08/05 18:44:03, nduca wrote: > +brian and tom and alex who helped with the initial discussions. > > Thoughts on making 1/4th default now that we have low quality mode on? What is the current state/data on low quality mode?
On 2013/08/05 18:44:03, nduca wrote: > +brian and tom and alex who helped with the initial discussions. > > Thoughts on making 1/4th default now that we have low quality mode on? Oh, wow, I had forgotten that https://chromiumcodereview.appspot.com/15995033 had landed. I'm Android build sheriff the next couple of days, and then WFH or on vacation for the tail end of the week, so may not be able to run experiments. But the idea sounds good, and when I have the chance I can get 4+ devices to test this on. I'd try running smoothness and rasterize_and_record, but I don't know if Manfred's latest patches are going to be enough to fix r&r on Android? If I do end up too busy as sheriff, we can ask Sami or someone from Clank MTV to do the testing for us.
Low quality for low res tiles is on by default. I ran the experiment on N10 when I was developing it. The numbers are here: https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0ApcKk7MxQ2LsdGcxQ... tl;dr; is that depending on the page it's 10-20% time savings.
On 2013/08/05 18:58:26, vmpstr wrote: > Low quality for low res tiles is on by default. > > I ran the experiment on N10 when I was developing it. The numbers are here: > https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0ApcKk7MxQ2LsdGcxQ... > > tl;dr; is that depending on the page it's 10-20% time savings. (btw, this was with the old benchmark code)
What's the motivation for 1/4 vs 1/8? 1/8 is too blurry? 1/8 eats up too much raster time because it covers 4x the area so gets invalidated more often?
On 2013/08/06 16:37:41, enne wrote: > What's the motivation for 1/4 vs 1/8? 1/8 is too blurry? 1/8 eats up too much > raster time because it covers 4x the area so gets invalidated more often? Oops, sorry. Apparently I can't read patch descriptions. I see it's both. I'd be interested to see what sort of a performance difference this makes.
On 2013/08/06 16:38:59, enne wrote: > On 2013/08/06 16:37:41, enne wrote: > > What's the motivation for 1/4 vs 1/8? 1/8 is too blurry? 1/8 eats up too much > > raster time because it covers 4x the area so gets invalidated more often? > > Oops, sorry. Apparently I can't read patch descriptions. I see it's both. I'd > be interested to see what sort of a performance difference this makes. That patch description is kinda ambiguous. I meant to say that since we'll have more low res tiles, the raster cost is probably expected to go up (total, not per tile). However, because we have low quality low res tiles, that cost shouldn't be too bad. And yes, I would like to generate/see some actual concrete numbers before landing this.
Again, the goal here is to eliminate the device-specific scales and use one scale that we use everywhere. Or, barring that, come up with a principled way to choose the scale. The way it is now is definitely not how we should be doing things.
On 2013/08/06 18:04:37, nduca wrote: > Again, the goal here is to eliminate the device-specific scales and use one > scale that we use everywhere. Or, barring that, come up with a principled way to > choose the scale. The way it is now is definitely not how we should be doing > things. Which part of this discussion do you feel is not in that spirit? Most of the questions here are about performance, which seems like a good data point from which to choose a scale.
I wasn't disagreeing with the conversation. I was trying to offer context. Back in the day, the UX guidance we got from Android was to go to 1/4th everywhere if possible. I still think thats the correct thing to do, across the board. We didn't do that because of rasterization performance, and said we needed low quality mode to get there. We went a route of choosing this device by device saying that we'd go to 1/4h once we had low quality mode. I worry we're risking an analysis paralysis here. Lets definitely get the numbers, but unless they're really horrible, we should probably take the hit and go to 1/4th. If we then have to file followup bugs that skip rasterizing low quality when we know its not needed, that seems preferrable than staying at 1/8th & 1/4th depending on device.
On 2013/09/26 17:26:54, enne wrote: > lgtm, cf https://codereview.chromium.org/24456003/#msg3 I couldn't find the bug for this so I made a duplicate CL. LGTM and I'll delete mine once this lands.
On 2013/09/26 19:42:18, epenner wrote: > On 2013/09/26 17:26:54, enne wrote: > > lgtm, cf https://codereview.chromium.org/24456003/#msg3 > > I couldn't find the bug for this so I made a duplicate CL. > > LGTM and I'll delete mine once this lands. I plan put this in CQ sometime tomorrow, to give more people a change to comment. Thanks.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/22144008/1
Failed to apply patch for content/browser/gpu/gpu_data_manager_impl_private.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/browser/gpu/gpu_data_manager_impl_private.cc
Hunk #1 FAILED at 269.
Hunk #2 succeeded at 300 (offset -18 lines).
1 out of 2 hunks FAILED -- saving rejects to file
content/browser/gpu/gpu_data_manager_impl_private.cc.rej
Patch: content/browser/gpu/gpu_data_manager_impl_private.cc
Index: content/browser/gpu/gpu_data_manager_impl_private.cc
diff --git a/content/browser/gpu/gpu_data_manager_impl_private.cc
b/content/browser/gpu/gpu_data_manager_impl_private.cc
index
1bd1f6b95d9db1927cbdb08529964b0494d9bc22..546f0ac86eac89157d1de5f24da07276e2de3231
100644
--- a/content/browser/gpu/gpu_data_manager_impl_private.cc
+++ b/content/browser/gpu/gpu_data_manager_impl_private.cc
@@ -269,11 +269,6 @@ void ApplyAndroidWorkarounds(const gpu::GPUInfo& gpu_info,
gpu_info.gl_extensions.find("GL_VIV_shader_binary") !=
std::string::npos;
- bool is_nexus7 =
- gpu_info.machine_model.find("Nexus 7") != std::string::npos;
- bool is_nexus10 =
- gpu_info.machine_model.find("Nexus 10") != std::string::npos;
-
// IMG: avoid context switching perf problems, crashes with share groups
// Mali-T604: http://crbug.com/154715
// QualComm, NVIDIA: Crashes with share groups
@@ -318,14 +313,6 @@ void ApplyAndroidWorkarounds(const gpu::GPUInfo& gpu_info,
command_line->AppendSwitchASCII(
switches::kDefaultTileHeight, size.str());
}
-
- // Increase the resolution of low resolution tiles for Nexus tablets.
- if ((is_nexus7 || is_nexus10) &&
- !command_line->HasSwitch(
- cc::switches::kLowResolutionContentsScaleFactor)) {
- command_line->AppendSwitchASCII(
- cc::switches::kLowResolutionContentsScaleFactor, "0.25");
- }
}
#endif // OS_ANDROID
+piman for content/browser/gpu/
On 2013/09/30 17:06:24, vmpstr wrote: > +piman for content/browser/gpu/ Ping for owner's review.
On 2013/10/01 17:23:42, vmpstr wrote: > On 2013/09/30 17:06:24, vmpstr wrote: > > +piman for content/browser/gpu/ > > Ping for owner's review. LGTM - sorry, I thought I already reviewed this. But my lgtm doesn't show :(
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/22144008/26001
Message was sent while issue was closed.
Change committed as 226371 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
