media: Report first file read size UMA in CdmAdapter

Report the size of the first read file in CdmAdapter. Note that in a
CdmAdapter object, we could have multiple cdm::FileIO objects created.
However, we only report the size of the first file read across all
cdm::FileIO objects, to be consistent with the current UMA reported in
PpapiCdmAdapter.

Also, if a promise is rejected due to file related errors, we report
Media.EME.CdmFileIO.FileSizeKBOnError UMA with the size of the latest
read file. There might be a slight chance where the file size reported
is actually not the file causing the error, but that should be very
rare.

Media.EME.CdmFileIO.FileSizeKBOnError reporting is removed from
PpapiCdmAdapter because it's problematic. See crbug.com/780664 for
details.

BUG=721876,780664
TEST=Manually tested and checked about://histograms

Change-Id: I301880c70c361a38d655cca82ea3b280ff5b6027
Reviewed-on: https://chromium-review.googlesource.com/749041
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: John Rummell <jrummell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513835}

media/cdm/aes_decryptor_unittest.cc

@@ -495,13 +495,6 @@ class AesDecryptorTest : public testing::TestWithParam<TestType> {
     }
   }
 
-#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
-  std::unique_ptr<CdmFileIO> CreateCdmFileIO(cdm::FileIOClient* client) {
-    ADD_FAILURE() << "Should never be called";
-    return nullptr;
-  }
-#endif
-
   // Must be the first member to be initialized first and destroyed last.
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 

media/cdm/cdm_adapter.cc

