monitor_reconfigure: Try to improve resolution selection.

monitor_reconfigure: Try to improve resolution selection.

My initial thinking was that this should work as follows:
- find the device with the lowest max resolution
- set the other device to use the nearest resolution that
  it has to the first's (that's >= it)
- set the screen to use the first, smaller device's
  resolution (with black bars taking up the extra space
  on the larger device)

xrandr doesn't appear to let the screen be a lower resolution
than any of the devices, though.  Regardless, I tried this
version with a low-res Eee, a high-res Eee, and an X200, on
both a projector and on a VC device, and it did something
reasonable on all of them, so it seems like it's probably
worth checking it in soon to make Kan happy and then refining
it further. :-)

BUG=chromium-os:1796
TEST=tried it on an X200 and various Eee PCs with a few projectors

[Daniel Erat, 2010-04-16 01:23:38]

http://codereview.chromium.org/1646010/diff/1/3
File src/platform/monitor_reconfig/monitor_reconfigure_main.h (right):

http://codereview.chromium.org/1646010/diff/1/3#newcode49
src/platform/monitor_reconfig/monitor_reconfigure_main.h:49: bool
FindBestResolutions(
Had to catch the shuttle before I could add descriptions here, but I'll put them
in when I'm back in the office.

http://codereview.chromium.org/1646010/diff/1/3#newcode54
src/platform/monitor_reconfig/monitor_reconfigure_main.h:54: bool
FindNearestResolutions(
Chooses the highest resolution in |worse_device_modes| and then finds the lowest
resolution in |better_device_modes| that's larger than it.

These variable names are pretty bad; let me know if you can come up with
something better.  "better" and "worse" here refer to the highest resolution
offered by the devices.

[sosa, 2010-04-16 17:39:19]

Overall looks good.  Looking through your code review I think I finally see what
happened downstairs.  

I always assumed I think that the LVDS1 had the worst resolutions, but with the
1366x768 resolution of the newer machines, and prob 1280x1024 res of the
projector downstairs, the projector downstairs couldn't displays the last 74
pixels.  

Were you able to check in the changes to catch the xrandr resizes?  Otherwise,
the issue downstairs will still happen since the default res. is 1366 and if it
can't be re-sized will stay at 1366 even though xrandr has changed res res size
of the outputs down to 1280x800, 1280x1024

Also, about xrandr --fb ... I noted that you could set screen to a smaller size
(with an error/warning), but it would still respect it ... i.e. X wouldn't let
the mouse cursor move beyond the window and send the window manager the right
pixels to size it too.

http://codereview.chromium.org/1646010/diff/1/2
File src/platform/monitor_reconfig/monitor_reconfigure_main.cc (right):

http://codereview.chromium.org/1646010/diff/1/2#newcode123
src/platform/monitor_reconfig/monitor_reconfigure_main.cc:123: // to hold all
the devices.
xrandr gives an error but still sets it in my experience (from the tool at
least)

http://codereview.chromium.org/1646010/diff/1/2#newcode129
src/platform/monitor_reconfig/monitor_reconfigure_main.cc:129: return false;
Add a comment that this is only reached if the better is not in fact >= worse?

http://codereview.chromium.org/1646010/diff/1/2#newcode149
src/platform/monitor_reconfig/monitor_reconfigure_main.cc:149: // supported by
the LCD.
Great! I wanted to add this before, but my original assumption was that I should
never change the resolution of the lcd (since the window manager couldn't resize
when I first wrote this), so I never had to change the resolution back if
disconnected.

http://codereview.chromium.org/1646010/diff/1/2#newcode166
src/platform/monitor_reconfig/monitor_reconfigure_main.cc:166: // TODO: Maybe
avoid setting the screen res; xrandr should choose it for us.
I believe the issue was it didn't always set it so this was a safety push

http://codereview.chromium.org/1646010/diff/1/3
File src/platform/monitor_reconfig/monitor_reconfigure_main.h (right):

http://codereview.chromium.org/1646010/diff/1/3#newcode54
src/platform/monitor_reconfig/monitor_reconfigure_main.h:54: bool
FindNearestResolutions(
Probably smaller vs. larger is more clear worse/better, but I agree, when I was
first doing it myself, I had some bad names because it was hard to find good
names when doing this comparison and keep it generic.

On 2010/04/16 01:23:38, Daniel Erat wrote:
> Chooses the highest resolution in |worse_device_modes| and then finds the
lowest
> resolution in |better_device_modes| that's larger than it.
> 
> These variable names are pretty bad; let me know if you can come up with
> something better.  "better" and "worse" here refer to the highest resolution
> offered by the devices.

[sosa, 2010-04-16 20:12:47]

As a note, none of my questions should block this getting in.  LGTM

On Fri, Apr 16, 2010 at 10:39 AM,  <sosa@chromium.org> wrote:
> Overall looks good.  Looking through your code review I think I finally see
> what
> happened downstairs.
>
> I always assumed I think that the LVDS1 had the worst resolutions, but with
> the
> 1366x768 resolution of the newer machines, and prob 1280x1024 res of the
> projector downstairs, the projector downstairs couldn't displays the last 74
> pixels.
>
> Were you able to check in the changes to catch the xrandr resizes?
>  Otherwise,
> the issue downstairs will still happen since the default res. is 1366 and if
> it
> can't be re-sized will stay at 1366 even though xrandr has changed res res
> size
> of the outputs down to 1280x800, 1280x1024
>
> Also, about xrandr --fb ... I noted that you could set screen to a smaller
> size
> (with an error/warning), but it would still respect it ... i.e. X wouldn't
> let
> the mouse cursor move beyond the window and send the window manager the
> right
> pixels to size it too.
>
>
> http://codereview.chromium.org/1646010/diff/1/2
> File src/platform/monitor_reconfig/monitor_reconfigure_main.cc (right):
>
> http://codereview.chromium.org/1646010/diff/1/2#newcode123
> src/platform/monitor_reconfig/monitor_reconfigure_main.cc:123: // to
> hold all the devices.
> xrandr gives an error but still sets it in my experience (from the tool
> at least)
>
> http://codereview.chromium.org/1646010/diff/1/2#newcode129
> src/platform/monitor_reconfig/monitor_reconfigure_main.cc:129: return
> false;
> Add a comment that this is only reached if the better is not in fact >=
> worse?
>
> http://codereview.chromium.org/1646010/diff/1/2#newcode149
> src/platform/monitor_reconfig/monitor_reconfigure_main.cc:149: //
> supported by the LCD.
> Great! I wanted to add this before, but my original assumption was that
> I should never change the resolution of the lcd (since the window
> manager couldn't resize when I first wrote this), so I never had to
> change the resolution back if disconnected.
>
> http://codereview.chromium.org/1646010/diff/1/2#newcode166
> src/platform/monitor_reconfig/monitor_reconfigure_main.cc:166: // TODO:
> Maybe avoid setting the screen res; xrandr should choose it for us.
> I believe the issue was it didn't always set it so this was a safety
> push
>
> http://codereview.chromium.org/1646010/diff/1/3
> File src/platform/monitor_reconfig/monitor_reconfigure_main.h (right):
>
> http://codereview.chromium.org/1646010/diff/1/3#newcode54
> src/platform/monitor_reconfig/monitor_reconfigure_main.h:54: bool
> FindNearestResolutions(
> Probably smaller vs. larger is more clear worse/better, but I agree,
> when I was first doing it myself, I had some bad names because it was
> hard to find good names when doing this comparison and keep it generic.
>
> On 2010/04/16 01:23:38, Daniel Erat wrote:
>>
>> Chooses the highest resolution in |worse_device_modes| and then finds
>
> the lowest
>>
>> resolution in |better_device_modes| that's larger than it.
>
>> These variable names are pretty bad; let me know if you can come up
>
> with
>>
>> something better.  "better" and "worse" here refer to the highest
>
> resolution
>>
>> offered by the devices.
>
> http://codereview.chromium.org/1646010/show
>

[Daniel Erat, 2010-04-19 19:11:14]

> I always assumed I think that the LVDS1 had the worst
> resolutions, but with the 1366x768 resolution of the newer
> machines, and prob 1280x1024 res of the projector
> downstairs, the projector downstairs couldn't displays the
> last 74 pixels.

I think it was maybe a bit worse -- if it was like the two
projectors that I was testing on, its max is just 1024x768.

> Were you able to check in the changes to catch the xrandr
> resizes?  Otherwise, the issue downstairs will still
> happen since the default res. is 1366 and if it can't be
> re-sized will stay at 1366 even though xrandr has changed
> res res size of the outputs down to 1280x800, 1280x1024

Yep, that's in now.  I'm still not getting the xrandr 
events (no idea why -- I tried a lot of different things, 
and I believe I see the events when I trace the connection 
via xtrace, but I'm not seeing it in the WM's event loop), 
but I changed the WM to just watch for ConfigureNotify 
events stating that the root window has been resized.

> Also, about xrandr --fb ... I noted that you could set
> screen to a smaller size (with an error/warning), but it
> would still respect it ... i.e. X wouldn't let the mouse
> cursor move beyond the window and send the window manager
> the right pixels to size it too.

Okay, cool.  Maybe this is something that changed recently 
with an X server upgrade -- I think that I was seeing this 
work the way that you describe it at first, but not with a 
later build.

http://codereview.chromium.org/1646010/diff/1/2
File src/platform/monitor_reconfig/monitor_reconfigure_main.cc (right):

http://codereview.chromium.org/1646010/diff/1/2#newcode123
src/platform/monitor_reconfig/monitor_reconfigure_main.cc:123: // to hold all
the devices.
On 2010/04/16 17:39:19, sosa wrote:
> xrandr gives an error but still sets it in my experience (from the tool at
> least)

Ah, cool.  This matches what I think I was seeing at first, but I was thrown off
by the error message.

http://codereview.chromium.org/1646010/diff/1/2#newcode129
src/platform/monitor_reconfig/monitor_reconfigure_main.cc:129: return false;
On 2010/04/16 17:39:19, sosa wrote:
> Add a comment that this is only reached if the better is not in fact >= worse?

Sure, added a warning.  I think that this can also happen if we end up with
devices with weird aspect ratios -- (this is an exaggerated example, but) say
that the LCD maxes out at 1600x400 while the external display is 1024x768. 
We'll think that the LCD is the smaller device since it has fewer pixels (640000
vs. 786432), but then we'll choose 1600x400 and won't be able to find any
resolution from the external display that'll fit it.  I don't know how likely
this is to actually occur, so I figured I'd save it for another CL.

http://codereview.chromium.org/1646010/diff/1/2#newcode166
src/platform/monitor_reconfig/monitor_reconfigure_main.cc:166: // TODO: Maybe
avoid setting the screen res; xrandr should choose it for us.
On 2010/04/16 17:39:19, sosa wrote:
> I believe the issue was it didn't always set it so this was a safety push

Okay, removed the comment.

http://codereview.chromium.org/1646010/diff/1/3
File src/platform/monitor_reconfig/monitor_reconfigure_main.h (right):

http://codereview.chromium.org/1646010/diff/1/3#newcode49
src/platform/monitor_reconfig/monitor_reconfigure_main.h:49: bool
FindBestResolutions(
On 2010/04/16 01:23:38, Daniel Erat wrote:
> Had to catch the shuttle before I could add descriptions here, but I'll put
them
> in when I'm back in the office.

Done.

http://codereview.chromium.org/1646010/diff/1/3#newcode54
src/platform/monitor_reconfig/monitor_reconfigure_main.h:54: bool
FindNearestResolutions(
On 2010/04/16 17:39:19, sosa wrote:
> Probably smaller vs. larger is more clear worse/better, but I agree, when I
was
> first doing it myself, I had some bad names because it was hard to find good
> names when doing this comparison and keep it generic.

Thanks, smaller/larger are much better.  Updated.

[sosa, 2010-04-19 19:46:37]

lgtm!