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..3313f0349e8eb7ba95e8a409614682f57b8255cb 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
+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,212 @@ RealOutputConfiguratorDelegate::InitOutputSnapshot(
   return output;
 }
 
+bool RealOutputConfiguratorDelegate::GetHDCPState(RROutput id,
+                                                  HDCPState* state) {
+  CHECK(screen_) << "Server not grabbed";
+  unsigned char* values = NULL;
+  int actual_format;
+  unsigned long nitems;
+  unsigned long bytes_after;
+  Atom actual_type;
+  int success;

[dmichael (off chromium), 2013/09/09 20:37:15]

please initialize these (same for other variables below in your patch)

[kcwu, 2013/09/10 12:50:21]

Done.

+  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);
+  unsigned long value;
+  switch (state) {
+    case HDCP_State_Undesired:
+      value = XInternAtom(display_, "Undesired", False);

[dmichael (off chromium), 2013/09/09 20:37:15]

It looks like this function returns type "Atom"...  why are you using unsigned
long?

[kcwu, 2013/09/10 12:50:21]

Done.

+      break;
+    case HDCP_State_Desired:
+      value = XInternAtom(display_, "Desired", False);
+      break;
+    default:
+      DCHECK(0) << "Invalid HDCP state: " << state;
+      return false;
+  }
+  unsigned char *data = (unsigned char*)&value;

[dmichael (off chromium), 2013/09/09 20:37:15]

Please do not use C-style casts. I think you want reinterpret_cast.

[kcwu, 2013/09/10 12:50:21]

Done.

[kcwu, 2013/09/10 12:50:21]

Done.