@@ -39,6 +39,13 @@ namespace media {
 
 namespace {
 
+// Constants for UMA reporting of file size (in KB) via
+// UMA_HISTOGRAM_CUSTOM_COUNTS. Note that the histogram is log-scaled (rather
+// than linear).
+constexpr int kSizeKBMin = 1;
+constexpr int kSizeKBMax = 512 * 1024;  // 512MB
+constexpr int kSizeKBBuckets = 100;
+
 cdm::HdcpVersion ToCdmHdcpVersion(HdcpVersion hdcp_version) {
   switch (hdcp_version) {
     case media::HdcpVersion::kHdcpVersionNone:
@@ -881,10 +888,17 @@ void CdmAdapter::OnRejectPromise(uint32_t promise_id,
                                  const char* error_message,
                                  uint32_t error_message_size) {
   // This is the central place for library CDM promise rejection. Cannot report
-  // this is more generic classes like CdmPromise or CdmPromiseAdapter because
+  // this in more generic classes like CdmPromise or CdmPromiseAdapter because
   // they may be used multiple times in one promise chain that involves IPC.
   ReportSystemCodeUMA(key_system_, system_code);
 
+  // UMA to help track file related errors. See http://crbug.com/410630
+  if (system_code == 0x27) {
+    UMA_HISTOGRAM_CUSTOM_COUNTS("Media.EME.CdmFileIO.FileSizeKBOnError",
+                                last_read_file_size_kb_, kSizeKBMin, kSizeKBMax,
+                                kSizeKBBuckets);
+  }
+
   DCHECK(task_runner_->BelongsToCurrentThread());
   cdm_promise_adapter_.RejectPromise(
       promise_id, ToMediaExceptionType(exception), system_code,
@@ -1131,7 +1145,8 @@ void CdmAdapter::OnDeferredInitializationDone(cdm::StreamType stream_type,
 cdm::FileIO* CdmAdapter::CreateFileIO(cdm::FileIOClient* client) {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  std::unique_ptr<CdmFileIO> file_io = helper_->CreateCdmFileIO(client);
+  std::unique_ptr<CdmFileIO> file_io = helper_->CreateCdmFileIO(
+      client, base::Bind(&CdmAdapter::OnFileRead, weak_factory_.GetWeakPtr()));
 
   // The CDM owns the returned object and must call FileIO::Close()
   // to release it.
@@ -1207,4 +1222,17 @@ bool CdmAdapter::AudioFramesDataToAudioFrames(
   return true;
 }
 
+void CdmAdapter::OnFileRead(int file_size_bytes) {
+  DCHECK_GE(file_size_bytes, 0);
+  last_read_file_size_kb_ = file_size_bytes / 1024;
+
+  if (file_size_uma_reported_)
+    return;
+
+  UMA_HISTOGRAM_CUSTOM_COUNTS("Media.EME.CdmFileIO.FileSizeKBOnFirstRead",
+                              last_read_file_size_kb_, kSizeKBMin, kSizeKBMax,
+                              kSizeKBBuckets);
+  file_size_uma_reported_ = true;
+}
+
 }  // namespace media

media/cdm/cdm_adapter.h

@@ -209,6 +209,10 @@ class MEDIA_EXPORT CdmAdapter : public ContentDecryptionModule,
   void ReportOutputProtectionQueryResult(uint32_t link_mask,
                                          uint32_t protection_mask);
 
+  // Callback to report |file_size_bytes| of the file successfully read by
+  // cdm::FileIO.
+  void OnFileRead(int file_size_bytes);
+
   const std::string key_system_;
   const CdmConfig cdm_config_;
 
@@ -245,6 +249,10 @@ class MEDIA_EXPORT CdmAdapter : public ContentDecryptionModule,
   bool uma_for_output_protection_query_reported_ = false;
   bool uma_for_output_protection_positive_result_reported_ = false;
 
+  // Tracks CDM file IO related states.
+  int last_read_file_size_kb_ = 0;
+  bool file_size_uma_reported_ = false;
+
   // Used to keep track of promises while the CDM is processing the request.
   CdmPromiseAdapter cdm_promise_adapter_;
 

media/cdm/cdm_auxiliary_helper.cc

@@ -12,7 +12,8 @@ CdmAuxiliaryHelper::CdmAuxiliaryHelper() {}
 CdmAuxiliaryHelper::~CdmAuxiliaryHelper() {}
 
 std::unique_ptr<CdmFileIO> CdmAuxiliaryHelper::CreateCdmFileIO(
-    cdm::FileIOClient* client) {
+    cdm::FileIOClient* client,
+    CdmFileIO::FileReadCB file_read_cb) {
   return nullptr;
 }
 

media/cdm/cdm_auxiliary_helper.h

@@ -37,7 +37,9 @@ class MEDIA_EXPORT CdmAuxiliaryHelper : public CdmAllocator,
 
   // Given |client|, create a CdmFileIO object and return it. Caller owns the
   // returned object, and should only destroy it after Close() has been called.
-  virtual std::unique_ptr<CdmFileIO> CreateCdmFileIO(cdm::FileIOClient* client);
+  virtual std::unique_ptr<CdmFileIO> CreateCdmFileIO(
+      cdm::FileIOClient* client,
+      CdmFileIO::FileReadCB file_read_cb);
 
   // CdmAllocator implementation.
   cdm::Buffer* CreateCdmBuffer(size_t capacity) override;

media/cdm/cdm_file_io.h

@@ -5,6 +5,7 @@
 #ifndef MEDIA_CDM_CDM_FILE_IO_H_
 #define MEDIA_CDM_CDM_FILE_IO_H_
 
+#include "base/callback_forward.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "media/base/media_export.h"
@@ -16,6 +17,9 @@ namespace media {
 // used with std::unique_ptr.
 class MEDIA_EXPORT CdmFileIO : public cdm::FileIO {
  public:
+  // Callback to report the size of first file read by CdmFileIO.
+  using FileReadCB = base::RepeatingCallback<void(int)>;
+
   ~CdmFileIO() override;
 
  protected:

media/cdm/mock_helpers.cc

@@ -12,6 +12,13 @@ MockCdmAuxiliaryHelper::MockCdmAuxiliaryHelper(
 
 MockCdmAuxiliaryHelper::~MockCdmAuxiliaryHelper() {}
 
+std::unique_ptr<CdmFileIO> MockCdmAuxiliaryHelper::CreateCdmFileIO(
+    cdm::FileIOClient* client,
+    CdmFileIO::FileReadCB) {
+  NOTREACHED();
+  return nullptr;
+}
+
 cdm::Buffer* MockCdmAuxiliaryHelper::CreateCdmBuffer(size_t capacity) {
   return allocator_->CreateCdmBuffer(capacity);
 }

media/cdm/mock_helpers.h

@@ -24,8 +24,8 @@ class MockCdmAuxiliaryHelper : public CdmAuxiliaryHelper {
   MockCdmAuxiliaryHelper(std::unique_ptr<CdmAllocator> allocator);
   ~MockCdmAuxiliaryHelper() override;
 
-  MOCK_METHOD1(CreateCdmFileIO,
-               std::unique_ptr<CdmFileIO>(cdm::FileIOClient* client));
+  std::unique_ptr<CdmFileIO> CreateCdmFileIO(cdm::FileIOClient* client,
+                                             CdmFileIO::FileReadCB) override;
 
   cdm::Buffer* CreateCdmBuffer(size_t capacity) override;
   std::unique_ptr<VideoFrameImpl> CreateCdmVideoFrame() override;

media/cdm/ppapi/ppapi_cdm_adapter.cc

@@ -741,15 +741,6 @@ void PpapiCdmAdapter::OnRejectPromise(uint32_t promise_id,
                                       uint32_t system_code,
                                       const char* error_message,
                                       uint32_t error_message_size) {
-  // UMA to investigate http://crbug.com/410630
-  // TODO(xhwang): Remove after bug is fixed.
-  if (system_code == 0x27) {
-    pp::UMAPrivate uma_interface(this);
-    uma_interface.HistogramCustomCounts("Media.EME.CdmFileIO.FileSizeKBOnError",
-                                        last_read_file_size_kb_, kSizeKBMin,
-                                        kSizeKBMax, kSizeKBBuckets);
-  }
-
   RejectPromise(promise_id, exception, system_code,
                 std::string(error_message, error_message_size));
 }

media/mojo/services/mojo_cdm_file_io.cc

@@ -46,11 +46,16 @@ bool IsValidFileName(const std::string& name) {
 }  // namespace
 
 MojoCdmFileIO::MojoCdmFileIO(cdm::FileIOClient* client,
-                             mojom::CdmStorage* cdm_storage)
-    : client_(client), cdm_storage_(cdm_storage), weak_factory_(this) {
+                             mojom::CdmStorage* cdm_storage,
+                             FileReadCB file_read_cb)
+    : client_(client),
+      cdm_storage_(cdm_storage),
+      file_read_cb_(std::move(file_read_cb)),
+      weak_factory_(this) {
   DVLOG(3) << __func__;
   DCHECK(client_);
   DCHECK(cdm_storage_);
+  // |file_read_cb_| may be null in tests.
 }
 
 MojoCdmFileIO::~MojoCdmFileIO() {
@@ -167,29 +172,31 @@ void MojoCdmFileIO::DoRead(int64_t num_bytes) {
   // file into a buffer and passing it back to |client_|. As these should be
   // small files, we don't worry about breaking it up into chunks to read it.
 
-  // If the file has 0 bytes, no need to read anything.
-  if (num_bytes == 0) {
-    state_ = State::kOpened;
-    client_->OnReadComplete(ClientStatus::kSuccess, nullptr, 0);
-    return;
-  }
-
   // Read the contents of the file. Read() sizes (provided and returned) are
   // type int, so cast appropriately.
   int bytes_to_read = base::checked_cast<int>(num_bytes);
   std::vector<uint8_t> buffer(bytes_to_read);
-  int bytes_read = file_for_reading_.Read(
-      0, reinterpret_cast<char*>(buffer.data()), bytes_to_read);
-  if (bytes_to_read != bytes_read) {
-    // Unable to read the contents of the file. Setting |state_| to kOpened
-    // so that the CDM can write something valid to this file.
-    DVLOG(1) << "Failed to read file " << file_name_ << ". Requested "
-             << bytes_to_read << " bytes, got " << bytes_read;
-    state_ = State::kOpened;
-    OnError(ErrorType::kReadError);
-    return;
+
+  // If the file has 0 bytes, no need to read anything.
+  if (bytes_to_read != 0) {
+    int bytes_read = file_for_reading_.Read(
+        0, reinterpret_cast<char*>(buffer.data()), bytes_to_read);
+    if (bytes_to_read != bytes_read) {
+      // Unable to read the contents of the file. Setting |state_| to kOpened
+      // so that the CDM can write something valid to this file.
+      DVLOG(1) << "Failed to read file " << file_name_ << ". Requested "
+               << bytes_to_read << " bytes, got " << bytes_read;
+      state_ = State::kOpened;
+      OnError(ErrorType::kReadError);
+      return;
+    }
   }
 
+  // Call this before OnReadComplete() so that we always have the latest file
+  // size before CDM fires errors.
+  if (file_read_cb_)
+    file_read_cb_.Run(bytes_to_read);
+
   state_ = State::kOpened;
   client_->OnReadComplete(ClientStatus::kSuccess, buffer.data(), buffer.size());
 }

media/mojo/services/mojo_cdm_file_io.h

@@ -30,7 +30,9 @@ namespace media {
 // Implements a CdmFileIO that communicates with mojom::CdmStorage.
 class MEDIA_MOJO_EXPORT MojoCdmFileIO : public CdmFileIO {
  public:
-  MojoCdmFileIO(cdm::FileIOClient* client, mojom::CdmStorage* cdm_storage);
+  MojoCdmFileIO(cdm::FileIOClient* client,
+                mojom::CdmStorage* cdm_storage,
+                FileReadCB file_read_cb);
   ~MojoCdmFileIO() override;
 
   // CdmFileIO implementation.
@@ -88,6 +90,9 @@ class MEDIA_MOJO_EXPORT MojoCdmFileIO : public CdmFileIO {
 
   mojom::CdmStorage* cdm_storage_;
 
+  // Callback to report the size of a file that was read.
+  FileReadCB file_read_cb_;
+
   // Keep track of the file being used. As this class can only be used for
   // accessing a single file, once |file_name_| is set it shouldn't be changed.
   // |file_name_| is only saved for logging purposes.

media/mojo/services/mojo_cdm_file_io_unittest.cc

@@ -78,8 +78,8 @@ class MojoCdmFileIOTest : public testing::Test {
     client_ = std::make_unique<MockFileIOClient>();
     cdm_storage_ = std::make_unique<MockCdmStorage>();
     ASSERT_TRUE(cdm_storage_->SetUp());
-    file_io_ =
-        std::make_unique<MojoCdmFileIO>(client_.get(), cdm_storage_.get());
+    file_io_ = std::make_unique<MojoCdmFileIO>(
+        client_.get(), cdm_storage_.get(), CdmFileIO::FileReadCB());
   }
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;

media/mojo/services/mojo_cdm_helper.cc

@@ -19,11 +19,13 @@ MojoCdmHelper::MojoCdmHelper(
 MojoCdmHelper::~MojoCdmHelper() {}
 
 std::unique_ptr<CdmFileIO> MojoCdmHelper::CreateCdmFileIO(
-    cdm::FileIOClient* client) {
+    cdm::FileIOClient* client,
+    CdmFileIO::FileReadCB file_read_cb) {
   ConnectToCdmStorage();
 
   // Pass a reference to CdmStorage so that MojoCdmFileIO can open a file.
-  return std::make_unique<MojoCdmFileIO>(client, cdm_storage_ptr_.get());
+  return std::make_unique<MojoCdmFileIO>(client, cdm_storage_ptr_.get(),
+                                         std::move(file_read_cb));
 }
 
 cdm::Buffer* MojoCdmHelper::CreateCdmBuffer(size_t capacity) {

media/mojo/services/mojo_cdm_helper.h

@@ -34,7 +34,9 @@ class MEDIA_MOJO_EXPORT MojoCdmHelper final : public CdmAuxiliaryHelper {
   ~MojoCdmHelper() final;
 
   // CdmAuxiliaryHelper implementation.
-  std::unique_ptr<CdmFileIO> CreateCdmFileIO(cdm::FileIOClient* client) final;
+  std::unique_ptr<CdmFileIO> CreateCdmFileIO(
+      cdm::FileIOClient* client,
+      CdmFileIO::FileReadCB file_read_cb) final;
   cdm::Buffer* CreateCdmBuffer(size_t capacity) final;
   std::unique_ptr<VideoFrameImpl> CreateCdmVideoFrame() final;
   void QueryStatus(QueryStatusCB callback) final;