Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11566037:
Increase default maximum desktop size and make it configurable. (Closed)
|
Created:
8 years ago by Jamie Modified:
8 years ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, pam+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIncrease default maximum desktop size and make it configurable.
BUG=166099
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173214
Patch Set 1 #Patch Set 2 : Removed debug print #
Total comments: 6
Patch Set 3 : Fixed typo. #Patch Set 4 : Support a sensible initial size and a much larger maximum size by default. #
Total comments: 12
Patch Set 5 : Reviewer comments. #Messages
Total messages: 12 (0 generated)
ptal
https://codereview.chromium.org/11566037/diff/2001/remoting/tools/me2me_virtu... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/11566037/diff/2001/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:37: MAXIMUM_SIZE_ENV_VAR = "CHROME_REMOTE_DESKTOP_MAXIMUM_DESTOP_SIZE" typo: DESTOP https://codereview.chromium.org/11566037/diff/2001/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:752: DEFAULT_SIZE = "3840x1600" This is an insane default size, e.g. if the user connects with resize-to-client disabled; why not set MAXIMUM_SIZE to this, or to the size the user specifies, and have a separate default size?
https://codereview.chromium.org/11566037/diff/2001/remoting/tools/me2me_virtu... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/11566037/diff/2001/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:37: MAXIMUM_SIZE_ENV_VAR = "CHROME_REMOTE_DESKTOP_MAXIMUM_DESTOP_SIZE" On 2012/12/14 19:07:00, Wez wrote: > typo: DESTOP Done. https://codereview.chromium.org/11566037/diff/2001/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:752: DEFAULT_SIZE = "3840x1600" On 2012/12/14 19:07:00, Wez wrote: > This is an insane default size, e.g. if the user connects with resize-to-client > disabled; why not set MAXIMUM_SIZE to this, or to the size the user specifies, > and have a separate default size? We don't have a MAXIMUM_SIZE variable--it's computed from the -s options specified on the command-line. This is just the value that's used if no -s options are specified on the command-line. How about having an environment variable that can specify multiple default sizes, and having that default to, say, 1600x1200,3840x1600? I think that should work in both the resize and non-resize cases.
https://codereview.chromium.org/11566037/diff/2001/remoting/tools/me2me_virtu... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/11566037/diff/2001/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:752: DEFAULT_SIZE = "3840x1600" On 2012/12/14 19:29:16, Jamie wrote: > On 2012/12/14 19:07:00, Wez wrote: > > This is an insane default size, e.g. if the user connects with > resize-to-client > > disabled; why not set MAXIMUM_SIZE to this, or to the size the user specifies, > > and have a separate default size? > > We don't have a MAXIMUM_SIZE variable--it's computed from the -s options > specified on the command-line. This is just the value that's used if no -s > options are specified on the command-line. How about having an environment > variable that can specify multiple default sizes, and having that default to, > say, 1600x1200,3840x1600? I think that should work in both the resize and > non-resize cases. SGTM :)
https://codereview.chromium.org/11566037/diff/2001/remoting/tools/me2me_virtu... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/11566037/diff/2001/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:752: DEFAULT_SIZE = "3840x1600" On 2012/12/14 19:30:13, Wez wrote: > On 2012/12/14 19:29:16, Jamie wrote: > > On 2012/12/14 19:07:00, Wez wrote: > > > This is an insane default size, e.g. if the user connects with > > resize-to-client > > > disabled; why not set MAXIMUM_SIZE to this, or to the size the user > specifies, > > > and have a separate default size? > > > > We don't have a MAXIMUM_SIZE variable--it's computed from the -s options > > specified on the command-line. This is just the value that's used if no -s > > options are specified on the command-line. How about having an environment > > variable that can specify multiple default sizes, and having that default to, > > say, 1600x1200,3840x1600? I think that should work in both the resize and > > non-resize cases. > > SGTM :) Done.
https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:37: DEFAULT_SIZES_ENV_VAR = "CHROME_REMOTE_DESKTOP_DEFAULT_DESKTOP_SIZES" nit: Consider describing the format this variable must take, either here or better still in the usage output. https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:839: default_sizes = "1600x1200,3840x1600" nit: This still feels like something that should be a constant at the top of the file, aking to the X port number. https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:841: default_sizes = os.environ[DEFAULT_SIZES_ENV_VAR] Do we get enough of the user's environment for this to work as expected at boot time?
https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:834: # specified on the command-line, provide a relatively small size to handle In the non-RANDR case, we are taking the box-union of all the |sizes| and passing that for the "-screen" X server option - see _launch_x_server(). That is not "relatively small" :) We might have to revisit this logic if we really do want a small-ish default size? https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:839: default_sizes = "1600x1200,3840x1600" It's a strange letter-boxy default. I'd be inclined to increase the final 1600 to, say, 2560 (in case someone wants to rotate a large monitor into portrait mode, or wants to stack two smaller monitors vertically). WDYT?
https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:37: DEFAULT_SIZES_ENV_VAR = "CHROME_REMOTE_DESKTOP_DEFAULT_DESKTOP_SIZES" On 2012/12/14 19:49:03, Wez wrote: > nit: Consider describing the format this variable must take, either here or > better still in the usage output. Done. The usage output doesn't mention this variable, and I consider it to be an advanced option, so I think documenting it here is fine. https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:834: # specified on the command-line, provide a relatively small size to handle On 2012/12/14 19:59:05, Lambros wrote: > In the non-RANDR case, we are taking the box-union of all the |sizes| and > passing that for the "-screen" X server option - see _launch_x_server(). That is > not "relatively small" :) We might have to revisit this logic if we really do > want a small-ish default size? I don't think that the non-RANDR case is something we need to optimize for, given that it's by far the less common use-case. The user can always fix things via a .profile tweak. https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:839: default_sizes = "1600x1200,3840x1600" On 2012/12/14 19:59:05, Lambros wrote: > It's a strange letter-boxy default. I'd be inclined to increase the final 1600 > to, say, 2560 (in case someone wants to rotate a large monitor into portrait > mode, or wants to stack two smaller monitors vertically). WDYT? That increases Xvfb memory usage from 34 to 48Mb, which I think is starting to get too big for a default setup. We have to strike a balance between making sure that things "just work" for common use cases, without being too resource-hungry. I don't think we can make this work OOtB for all users, hence the environment variable. https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:839: default_sizes = "1600x1200,3840x1600" On 2012/12/14 19:49:03, Wez wrote: > nit: This still feels like something that should be a constant at the top of the > file, aking to the X port number. Done. https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:841: default_sizes = os.environ[DEFAULT_SIZES_ENV_VAR] On 2012/12/14 19:49:03, Wez wrote: > Do we get enough of the user's environment for this to work as expected at boot > time? Yes, Lambros did a test and confirmed that .profile is sourced bu su --login.
https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:839: default_sizes = "1600x1200,3840x1600" On 2012/12/14 21:13:17, Jamie wrote: > On 2012/12/14 19:59:05, Lambros wrote: > > It's a strange letter-boxy default. I'd be inclined to increase the final 1600 > > to, say, 2560 (in case someone wants to rotate a large monitor into portrait > > mode, or wants to stack two smaller monitors vertically). WDYT? > > That increases Xvfb memory usage from 34 to 48Mb, which I think is starting to > get too big for a default setup. We have to strike a balance between making sure > that things "just work" for common use cases, without being too resource-hungry. > I don't think we can make this work OOtB for all users, hence the environment > variable. Linux will only actually commit pages that we touch - that doesn't really help with the width, since pages are 4KB on x86, so the pages will fall on 1024 pixel boundaires, but it should mean that pages containing pixels beyond the bottom of the current dimensions just count against swap quota, in effect.
https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... File remoting/tools/me2me_virtual_host.py (right): https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... remoting/tools/me2me_virtual_host.py:839: default_sizes = "1600x1200,3840x1600" On 2012/12/14 21:19:20, Wez wrote: > On 2012/12/14 21:13:17, Jamie wrote: > > On 2012/12/14 19:59:05, Lambros wrote: > > > It's a strange letter-boxy default. I'd be inclined to increase the final > 1600 > > > to, say, 2560 (in case someone wants to rotate a large monitor into portrait > > > mode, or wants to stack two smaller monitors vertically). WDYT? > > > > That increases Xvfb memory usage from 34 to 48Mb, which I think is starting to > > get too big for a default setup. We have to strike a balance between making > sure > > that things "just work" for common use cases, without being too > resource-hungry. > > I don't think we can make this work OOtB for all users, hence the environment > > variable. > > Linux will only actually commit pages that we touch - that doesn't really help > with the width, since pages are 4KB on x86, so the pages will fall on 1024 pixel > boundaires, but it should mean that pages containing pixels beyond the bottom of > the current dimensions just count against swap quota, in effect. Is this a vote for increasing the height or just a comment?
On 2012/12/14 21:23:19, Jamie wrote: > https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... > File remoting/tools/me2me_virtual_host.py (right): > > https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... > remoting/tools/me2me_virtual_host.py:839: default_sizes = "1600x1200,3840x1600" > On 2012/12/14 21:19:20, Wez wrote: > > On 2012/12/14 21:13:17, Jamie wrote: > > > On 2012/12/14 19:59:05, Lambros wrote: > > > > It's a strange letter-boxy default. I'd be inclined to increase the final > > 1600 > > > > to, say, 2560 (in case someone wants to rotate a large monitor into > portrait > > > > mode, or wants to stack two smaller monitors vertically). WDYT? > > > > > > That increases Xvfb memory usage from 34 to 48Mb, which I think is starting > to > > > get too big for a default setup. We have to strike a balance between making > > sure > > > that things "just work" for common use cases, without being too > > resource-hungry. > > > I don't think we can make this work OOtB for all users, hence the > environment > > > variable. > > > > Linux will only actually commit pages that we touch - that doesn't really help > > with the width, since pages are 4KB on x86, so the pages will fall on 1024 > pixel > > boundaires, but it should mean that pages containing pixels beyond the bottom > of > > the current dimensions just count against swap quota, in effect. > > Is this a vote for increasing the height or just a comment? FWIW, I checked the memory usage using the "rss" column of ps, which is resident memory. Perhaps Xvfb is zeroing the framebuffer?
On 2012/12/14 21:26:44, Jamie wrote: > On 2012/12/14 21:23:19, Jamie wrote: > > > https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... > > File remoting/tools/me2me_virtual_host.py (right): > > > > > https://codereview.chromium.org/11566037/diff/4002/remoting/tools/me2me_virtu... > > remoting/tools/me2me_virtual_host.py:839: default_sizes = > "1600x1200,3840x1600" > > On 2012/12/14 21:19:20, Wez wrote: > > > On 2012/12/14 21:13:17, Jamie wrote: > > > > On 2012/12/14 19:59:05, Lambros wrote: > > > > > It's a strange letter-boxy default. I'd be inclined to increase the > final > > > 1600 > > > > > to, say, 2560 (in case someone wants to rotate a large monitor into > > portrait > > > > > mode, or wants to stack two smaller monitors vertically). WDYT? > > > > > > > > That increases Xvfb memory usage from 34 to 48Mb, which I think is > starting > > to > > > > get too big for a default setup. We have to strike a balance between > making > > > sure > > > > that things "just work" for common use cases, without being too > > > resource-hungry. > > > > I don't think we can make this work OOtB for all users, hence the > > environment > > > > variable. > > > > > > Linux will only actually commit pages that we touch - that doesn't really > help > > > with the width, since pages are 4KB on x86, so the pages will fall on 1024 > > pixel > > > boundaires, but it should mean that pages containing pixels beyond the > bottom > > of > > > the current dimensions just count against swap quota, in effect. > > > > Is this a vote for increasing the height or just a comment? > > FWIW, I checked the memory usage using the "rss" column of ps, which is resident > memory. Perhaps Xvfb is zeroing the framebuffer? Let's stick with the letter-box size for now, and revisit this or see if we can follow-up w/ a patch to optimize Xvfb not to touch the memory til it needs it. LGTM |
