Pepper API implementation for output protection.

chromeos/display/real_output_configurator_delegate.cc

Index: chromeos/display/real_output_configurator_delegate.cc
diff --git a/chromeos/display/real_output_configurator_delegate.cc b/chromeos/display/real_output_configurator_delegate.cc
index 6d0c78254112c5e678f0149aefc181cb6ebd448e..b6eb721bd8004ec855114cda879f824c81eb7511 100644
--- a/chromeos/display/real_output_configurator_delegate.cc
+++ b/chromeos/display/real_output_configurator_delegate.cc
@@ -30,6 +30,12 @@ const float kMmInInch = 25.4;
 const float kDpi96 = 96.0;
 const float kPixelsToMmScale = kMmInInch / kDpi96;
 
+// Prefixes of putput name

[ddorwin, 2013/09/12 04:22:13]

output

[kcwu, 2013/09/12 18:22:08]

Done.

+const char kOutputPortName_VGA[] = "VGA";
+const char kOutputPortName_HDMI[] = "HDMI";
+const char kOutputPortName_DVI[] = "DVI";
+const char kOutputPortName_DisplayPort[] = "DP";
+
 bool IsInternalOutput(const XRROutputInfo* output_info) {
   return IsInternalOutputName(std::string(output_info->name));
 }
@@ -370,6 +376,215 @@ RealOutputConfiguratorDelegate::InitOutputSnapshot(
   return output;
 }
 