+  XRRChangeOutputProperty(display_, id, name, XA_ATOM, 32,
+                          PropModeReplace, data, 1);
+  return true;
+}
+
+bool DetermineLinkType(const XRROutputInfo* output_info,

[dmichael (off chromium), 2013/09/09 20:37:15]

const-reference?

[kcwu, 2013/09/10 12:50:21]

Existing code, IsInternalOutput() takes const-pointer. Should I use
const-reference?

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

Yes. It looks like IsInternalOutput assumes that the pointer is valid, so const&
makes the most sense there too.

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

Done.

+                       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::GetOutputLinkType(

[dmichael (off chromium), 2013/09/09 20:37:15]

Type->Types? It looks like you can retrieve more than one?

[kcwu, 2013/09/10 12:50:21]

Done.

+    std::vector<PP_OutputProtectionLinkType_Private>& link_type) {

[dmichael (off chromium), 2013/09/09 20:37:15]

out-params should be by pointer:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

[kcwu, 2013/09/10 12:50:21]

Done.

+  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);

[dmichael (off chromium), 2013/09/09 20:37:15]

Can this return NULL? It looks like it can:
http://sourcecodebrowser.com/libxrandr/1.2.2/_xrr_output_8c.html#a41ed0191cd9...

[kcwu, 2013/09/10 12:50:21]

Done.
BTW, this is copied from existing code, line 121. It didn't check NULL. I will
address this in another CL.

+    if (output_info->connection != RR_Connected) {
+      XRRFreeOutputInfo(output_info);

[dmichael (off chromium), 2013/09/09 20:37:15]

optional suggestion: You could use scoped_ptr<XRROutputInfo,
XRROutputInfoDeleter>

Where XRROutputInfoDeleter looks like this:
struct XRROutputInfoDeleter {
  inline void operator()(void* ptr) const {
    XRRFreeOutputInfo(static_cast<XRRFreeOutputInfo*>(ptr));
  }
};

If you end up with more than 1 or 2 places you have to call XRRFreeOutputInfo in
any function, it's probably a good idea to use the RAII idiom (a scoper that
cleans up after you). After the overhead of defining the Deleter functor, it
should shorten your code and make it harder to forget to free.

That said...  in this case, it's probably simpler to just do my next
suggestion...

[kcwu, 2013/09/10 12:50:21]

Done.

+      link_type.push_back(PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_NONE);
+      continue;

[dmichael (off chromium), 2013/09/09 20:37:15]

Why not just use an else? And I'd maybe flip the cases. I.e:
if (output_info->connection == RR_COnnected) {
  // stuff form 475-480
} else {
  // stuff from 470-471
}

This also means you can do XRRFreeOutputInfo in one place, after the else.

[kcwu, 2013/09/10 12:50:21]

Done.

+    }
+
+    PP_OutputProtectionLinkType_Private type;
+    if (DetermineLinkType(output_info, &type))
+      link_type.push_back(type);
+    else
+      result = false;
+    XRRFreeOutputInfo(output_info);
+  }
+  return result;
+}
+
+bool RealOutputConfiguratorDelegate::GetOutputLinkMask(uint32_t* link_mask) {
+  std::vector<PP_OutputProtectionLinkType_Private> link_type;
+  *link_mask = 0;
+  if (!GetOutputLinkType(link_type))
+    return false;
+
+  for (std::vector<PP_OutputProtectionLinkType_Private>::const_iterator it =
+       link_type.begin(); it != link_type.end(); ++it) {
+    *link_mask |= *it;
+  }
+  return true;
+}
+
+bool RealOutputConfiguratorDelegate::GetProtectionMethods(
+    uint32_t* protection_mask) {
+  CHECK(screen_) << "Server not grabbed";
+  std::vector<PP_OutputProtectionLinkType_Private> link_type;
+  if (!GetOutputLinkType(link_type))
+    return false;
+
+  uint32_t enabled = 0;
+  uint32_t unfulfiled = 0;
+
+  DCHECK(screen_->noutput == (int)link_type.size());

[dmichael (off chromium), 2013/09/09 20:37:15]

no C-style casts
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...

[kcwu, 2013/09/10 12:50:21]

Done.

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

nit: It's usually better to static_cast the type that has smaller range. I.e.,
static_cast<size_t>(screen_->noutput) instead.

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

Use DCHECK_EQ() instead as Dale suggested.

+  for (int i = 0; i < screen_->noutput; ++i) {
+    PP_OutputProtectionLinkType_Private type = link_type[i];
+    if (type & (PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_HDMI |
+                PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT)) {

[dmichael (off chromium), 2013/09/09 20:37:15]

optional nit: I would find it more readable as:
if (type & PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_HDMI) ||
   (type & PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT)) {
Maybe that's just me.

[kcwu, 2013/09/10 12:50:21]

Done.

+      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;

[dmichael (off chromium), 2013/09/09 20:37:15]

I don't really understand the APIs we're using... but this reads to me that if
there are 2 or more HDMI/DISPLAYPORTs, but any one of them is not HDCP-enabled,
that GetProtectionMethods will indicate that there is no HDCP support. Is that
what we want?

[kcwu, 2013/09/10 12:50:21]

Yes, content owner would like to playback their content only if protections are
enabled on all links. That bit means "fully enabled" on all applicable output
links.

+  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(
+    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_type;
+  if (!GetOutputLinkType(link_type))

[dmichael (off chromium), 2013/09/09 20:37:15]

You do this a lot. Might it make sense to have something like a member that gets
populated during GrabScreen()? Or is it just not used everywhere GrabScreen is?

[kcwu, 2013/09/10 12:50:21]

Right. It is not used otherwhere GrabServer is.
Anyway, I moved GetOutputLinkType() calls and reduced one.

+    return false;
+  bool ok = true;
+  for (int i = 0; i < screen_->noutput; ++i) {
+    RROutput this_id = screen_->outputs[i];
+    PP_OutputProtectionLinkType_Private type = link_type[i];
+    if (type & (PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_HDMI |
+                PP_OUTPUT_PROTECTION_LINK_TYPE_PRIVATE_DISPLAYPORT)) {

[dmichael (off chromium), 2013/09/09 20:37:15]

ditto, I somewhat prefer separate bit masking and logical ||. The intent is
clearer to me.

[kcwu, 2013/09/10 12:50:21]

Done.

+      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)
+      return false;
+  }
+  return true;
+}
+
+bool RealOutputConfiguratorDelegate::QueryOutputProtectionStatus(
+    chrome::PepperOutputProtectionHost* client,
+    uint32_t* link_mask,
+    uint32_t* protection_mask) {
+  bool ok = GetProtectionMethods(protection_mask) &&
+    GetOutputLinkMask(link_mask);

[dmichael (off chromium), 2013/09/09 20:37:15]

nit: 4-space indent

[kcwu, 2013/09/10 12:50:21]

Done.

+  ProtectionRequests::iterator it = client_protection_request_.find(client);
+
+  // Don't reveal protections requested by other clients.
+  if (it != client_protection_request_.end()) {
+    *protection_mask &= it->second;
+  } else {
+    *protection_mask = 0;
+  }
+  return ok;
+}
+
 void RealOutputConfiguratorDelegate::DestroyUnusedCrtcs(
     const std::vector<OutputConfigurator::OutputSnapshot>& outputs) {
   CHECK(screen_) << "Server not grabbed";