Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
584533002:
Don't enable HDCP again if it is active (Closed)
DescriptionDon't enable HDCP again if it is active
BUG=412841
Committed: https://crrev.com/7bf0d74a828bed0730daa361fd25603535136c0d
Cr-Commit-Position: refs/heads/master@{#296421}
Patch Set 1 #
Total comments: 7
Patch Set 2 : s/now_/current_/ #
Total comments: 8
Patch Set 3 : revise according to comments #Patch Set 4 : add comment #
Messages
Total messages: 15 (3 generated)
kcwu@chromium.org changed reviewers: + derat@chromium.org
Please take a look, thanks.
derat@chromium.org changed reviewers: + dnicoara@chromium.org
+dnicoara https://codereview.chromium.org/584533002/diff/1/ui/display/chromeos/display_... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/584533002/diff/1/ui/display/chromeos/display_... ui/display/chromeos/display_configurator.cc:254: HDCPState now_state; nit: current_state https://codereview.chromium.org/584533002/diff/1/ui/display/chromeos/display_... ui/display/chromeos/display_configurator.cc:255: if (!native_display_delegate_->GetHDCPState(*it->display, &now_state)) does this trigger a slow probe of the display? https://codereview.chromium.org/584533002/diff/1/ui/display/chromeos/display_... ui/display/chromeos/display_configurator.cc:257: bool now_desired = (now_state != HDCP_STATE_UNDESIRED); nit: s/now/current/; "new" and "now" look too similar
https://codereview.chromium.org/584533002/diff/1/ui/display/chromeos/display_... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/584533002/diff/1/ui/display/chromeos/display_... ui/display/chromeos/display_configurator.cc:255: if (!native_display_delegate_->GetHDCPState(*it->display, &now_state)) On 2014/09/18 14:45:30, Daniel Erat wrote: > does this trigger a slow probe of the display? crbug.com/374465 tracked an issue where GetHDCP was slow. Though it should be fixed. A couple of follow-up questions: 1) Shouldn't the driver handle this correctly? I'm not particularly familiar with the API contract and documentation is sparse, but it seems that only certain drivers disable HDCP before setting the new state. 2) Reading the bug suggests that SetHDCP is called multiple times. Is it done in a polling manner? It has been suggested that the HDCP state shouldn't be polled. 3) Would it make sense to cache the HDCP state? It would be nice to expand the interface comments to be more explicit what the expected behavior of these functions should be.
https://codereview.chromium.org/584533002/diff/1/ui/display/chromeos/display_... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/584533002/diff/1/ui/display/chromeos/display_... ui/display/chromeos/display_configurator.cc:254: HDCPState now_state; On 2014/09/18 14:45:30, Daniel Erat wrote: > nit: current_state Done. https://codereview.chromium.org/584533002/diff/1/ui/display/chromeos/display_... ui/display/chromeos/display_configurator.cc:255: if (!native_display_delegate_->GetHDCPState(*it->display, &now_state)) On 2014/09/18 15:39:16, dnicoara wrote: > On 2014/09/18 14:45:30, Daniel Erat wrote: > > does this trigger a slow probe of the display? > > crbug.com/374465 tracked an issue where GetHDCP was slow. Though it should be > fixed. > > A couple of follow-up questions: > 1) Shouldn't the driver handle this correctly? I'm not particularly familiar > with the API contract and documentation is sparse, but it seems that only > certain drivers disable HDCP before setting the new state. The driver is fixed. See comment #2 in crbug/412841. I originally assume the driver can handle this, so I could bypass multiple enable operation. According the bug, maybe we need to prevent another driver's fault in the future. > 2) Reading the bug suggests that SetHDCP is called multiple times. Is it done in > a polling manner? It has been suggested that the HDCP state shouldn't be polled. Currently, there is no way to get notification of HDCP state change, so HDCP state need to be polled in case HDCP is disabled by other program for unknown reason. > 3) Would it make sense to cache the HDCP state? As the reason in 2), we may cache the HDCP state for few seconds for performance but should not cache longer in user side. > It would be nice to expand the interface comments to be more explicit what the > expected behavior of these functions should be. https://codereview.chromium.org/584533002/diff/1/ui/display/chromeos/display_... ui/display/chromeos/display_configurator.cc:257: bool now_desired = (now_state != HDCP_STATE_UNDESIRED); On 2014/09/18 14:45:30, Daniel Erat wrote: > nit: s/now/current/; "new" and "now" look too similar Done.
lgtm with nits https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... ui/display/chromeos/display_configurator_unittest.cc:1197: EXPECT_NE(kNoActions, log_->GetActionsAndClear()); i'd remove the EXPECT_NE() here; your test doesn't care about what this returns https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... ui/display/chromeos/display_configurator_unittest.cc:1209: // Don't enable again if HDCP is already active nit: add a trailing period to this sentence
Thank you for the explanations! lgtm https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... ui/display/chromeos/display_configurator.cc:255: if (!native_display_delegate_->GetHDCPState(*it->display, Would you mind adding a comment here to explain why this is necessary? I'm afraid someone will try to optimize this out in a refactor.
dnicoara@, could you take a look? thanks https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... ui/display/chromeos/display_configurator.cc:255: if (!native_display_delegate_->GetHDCPState(*it->display, On 2014/09/19 15:57:53, dnicoara wrote: > Would you mind adding a comment here to explain why this is necessary? I'm > afraid someone will try to optimize this out in a refactor. I am not sure what you mean. I added one more sentence in line 260. Please take a look. https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... ui/display/chromeos/display_configurator_unittest.cc:1197: EXPECT_NE(kNoActions, log_->GetActionsAndClear()); On 2014/09/19 15:45:55, Daniel Erat wrote: > i'd remove the EXPECT_NE() here; your test doesn't care about what this returns Done. https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... ui/display/chromeos/display_configurator_unittest.cc:1209: // Don't enable again if HDCP is already active On 2014/09/19 15:45:55, Daniel Erat wrote: > nit: add a trailing period to this sentence Done.
https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... ui/display/chromeos/display_configurator.cc:255: if (!native_display_delegate_->GetHDCPState(*it->display, On 2014/09/23 05:43:22, kcwu wrote: > On 2014/09/19 15:57:53, dnicoara wrote: > > Would you mind adding a comment here to explain why this is necessary? I'm > > afraid someone will try to optimize this out in a refactor. > > I am not sure what you mean. I added one more sentence in line 260. Please take > a look. Awesome! I'm thinking something along the lines "Need to poll the driver for updates since other applications may have updated the state". Something along the lines of your explanation in message #6.
https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/584533002/diff/20001/ui/display/chromeos/disp... ui/display/chromeos/display_configurator.cc:255: if (!native_display_delegate_->GetHDCPState(*it->display, On 2014/09/23 13:35:44, dnicoara wrote: > On 2014/09/23 05:43:22, kcwu wrote: > > On 2014/09/19 15:57:53, dnicoara wrote: > > > Would you mind adding a comment here to explain why this is necessary? I'm > > > afraid someone will try to optimize this out in a refactor. > > > > I am not sure what you mean. I added one more sentence in line 260. Please > take > > a look. > > Awesome! I'm thinking something along the lines "Need to poll the driver for > updates since other applications may have updated the state". Something along > the lines of your explanation in message #6. Done.
The CQ bit was checked by kcwu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/584533002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as a4e39eb88ebece45f4693a33909bf5d45545eb8d
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7bf0d74a828bed0730daa361fd25603535136c0d Cr-Commit-Position: refs/heads/master@{#296421} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
