Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
120223003:
Support failing modeset (Closed)
|
Created:
7 years ago by dsodman Modified:
6 years, 11 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport failing modeset
Add support to gracefully handle the case of a modeset failing by trying
the next lower resolution
BUG=309720
TEST=Test with external HDMI monitor and Display Settings
R=oshima@chromium.org, marcheu@chromium.org
Signed-off-by: David Sodman <dsodman@chromium.org>;
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244030
Patch Set 1 #
Total comments: 5
Patch Set 2 : Update based on review feedback #
Total comments: 43
Patch Set 3 : Update unittest and response to code review #
Total comments: 37
Patch Set 4 : Review feedback #
Total comments: 4
Patch Set 5 : Final rev #
Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/120223003/diff/1/chromeos/display/output_conf... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/120223003/diff/1/chromeos/display/output_conf... chromeos/display/output_configurator.cc:971: } If we are in mirror mode, and modeset fails on one of the monitors, we will end up with two mismatched modes. I'm not sure what the effect will be, but most likely not what we want. We should probably fix that before landing this change. To fix it, it might be sufficient to return false in this case.
please add tests for any changes you make to this file to output_configurator_unittest.cc https://codereview.chromium.org/120223003/diff/1/chromeos/display/output_conf... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/120223003/diff/1/chromeos/display/output_conf... chromeos/display/output_configurator.cc:946: for (size_t i = 0; i < updated_outputs.size(); ) { please structure this code in a more logical way: for (size_t i = 0; i < updated_outputs.size(); ++i) { ... while (!delegate_->ConfigureCrtc(...)) { updated_outputs[i].current_mode = ... // break when you run out of modes to try } } https://codereview.chromium.org/120223003/diff/1/chromeos/display/output_conf... chromeos/display/output_configurator.cc:962: output.mode_infos.find(output.current_mode)->second; indent 4 spaces, not 7 https://codereview.chromium.org/120223003/diff/1/chromeos/display/output_conf... chromeos/display/output_configurator.cc:963: int pixels = mode_info.width * mode_info.height; give this a better name like current_mode_pixels https://codereview.chromium.org/120223003/diff/1/chromeos/display/output_conf... chromeos/display/output_configurator.cc:974: continue; indent 2 spaces, not 0
On 2013/12/21 01:56:15, marcheu wrote: > https://codereview.chromium.org/120223003/diff/1/chromeos/display/output_conf... > File chromeos/display/output_configurator.cc (right): > > https://codereview.chromium.org/120223003/diff/1/chromeos/display/output_conf... > chromeos/display/output_configurator.cc:971: } > If we are in mirror mode, and modeset fails on one of the monitors, we will end > up with two mismatched modes. I'm not sure what the effect will be, but most > likely not what we want. We should probably fix that before landing this change. > > To fix it, it might be sufficient to return false in this case. Thanks for the feedback. I agree that returning false in this case should be sufficient to handle the error case gracefully. I've updated to do that.
Thanks for the review feedback. I've updated to address each of the points. Please review and let me know if there are further concerns.
https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:956: int best_mode_pixels = 0; nit: add a comment before this line summarizing what the loop does, like: // Find the mode with the next-highest resolution. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:958: output.mode_infos.find(output.current_mode)->second; nit: do this instead? const ModeInfo* mode_info = GetModeInfo(output.current_mode); if (!mode_info) break; https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:966: best_mode_pixels = current_mode_pixels; shouldn't this be: best_mode_pixels = pixel_count; ? if that's correct, please make sure you have a test that would've caught this. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:974: // if we are trying to set mirror mode and one of the modeset's fails, nit: s/if/If/ https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:976: // false to let the observer's be aware nit: s/observer's/observers/, also add trailing period https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:977: if (output_state == STATE_DUAL_MIRROR) style guide requires curly brackets on the outermost if statement here (since its statement is more than a single line), but it would be better to just do: if (output_state == STATE_DUAL_MIRROR && output_power[i] && output.current_mode != output.mirror_mode) return false; https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:979: return false; shouldn't you only return after you've finished iterating through all of the outputs? please make sure you have a test for the case where configuring the first output fails to make sure that the second output still gets configured. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:982: delegate_->ConfigureCTM(output.touch_device_id, output.transform); shouldn't this come before the previous return statement? otherwise it seems like you could end up with touch device settings that don't match the current mode. please add a test for this too. (this comment is perhaps irrelevant if the previous return statement gets moved outside of the loop as suggested above.) https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:983: cached_outputs_[i] = updated_outputs[i]; i think that this also needs to happen before the previous return statement; otherwise the cached state won't match reality. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:107: : max_configurable_pixels_(kMaxTestWidth * kMaxTestHeight), please make this default to 0 and document that 0 values will result in no limit being enforced. get rid of the above kMaxTest* constants, and make tests that want to trigger failures instead just pass their own reasonable values to set_max_configurable_pixels(). https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:119: void set_max_configurable_pixels(long pixels) { why long? int seems like it should be big enough, but please use either int64 or uint64 if you need a specific size. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:159: outputs_[output-1].mode_infos[mode] = OutputConfigurator::ModeInfo( please don't add an undocumented assumption that output #n will be at index n-1. those just happen to be defaults used by SetUp(). you need to iterate through |outputs_| to find the correct output instead. there should probably be a private method for this since ConfigureCrtc() needs it too: OutputSnapshot* GetOutput(RROutput output); https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:235: // Simulate pixel-clk limitations nit: avoid abbreviations ("pixel clock"?) also describe what this actually does. is something like the following correct? // Attempts to use a mode containing more pixels than this will fail. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1089: outputs_[0].mirror_mode = kBigModeId; OutputConfigurator is responsible for choosing the mirror mode and updating this member accordingly -- if you need to manually assign it now to get an existing test to pass, that seems like a sign that something is broken. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1283: OutputConfigurator::OutputSnapshot *output; nit: '*' goes on left side of space https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1288: nit: delete extra blank line https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1291: // when the test is done nit: add trailing period https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1297: output->mode_infos[11]=OutputConfigurator::ModeInfo(2560, 1600, false, 60.0); there should be spaces on either side of the equals sign, even if it makes the line wrap. it would probably be better to just add a helper method to the base test class, though: RRMode CreateMode(OutputConfigurator::OutputSnapshot* output, int width, int height, bool interlaced, float refresh_rate); https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1314: configurator_.Init(false); don't call Init() multiple times; the later call to UpdateOutputs() should be sufficient. the multiple calls to UpdateOutputs() seem wrong too. the general pattern should be along these lines: // update outputs_ as desired ... // copy the outputs to the delegator and notify the configurator UpdateOutputs(1, true); // now check that the configurator performed the correct actions EXPECT_EQ(..., delegate_->GetActionsAndClear()); doing it this way exercises the code in the same way that it's used in the real world. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1319: outputs_[0].output, outputs_[0].crtc, outputs_[0].selected_mode, true); UpdateOutputs() should already do this https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1333: configurator_.Init(false); remove this too https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1356: outputs_[0] = backup_output; you shouldn't need to restore anything at the end of a test; the test harness is torn down and rebuilt for each test
Thanks for the detailed feedback. I've tried to respond to each comment. Let me know if there are further issues/concerns. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:956: int best_mode_pixels = 0; On 2014/01/06 17:15:29, Daniel Erat wrote: > nit: add a comment before this line summarizing what the loop does, like: > > // Find the mode with the next-highest resolution. Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:958: output.mode_infos.find(output.current_mode)->second; On 2014/01/06 17:15:29, Daniel Erat wrote: > nit: do this instead? > > const ModeInfo* mode_info = GetModeInfo(output.current_mode); > if (!mode_info) > break; Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:966: best_mode_pixels = current_mode_pixels; On 2014/01/06 17:15:29, Daniel Erat wrote: > shouldn't this be: > > best_mode_pixels = pixel_count; > > ? if that's correct, please make sure you have a test that would've caught this. Thanks for catching this. You are right. I also updated tests to make sure that the test would have caught it. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:974: // if we are trying to set mirror mode and one of the modeset's fails, On 2014/01/06 17:15:29, Daniel Erat wrote: > nit: s/if/If/ Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:976: // false to let the observer's be aware On 2014/01/06 17:15:29, Daniel Erat wrote: > nit: s/observer's/observers/, also add trailing period Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:977: if (output_state == STATE_DUAL_MIRROR) On 2014/01/06 17:15:29, Daniel Erat wrote: > style guide requires curly brackets on the outermost if statement here (since > its statement is more than a single line), but it would be better to just do: > > if (output_state == STATE_DUAL_MIRROR && > output_power[i] && > output.current_mode != output.mirror_mode) > return false; Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:979: return false; On 2014/01/06 17:15:29, Daniel Erat wrote: > shouldn't you only return after you've finished iterating through all of the > outputs? please make sure you have a test for the case where configuring the > first output fails to make sure that the second output still gets configured. Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator.cc:983: cached_outputs_[i] = updated_outputs[i]; On 2014/01/06 17:15:29, Daniel Erat wrote: > i think that this also needs to happen before the previous return statement; > otherwise the cached state won't match reality. Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:107: : max_configurable_pixels_(kMaxTestWidth * kMaxTestHeight), On 2014/01/06 17:15:29, Daniel Erat wrote: > please make this default to 0 and document that 0 values will result in no limit > being enforced. get rid of the above kMaxTest* constants, and make tests that > want to trigger failures instead just pass their own reasonable values to > set_max_configurable_pixels(). Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:107: : max_configurable_pixels_(kMaxTestWidth * kMaxTestHeight), On 2014/01/06 17:15:29, Daniel Erat wrote: > please make this default to 0 and document that 0 values will result in no limit > being enforced. get rid of the above kMaxTest* constants, and make tests that > want to trigger failures instead just pass their own reasonable values to > set_max_configurable_pixels(). Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:119: void set_max_configurable_pixels(long pixels) { On 2014/01/06 17:15:29, Daniel Erat wrote: > why long? int seems like it should be big enough, but please use either int64 or > uint64 if you need a specific size. Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:159: outputs_[output-1].mode_infos[mode] = OutputConfigurator::ModeInfo( On 2014/01/06 17:15:29, Daniel Erat wrote: > please don't add an undocumented assumption that output #n will be at index n-1. > those just happen to be defaults used by SetUp(). you need to iterate through > |outputs_| to find the correct output instead. there should probably be a > private method for this since ConfigureCrtc() needs it too: > > OutputSnapshot* GetOutput(RROutput output); Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:235: // Simulate pixel-clk limitations On 2014/01/06 17:15:29, Daniel Erat wrote: > nit: avoid abbreviations ("pixel clock"?) > > also describe what this actually does. is something like the following correct? > > // Attempts to use a mode containing more pixels than this will fail. Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1089: outputs_[0].mirror_mode = kBigModeId; On 2014/01/06 17:15:29, Daniel Erat wrote: > OutputConfigurator is responsible for choosing the mirror mode and updating this > member accordingly -- if you need to manually assign it now to get an existing > test to pass, that seems like a sign that something is broken. Thanks for the explanation. This is indeed not necessary. Might have been an artifact of an early version where I did have something wrong. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1283: OutputConfigurator::OutputSnapshot *output; On 2014/01/06 17:15:29, Daniel Erat wrote: > nit: '*' goes on left side of space Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1288: On 2014/01/06 17:15:29, Daniel Erat wrote: > nit: delete extra blank line Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1291: // when the test is done On 2014/01/06 17:15:29, Daniel Erat wrote: > nit: add trailing period Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1297: output->mode_infos[11]=OutputConfigurator::ModeInfo(2560, 1600, false, 60.0); On 2014/01/06 17:15:29, Daniel Erat wrote: > there should be spaces on either side of the equals sign, even if it makes the > line wrap. it would probably be better to just add a helper method to the base > test class, though: > > RRMode CreateMode(OutputConfigurator::OutputSnapshot* output, > int width, > int height, > bool interlaced, > float refresh_rate); Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1314: configurator_.Init(false); On 2014/01/06 17:15:29, Daniel Erat wrote: > don't call Init() multiple times; the later call to UpdateOutputs() should be > sufficient. the multiple calls to UpdateOutputs() seem wrong too. the general > pattern should be along these lines: > > // update outputs_ as desired > ... > // copy the outputs to the delegator and notify the configurator > UpdateOutputs(1, true); > // now check that the configurator performed the correct actions > EXPECT_EQ(..., delegate_->GetActionsAndClear()); > > doing it this way exercises the code in the same way that it's used in the real > world. Thanks for the explanation. I've tried to update it to model it how it should be. Let me know if there is still something wrong. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1333: configurator_.Init(false); On 2014/01/06 17:15:29, Daniel Erat wrote: > remove this too Done. https://codereview.chromium.org/120223003/diff/230001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1356: outputs_[0] = backup_output; On 2014/01/06 17:15:29, Daniel Erat wrote: > you shouldn't need to restore anything at the end of a test; the test harness is > torn down and rebuilt for each test Done.
https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator.cc:815: bool status; move this down to the point where you need it, and maybe rename it to be more descriptive (i.e. do "bool all_succeeded = true" on line 945) https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator.cc:967: it != output.mode_infos.end(); it++) { nit: indent one more space https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator.cc:980: // If we are trying to set mirror mode and one of the modeset's fails, nit: s/modeset's/modesets/ https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator.cc:991: cached_outputs_[i] = updated_outputs[i]; ConfigureCTM() should only be run and cached_outputs_[i] should only be updated if the call to ConfigureCrtc() succeeded, right? i'm wondering if you need something structured more like this (switching to "while (true)" since it arguably simplifies the code a bit): bool configure_succeeded = false; while (true) { if (delegate_->ConfigureCrtc(...)) { configure_suceeded = true; break; } ... if (!mode_info) break; ... if (best_mode_pixels == 0) break; } if (configure_succeeded) { if (output.touch_device_id) delegate_->ConfigureCTM(...); cached_outputs_[i] = updated_outputs[i]; } else { all_succeeded = false; } https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator.cc:997: return status; i'm also not sure whether output_state_ and power_state_ should be updated unconditionally here, or only if all of the configures were successful. whether they're left unchanged or updated determines whether a later call setting the same output state or power state will run or be ignored, i believe, so it might be better to leave them unchanged if that means that we'll at least try to reconfigure everything again later. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:122: // return success regardless of the resolution. nit: merge all of this documentation with the other comment that appears at the point where the variable is defined (i.e. getters and setters usually don't have any comments of their own) https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:165: if (snapshot != NULL) { nit: if (!snapshot) return; (or maybe just CHECK(snapshot) instead? if OutputConfigurator tries to add a mode to a bogus output, that seems like a reasonable thing to crash the test about in a controlled way) https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:167: *OutputConfigurator::GetModeInfo(*snapshot, mode); probably also want to check GetModeInfo()'s return value here so that an invalid mode won't make the test segfault: OutputConfigurator::OutputSnapshot::ModeInfo* mode_info = OutputConfigurator::GetModeInfo(*snapshot, mode); if (mode_info) snapshot->mode_infos[mode] = mode_info; https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:177: // |max_configurable_pixels_| value of 0 means no limit on resolution nit: don't need this comment https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:178: if (max_configurable_pixels_ == 0) { nit: don't need curly brackets for an 'if' with a single-line statement https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:183: if (snapshot) { nit: if (!snapshot) return false; const OutputConfigurator... (return early to avoid longer indented blocks and to keep the "return false" error-handling close to the error condition) https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1303: int current_mode; nit: move this down to the place where it's first used (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...) https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1305: test_api_.SendScreenChangeEvent(); i don't think you need this or the kUpdateXRandR check (the initial config gets loaded by the call to Start() in InitWithSingleOutput(), and nothing's changed since then) https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1315: for (int i = 0; i < 2; i++) { nit: s/2/outputs_.size()/ https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1330: outputs_[i].selected_mode = kFirstMode; s/selected_mode/current_mode/ ? (selected_mode gets set by OutputConfigurator -- OutputSnapshots usually start out with only current_mode and native_mode set; see RealOutputConfiguratorDelegate::InitOutputSnapshot() in chromeos/display/real_output_configurator_delegate.cc) https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1334: current_mode = kFirstMode; this doesn't seem to do anything (i don't see it used later) https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1344: test_api_.SendOutputChangeEvent( i don't think you need this call either; the next call to UpdateOutputs() should send an event (since its |send_events| argument is set) https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1366: test_api_.SendOutputChangeEvent( same here
Again, appreciate the detailed feedback. Let me know if there is anything else I should change https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator.cc:815: bool status; On 2014/01/08 21:40:28, Daniel Erat wrote: > move this down to the point where you need it, and maybe rename it to be more > descriptive (i.e. do "bool all_succeeded = true" on line 945) Done. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator.cc:967: it != output.mode_infos.end(); it++) { On 2014/01/08 21:40:28, Daniel Erat wrote: > nit: indent one more space Done. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator.cc:980: // If we are trying to set mirror mode and one of the modeset's fails, On 2014/01/08 21:40:28, Daniel Erat wrote: > nit: s/modeset's/modesets/ Done. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator.cc:991: cached_outputs_[i] = updated_outputs[i]; On 2014/01/08 21:40:28, Daniel Erat wrote: > ConfigureCTM() should only be run and cached_outputs_[i] should only be updated > if the call to ConfigureCrtc() succeeded, right? > > i'm wondering if you need something structured more like this (switching to > "while (true)" since it arguably simplifies the code a bit): > > bool configure_succeeded = false; > while (true) { > if (delegate_->ConfigureCrtc(...)) { > configure_suceeded = true; > break; > } > ... > if (!mode_info) > break; > ... > if (best_mode_pixels == 0) > break; > } > > if (configure_succeeded) { > if (output.touch_device_id) > delegate_->ConfigureCTM(...); > cached_outputs_[i] = updated_outputs[i]; > } else { > all_succeeded = false; > } To be honest, I am not entirely sure what is the impact of calling ConfigureCTM(...) when ConfigureCrtc has failed. I will take your word for it that it would lead to an inconsistency. I modified as you suggested, but, it causes a new failure in AvoidUnnecessaryProbes. The reason is that prior to this revision (and prior to my change altogether), EnterState returns true when ConfigureCrtc fails. With this change, it is returning false and EnterStateOrFallbackToSoftwareMirroring is calling EnterState again with EXTENDED mode. It seems to me that indicates that the expected output of the AvoidUnnessecaryProbes needs to change, so, I updated it, but, you should tell me if that is correct or if this has introduced a bug. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator.cc:997: return status; On 2014/01/08 21:40:28, Daniel Erat wrote: > i'm also not sure whether output_state_ and power_state_ should be updated > unconditionally here, or only if all of the configures were successful. whether > they're left unchanged or updated determines whether a later call setting the > same output state or power state will run or be ignored, i believe, so it might > be better to leave them unchanged if that means that we'll at least try to > reconfigure everything again later. Done. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:122: // return success regardless of the resolution. On 2014/01/08 21:40:28, Daniel Erat wrote: > nit: merge all of this documentation with the other comment that appears at the > point where the variable is defined (i.e. getters and setters usually don't have > any comments of their own) Done. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:165: if (snapshot != NULL) { On 2014/01/08 21:40:28, Daniel Erat wrote: > nit: > > if (!snapshot) > return; > > (or maybe just CHECK(snapshot) instead? if OutputConfigurator tries to add a > mode to a bogus output, that seems like a reasonable thing to crash the test > about in a controlled way) I did this and also did the same with mode_info (used CHECK(mode_info) but that caused PanelFitting test to fail. So, I realized that this code added in AddOutputMode should not have been added to begin with. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:177: // |max_configurable_pixels_| value of 0 means no limit on resolution On 2014/01/08 21:40:28, Daniel Erat wrote: > nit: don't need this comment Done. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:178: if (max_configurable_pixels_ == 0) { On 2014/01/08 21:40:28, Daniel Erat wrote: > nit: don't need curly brackets for an 'if' with a single-line statement Done. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:183: if (snapshot) { On 2014/01/08 21:40:28, Daniel Erat wrote: > nit: > > if (!snapshot) > return false; > > const OutputConfigurator... > > (return early to avoid longer indented blocks and to keep the "return false" > error-handling close to the error condition) Done. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1303: int current_mode; On 2014/01/08 21:40:28, Daniel Erat wrote: > nit: move this down to the place where it's first used > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...) Done. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1305: test_api_.SendScreenChangeEvent(); On 2014/01/08 21:40:28, Daniel Erat wrote: > i don't think you need this or the kUpdateXRandR check (the initial config gets > loaded by the call to Start() in InitWithSingleOutput(), and nothing's changed > since then) That's fine. I had modeled this after the AvoidUnnessecaryProbes which does the same thing. I have removed. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1315: for (int i = 0; i < 2; i++) { On 2014/01/08 21:40:28, Daniel Erat wrote: > nit: s/2/outputs_.size()/ Here outputs_ is an array not a vector: OutputConfigurator::OutputSnapshot outputs_[2]; Do you want me to change it? https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1330: outputs_[i].selected_mode = kFirstMode; On 2014/01/08 21:40:28, Daniel Erat wrote: > s/selected_mode/current_mode/ ? > > (selected_mode gets set by OutputConfigurator -- OutputSnapshots usually start > out with only current_mode and native_mode set; see > RealOutputConfiguratorDelegate::InitOutputSnapshot() in > chromeos/display/real_output_configurator_delegate.cc) Done. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1334: current_mode = kFirstMode; On 2014/01/08 21:40:28, Daniel Erat wrote: > this doesn't seem to do anything (i don't see it used later) Done. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1344: test_api_.SendOutputChangeEvent( On 2014/01/08 21:40:28, Daniel Erat wrote: > i don't think you need this call either; the next call to UpdateOutputs() should > send an event (since its |send_events| argument is set) I removed. I had modeled it after AvoidUnnecessaryProbes https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1366: test_api_.SendOutputChangeEvent( On 2014/01/08 21:40:28, Daniel Erat wrote: > same here Done.
Thanks! LGTM with a few nits; cc-ing Sadrul for the CTM question. https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator.cc:991: cached_outputs_[i] = updated_outputs[i]; On 2014/01/09 00:26:28, dsodman wrote: > On 2014/01/08 21:40:28, Daniel Erat wrote: > > ConfigureCTM() should only be run and cached_outputs_[i] should only be > updated > > if the call to ConfigureCrtc() succeeded, right? > > > > i'm wondering if you need something structured more like this (switching to > > "while (true)" since it arguably simplifies the code a bit): > > > > bool configure_succeeded = false; > > while (true) { > > if (delegate_->ConfigureCrtc(...)) { > > configure_suceeded = true; > > break; > > } > > ... > > if (!mode_info) > > break; > > ... > > if (best_mode_pixels == 0) > > break; > > } > > > > if (configure_succeeded) { > > if (output.touch_device_id) > > delegate_->ConfigureCTM(...); > > cached_outputs_[i] = updated_outputs[i]; > > } else { > > all_succeeded = false; > > } > > To be honest, I am not entirely sure what is the impact of calling > ConfigureCTM(...) when ConfigureCrtc has failed. I will take your word for it > that it would lead to an inconsistency. I modified as you suggested, but, it > causes a new failure in AvoidUnnecessaryProbes. The reason is that prior to > this revision (and prior to my change altogether), EnterState returns true when > ConfigureCrtc fails. With this change, it is returning false and > EnterStateOrFallbackToSoftwareMirroring is calling EnterState again with > EXTENDED mode. It seems to me that indicates that the expected output of the > AvoidUnnessecaryProbes needs to change, so, I updated it, but, you should tell > me if that is correct or if this has introduced a bug. i'm not an expert on the touch code, but i believe that this matrix transforms touch events ("Coordinate Transformation Matrix"; see RealOutputConfiguratorDelegate::ConfigureCTM()), so it ought to match whatever mode is being used for the output. maybe sadrul@ can confirm... https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1315: for (int i = 0; i < 2; i++) { On 2014/01/09 00:26:28, dsodman wrote: > On 2014/01/08 21:40:28, Daniel Erat wrote: > > nit: s/2/outputs_.size()/ > > Here outputs_ is an array not a vector: > > OutputConfigurator::OutputSnapshot outputs_[2]; > > Do you want me to change it? ah, i think that arraysize(outputs_) should work, in that case. https://codereview.chromium.org/120223003/diff/540001/chromeos/display/output... File chromeos/display/output_configurator.cc (right): https://codereview.chromium.org/120223003/diff/540001/chromeos/display/output... chromeos/display/output_configurator.cc:952: configure_succeeded = false; nit: just initialize the variable to false where it's defined outside of the loop; that way you can set it to true when you break out of the loop on success but not need to set it to false in the multiple places where you break out of the loop on failure https://codereview.chromium.org/120223003/diff/540001/chromeos/display/output... File chromeos/display/output_configurator_unittest.cc (right): https://codereview.chromium.org/120223003/diff/540001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:169: nit: remove this blank line https://codereview.chromium.org/120223003/diff/540001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:175: nit: remove this blank line https://codereview.chromium.org/120223003/diff/540001/chromeos/display/output... chromeos/display/output_configurator_unittest.cc:1315: int current_mode = kFirstMode; nit: move this definition down to the place where it's used in the for loop now
On 2014/01/09 00:40:29, Daniel Erat wrote: > Thanks! LGTM with a few nits; cc-ing Sadrul for the CTM question. > > https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... > File chromeos/display/output_configurator.cc (right): > > https://codereview.chromium.org/120223003/diff/430001/chromeos/display/output... > chromeos/display/output_configurator.cc:991: cached_outputs_[i] = > updated_outputs[i]; > On 2014/01/09 00:26:28, dsodman wrote: > > On 2014/01/08 21:40:28, Daniel Erat wrote: > > > ConfigureCTM() should only be run and cached_outputs_[i] should only be > > updated > > > if the call to ConfigureCrtc() succeeded, right? > > > > > > i'm wondering if you need something structured more like this (switching to > > > "while (true)" since it arguably simplifies the code a bit): > > > > > > bool configure_succeeded = false; > > > while (true) { > > > if (delegate_->ConfigureCrtc(...)) { > > > configure_suceeded = true; > > > break; > > > } > > > ... > > > if (!mode_info) > > > break; > > > ... > > > if (best_mode_pixels == 0) > > > break; > > > } > > > > > > if (configure_succeeded) { > > > if (output.touch_device_id) > > > delegate_->ConfigureCTM(...); > > > cached_outputs_[i] = updated_outputs[i]; > > > } else { > > > all_succeeded = false; > > > } > > > > To be honest, I am not entirely sure what is the impact of calling > > ConfigureCTM(...) when ConfigureCrtc has failed. I will take your word for it > > that it would lead to an inconsistency. I modified as you suggested, but, it > > causes a new failure in AvoidUnnecessaryProbes. The reason is that prior to > > this revision (and prior to my change altogether), EnterState returns true > when > > ConfigureCrtc fails. With this change, it is returning false and > > EnterStateOrFallbackToSoftwareMirroring is calling EnterState again with > > EXTENDED mode. It seems to me that indicates that the expected output of the > > AvoidUnnessecaryProbes needs to change, so, I updated it, but, you should tell > > me if that is correct or if this has introduced a bug. > > i'm not an expert on the touch code, but i believe that this matrix transforms > touch events ("Coordinate Transformation Matrix"; see > RealOutputConfiguratorDelegate::ConfigureCTM()), so it ought to match whatever > mode is being used for the output. maybe sadrul@ can confirm... Yep, sounds right. LG
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dsodman@chromium.org/120223003/660001
Message was sent while issue was closed.
Change committed as 244030 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