+bool RealOutputConfiguratorDelegate::GetHDCPState(RROutput id,
+                                                  HDCPState* state) {
+  CHECK(screen_) << "Server not grabbed";

[DaleCurtis, 2013/09/12 01:23:13]

This will crash the browser process. Do you want to instead return false /
dcheck?

[kcwu, 2013/09/12 18:22:08]

All checks of screen_ in this file are using CHECK(). I just follow them.

+  unsigned char* values = NULL;
+  int actual_format = 0;
+  unsigned long nitems = 0;
+  unsigned long bytes_after = 0;
+  Atom actual_type = None;
+  int success = 0;
+  Atom prop = XInternAtom(display_, "Content Protection", False);
+
+  bool ok = true;
+  success = XRRGetOutputProperty(display_, id, prop, 0, 100, False,
+                                 False, AnyPropertyType, &actual_type,
+                                 &actual_format, &nitems, &bytes_after,
+                                 &values);
+  if (actual_type == None) {
+    LOG(ERROR) << "Property 'Content Protection' does not exist";
+    ok = false;
+  } else if (success == Success && actual_type == XA_ATOM &&
+             actual_format == 32 && nitems == 1) {
+    Atom value = reinterpret_cast<Atom*>(values)[0];
+    if (value == XInternAtom(display_, "Undesired", False)) {
+      *state = HDCP_State_Undesired;
+    } else if (value == XInternAtom(display_, "Desired", False)) {
+      *state = HDCP_State_Desired;
+    } else if (value == XInternAtom(display_, "Enabled", False)) {
+      *state = HDCP_State_Enabled;
+    } else {
+      LOG(ERROR) << "Unknown property value: " << value;
+      ok = false;
+    }
+  } else {
+    LOG(ERROR) << "XRRGetOutputProperty failed";
+    ok = false;
+  }
+  if (values)
+    XFree(values);
+
+  VLOG(3) << "HDCP state: " << ok << "," << *state;
+  return ok;
+}
+
+bool RealOutputConfiguratorDelegate::SetHDCPState(RROutput id,
+                                                  HDCPState state) {
+  CHECK(screen_) << "Server not grabbed";
+  Atom name = XInternAtom(display_, "Content Protection", False);
+  Atom value;
+  switch (state) {
+    case HDCP_State_Undesired:
+      value = XInternAtom(display_, "Undesired", False);
+      break;
+    case HDCP_State_Desired:
+      value = XInternAtom(display_, "Desired", False);
+      break;
+    default:
+      DCHECK(0) << "Invalid HDCP state: " << state;

[DaleCurtis, 2013/09/12 01:23:13]

NOTREACHED() ?

[kcwu, 2013/09/12 18:22:08]

Done.

+      return false;
+  }
+  unsigned char *data = reinterpret_cast<unsigned char*>(&value);
+  XRRChangeOutputProperty(display_, id, name, XA_ATOM, 32,
+                          PropModeReplace, data, 1);
+  return true;
+}
+
+bool DetermineLinkType(const XRROutputInfo* output_info,
+                       PP_OutputProtectionLinkType_Private* link_type) {
+  std::string name(output_info->name);
+  if (IsInternalOutput(output_info)) {
+    *link_type = PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_INTERNAL;
+  } else if (name.find(kOutputPortName_VGA) == 0) {
+    *link_type = PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_VGA;
+  } else if (name.find(kOutputPortName_HDMI) == 0) {
+    *link_type = PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_HDMI;
+  } else if (name.find(kOutputPortName_DisplayPort) == 0) {
+    *link_type = PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT;
+  } else {
+    DCHECK(0) << "Unknown link type: " << name;
+    return false;
+  }
+  return true;
+}
+
+bool RealOutputConfiguratorDelegate::GetOutputLinkTypes(
+    std::vector<PP_OutputProtectionLinkType_Private>* link_types) {
+  CHECK(screen_) << "Server not grabbed";
+  bool result = true;
+  for (int i = 0; i < screen_->noutput; ++i) {
+    RROutput this_id = screen_->outputs[i];
+    XRROutputInfo* output_info = XRRGetOutputInfo(display_, screen_, this_id);

[DaleCurtis, 2013/09/12 01:23:13]

How expensive is this query?  I ask since it'll be queried pretty frequently on
the UI thread.

As an example, we see delays in the hundreds of milliseconds when querying for
webcams, audio devices, etc. 

[kcwu, 2013/09/12 18:22:08]

This query should be very fast. I can measure the number tomorrow.

+    if (output_info == NULL) {
+      result = false;
+      break;
+    }
+    if (output_info->connection == RR_Connected) {

[dmichael (off chromium), 2013/09/11 18:22:45]

you could use "else if" instead of break. I find breaks a little harder to
reason about.

[kcwu, 2013/09/12 18:22:08]

These logic are refactored into separate function,
QueryOutputProtectionLinkType().

+      PP_OutputProtectionLinkType_Private type;
+      if (DetermineLinkType(output_info, &type))
+        link_types->push_back(type);
+      else
+        result = false;

[DaleCurtis, 2013/09/12 01:23:13]

Do you want to break here? Should link_types be cleared on failure?

[kcwu, 2013/09/12 18:22:08]

These logic are refactored into separate function,
QueryOutputProtectionLinkType().

I didn't define the value of link_types for the case of failure. Should I?

+    } else {
+      link_types->push_back(PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_NONE);
+    }
+    XRRFreeOutputInfo(output_info);
+  }
+  return result;
+}
+
+bool RealOutputConfiguratorDelegate::GetOutputLinkMask(

[ddorwin, 2013/09/12 04:22:13]

This is really a "Convert" function. Also, it could be a local function instead
of class member.

[kcwu, 2013/09/12 18:22:08]

These methods are restructured.

+    const std::vector<PP_OutputProtectionLinkType_Private>& link_types,
+    uint32_t* link_mask) {
+  *link_mask = 0;
+
+  for (std::vector<PP_OutputProtectionLinkType_Private>::const_iterator it =
+       link_types.begin(); it != link_types.end(); ++it) {
+    *link_mask |= *it;
+  }
+  return true;
+}
+
+bool RealOutputConfiguratorDelegate::GetProtectionMethods(

[ddorwin, 2013/09/12 04:22:13]

This function could easily return the link_mask, simplifying callers. (If you
don't want to always populate the mask, you could handle a NULL output
parameter.

[kcwu, 2013/09/12 18:22:08]

Done.

+    const std::vector<PP_OutputProtectionLinkType_Private>& link_types,
+    uint32_t* protection_mask) {
+  CHECK(screen_) << "Server not grabbed";
+  uint32_t enabled = 0;
+  uint32_t unfulfiled = 0;
+
+  DCHECK(screen_->noutput == static_cast<int>(link_types.size()));

[DaleCurtis, 2013/09/12 01:23:13]

DCHECK_EQ ?

[kcwu, 2013/09/12 18:22:08]

Done.

+  for (int i = 0; i < screen_->noutput; ++i) {
+    PP_OutputProtectionLinkType_Private type = link_types[i];
+    if ((type & PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_HDMI) ||
+        (type & PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT)) {
+      HDCPState state;
+      RROutput this_id = screen_->outputs[i];
+      if (!GetHDCPState(this_id, &state))
+        return false;
+      if (state == HDCP_State_Enabled)
+        enabled |= PP_OUTPUT_PROTECTION_METHOD_PRIVATE_HDCP;
+      else
+        unfulfiled |= PP_OUTPUT_PROTECTION_METHOD_PRIVATE_HDCP;
+    }
+  }
+  *protection_mask = enabled & ~unfulfiled;
+  return true;
+}
+
+uint32_t RealOutputConfiguratorDelegate::GetCombinedProtectionRequest() {
+  uint32_t combined_requirement = 0;
+  for (ProtectionRequests::const_iterator it =
+           client_protection_request_.begin();
+       it != client_protection_request_.end();
+       ++it) {
+    combined_requirement |= it->second;
+  }
+  return combined_requirement;
+}
+
+bool RealOutputConfiguratorDelegate::EnableOutputProtection(
+    const chrome::PepperOutputProtectionHost* client,
+    uint32_t desired_method_mask) {
+  if (desired_method_mask == PP_OUTPUT_PROTECTION_METHOD_PRIVATE_NONE)
+    client_protection_request_.erase(client);
+  else
+    client_protection_request_[client] = desired_method_mask;
+
+  uint32_t all_desires = GetCombinedProtectionRequest();
+  std::vector<PP_OutputProtectionLinkType_Private> link_types;
+  if (!GetOutputLinkTypes(&link_types))

[ddorwin, 2013/09/12 04:22:13]

It's odd that we iterate through the outputs to get this vector then iterate
through the outputs again.
(Also, in the meantime, the values of outputs could change.)

It might be better to extract 467-482 into a function that gets the link type
for a single output. That could be called for each iteration of 549.

[kcwu, 2013/09/12 18:22:08]

Done.
Per this suggestion. These methods are refactored.

+    return false;
+  bool ok = true;
+  for (int i = 0; i < screen_->noutput; ++i) {
+    RROutput this_id = screen_->outputs[i];
+    PP_OutputProtectionLinkType_Private type = link_types[i];
+    if ((type & PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_HDMI) ||
+        (type & PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT)) {
+      if (all_desires & PP_OUTPUT_PROTECTION_METHOD_PRIVATE_HDCP) {
+        ok = SetHDCPState(this_id, HDCP_State_Desired);
+      } else {
+        ok = SetHDCPState(this_id, HDCP_State_Undesired);
+      }
+    }
+
+    if (!ok)

[ddorwin, 2013/09/12 04:22:13]

"&& ok" could just be a condition in the for loop then you'd return ok below.

[kcwu, 2013/09/12 18:22:08]

Done.

+      return false;
+  }
+  return true;
+}
+
+bool RealOutputConfiguratorDelegate::QueryOutputProtectionStatus(
+    const chrome::PepperOutputProtectionHost* client,
+    uint32_t* link_mask,
+    uint32_t* protection_mask) {
+  std::vector<PP_OutputProtectionLinkType_Private> link_types;
+  if (!GetOutputLinkTypes(&link_types))
+    return false;
+
+  bool ok = GetProtectionMethods(link_types, protection_mask) &&
+      GetOutputLinkMask(link_types, link_mask);
+  ProtectionRequests::iterator it = client_protection_request_.find(client);

[ddorwin, 2013/09/12 04:22:13]

This should be with the code below (separated by an empty line).

[kcwu, 2013/09/12 18:22:08]

Done.

+
+  // Don't reveal protections requested by other clients.
+  if (it != client_protection_request_.end()) {
+    *protection_mask &= it->second;

[ddorwin, 2013/09/12 04:22:13]

It might help to comment or assign |second| to a const ref. I assume this is the
requested_mask.

[kcwu, 2013/09/12 18:22:08]

Done.

+  } else {
+    *protection_mask = 0;

[ddorwin, 2013/09/12 04:22:13]

Should this be a NOTREACHED() or can it actually happen?

[kcwu, 2013/09/12 18:22:08]

This happens if the client Enable(none) and QueryStatus().

+  }
+  return ok;
+}
+
 void RealOutputConfiguratorDelegate::DestroyUnusedCrtcs(
     const std::vector<OutputConfigurator::OutputSnapshot>& outputs) {
   CHECK(screen_) << "Server not grabbed";