New issue
Advanced search Search tips

Issue 780664 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 721876



Sign in to add a comment

Fix Media.EME.CdmFileIO.FileSizeKBOnError UMA

Project Member Reported by xhw...@chromium.org, Nov 2 2017

Issue description

In PpapiCdmAdapter, currently Media.EME.CdmFileIO.FileSizeKBOnError reports |last_read_file_size_kb_|, which is only reported on first file read. However, the error could happen much later, e.g. on the N'th file read. So the file size we report in this UMA isn't accurate.

To fix this, we should report all read file size, and really use the latest one to report this UMA.
 
Actually that was what I originally did in https://codereview.chromium.org/568623003/#ps1

But according to https://codereview.chromium.org/568623003/#msg15, 

"the callback created by CompletionCallbackFactory is for single-use only:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/utility/comp..., So I updated this CL so that CdmFileIOImpl only reports the file size of the first read. This should be fine for our use cases."

That's how we end up with the current weird behavior.

Looking at existing data, I don't think the current UMA provides any value. I think we should just add a new one that does the right thing.
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/83a6f4d54685ab63c586b6f14c8e224778493e6b

commit 83a6f4d54685ab63c586b6f14c8e224778493e6b
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Fri Nov 03 17:57:20 2017

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}
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/cdm/aes_decryptor_unittest.cc
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/cdm/cdm_adapter.cc
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/cdm/cdm_adapter.h
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/cdm/cdm_auxiliary_helper.cc
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/cdm/cdm_auxiliary_helper.h
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/cdm/cdm_file_io.h
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/cdm/mock_helpers.cc
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/cdm/mock_helpers.h
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/cdm/ppapi/ppapi_cdm_adapter.cc
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/mojo/services/mojo_cdm_file_io.cc
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/mojo/services/mojo_cdm_file_io.h
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/mojo/services/mojo_cdm_file_io_unittest.cc
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/mojo/services/mojo_cdm_helper.cc
[modify] https://crrev.com/83a6f4d54685ab63c586b6f14c8e224778493e6b/media/mojo/services/mojo_cdm_helper.h

Status: Fixed (was: Started)
For the record, Media.EME.CdmFileIO.FileSizeKBOnError reporting is removed from PpapiCdmAdapter (for pepper CDM) and from now on it will only be reported in CdmAdapter (for mojo CDM).

Sign in to add a comment