Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
111893004:
Change bench_pictures configs (Closed)
|
Created:
7 years ago by borenet Modified:
7 years ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionChange bench_pictures configs
Aiming at CPU vs GPU
R=epoger@google.com, robertphillips@google.com
Committed: https://code.google.com/p/skia/source/detail?r=12799
Patch Set 1 #
Total comments: 1
Patch Set 2 : normalize everything #
Total comments: 5
Patch Set 3 : Remove rtree, enable gpu on N10 #Messages
Total messages: 15 (0 generated)
It looks like we don't have a viewport config set up for ANGLE and N10 msaa. Also, will this change correctly strip off the width_height suffix from the viewport configs?
Just remembered, is this also where we're going to plumb in the scaled (1.1?) tests?
lgtm https://codereview.chromium.org/111893004/diff/1/tools/bench_pictures.cfg File tools/bench_pictures.cfg (right): https://codereview.chromium.org/111893004/diff/1/tools/bench_pictures.cfg#new... tools/bench_pictures.cfg:41: # Viewport CPU and GPU As it stands in patchset 1, looks to me like this CL just removes the tiled configs from bench_pictures. Which I think is a fine first step, and a good candidate for its own CL (perhaps renamed something like "remove tiled configs from bench_pictures") As Rob notes, more changes to come.
Rob, I don't feel particularly comfortable removing the width_height from the config names as long as the viewports are different sizes. It would imply that they are equivalent when in reality they aren't. Should we change to a single viewport size? And yes, I intend to add scaled configs here as well.
On 2013/12/11 22:05:18, borenet wrote: > Rob, I don't feel particularly comfortable removing the width_height from the > config names as long as the viewports are different sizes. It would imply that > they are equivalent when in reality they aren't. Should we change to a single > viewport size? > > And yes, I intend to add scaled configs here as well. My feeling is that using a standard size across all configs makes sense -- it gives a common point of comparison.
On 2013/12/12 13:29:33, JimVV wrote: > On 2013/12/11 22:05:18, borenet wrote: > > Rob, I don't feel particularly comfortable removing the width_height from the > > config names as long as the viewports are different sizes. It would imply > that > > they are equivalent when in reality they aren't. Should we change to a single > > viewport size? > > > > And yes, I intend to add scaled configs here as well. > > My feeling is that using a standard size across all configs makes sense -- it > gives a common point of comparison. I agree with Jim. I don't think the viewport should be related to the screen size.
On 2013/12/12 16:17:06, bsalomon wrote: > On 2013/12/12 13:29:33, JimVV wrote: > > On 2013/12/11 22:05:18, borenet wrote: > > > Rob, I don't feel particularly comfortable removing the width_height from > the > > > config names as long as the viewports are different sizes. It would imply > > that > > > they are equivalent when in reality they aren't. Should we change to a > single > > > viewport size? > > > > > > And yes, I intend to add scaled configs here as well. > > > > My feeling is that using a standard size across all configs makes sense -- it > > gives a common point of comparison. > > I agree with Jim. I don't think the viewport should be related to the screen > size. SGTM. How big should the Universal Viewport be?
On 2013/12/12 20:37:50, borenet wrote: > On 2013/12/12 16:17:06, bsalomon wrote: > > On 2013/12/12 13:29:33, JimVV wrote: > > > On 2013/12/11 22:05:18, borenet wrote: > > > > Rob, I don't feel particularly comfortable removing the width_height from > > the > > > > config names as long as the viewports are different sizes. It would imply > > > that > > > > they are equivalent when in reality they aren't. Should we change to a > > single > > > > viewport size? > > > > > > > > And yes, I intend to add scaled configs here as well. > > > > > > My feeling is that using a standard size across all configs makes sense -- > it > > > gives a common point of comparison. > > > > I agree with Jim. I don't think the viewport should be related to the screen > > size. > > SGTM. How big should the Universal Viewport be? This can be tricky. I think we should keep the universal viewport size in the name, since over time that "universal" size may change - imagine someday all devices we care about are at least Retina. Currently NexusS has the smallest 480x800. If we don't make assumptions on horizontal or vertical direction, the safest would be 480x480. However that seems a little small...
Patch set 2. 1. Make the Android bots do the same config list as the others. 2. Add scaled (1.1) CPU and GPU configs 3. Set the default viewport to 1000x1000 and use it throughout. I think I remember Mike saying 1000x1000 at some point. Will change if we have a better setting. https://codereview.chromium.org/111893004/diff/20001/tools/bench_pictures.cfg File tools/bench_pictures.cfg (right): https://codereview.chromium.org/111893004/diff/20001/tools/bench_pictures.cfg... tools/bench_pictures.cfg:83: 'nexus_10': default_no_gpu + [msaa4], I don't remember why the N10 was running without gpu configs - maybe because its viewport was too big to fit in GPU memory? If that's the case, we can probably turn it back on, right?
I'm a bit worried that we have no coverage on the tiled rendering speed since Chrome uses that. Would it be possible to do a 250x250 tiling of the 1000x1000 viewport? https://codereview.chromium.org/111893004/diff/20001/tools/bench_pictures.cfg File tools/bench_pictures.cfg (right): https://codereview.chromium.org/111893004/diff/20001/tools/bench_pictures.cfg... tools/bench_pictures.cfg:56: # Different bounding box heirarchies, for different modes. Remove RecordRTreeConfig()? https://codereview.chromium.org/111893004/diff/20001/tools/bench_pictures.cfg... tools/bench_pictures.cfg:83: 'nexus_10': default_no_gpu + [msaa4], Right!
Uploaded patch set 3. As for viewport + tile, I don't know whether that works right now. We should verify that it works (or make it work) and then we can add that config here. https://codereview.chromium.org/111893004/diff/20001/tools/bench_pictures.cfg File tools/bench_pictures.cfg (right): https://codereview.chromium.org/111893004/diff/20001/tools/bench_pictures.cfg... tools/bench_pictures.cfg:56: # Different bounding box heirarchies, for different modes. On 2013/12/19 17:41:15, robertphillips wrote: > Remove RecordRTreeConfig()? Gladly. https://codereview.chromium.org/111893004/diff/20001/tools/bench_pictures.cfg... tools/bench_pictures.cfg:83: 'nexus_10': default_no_gpu + [msaa4], On 2013/12/19 17:41:15, robertphillips wrote: > Right! Done.
LGTM. I propose we land this as is and then re-evaluate.
On 2013/12/20 14:36:35, robertphillips wrote: > LGTM. I propose we land this as is and then re-evaluate. Sounds fine to me.
Message was sent while issue was closed.
Committed patchset #3 manually as r12799 (presubmit successful). |
