media: Add Rappor reporting in MediaLog

media: Add Rappor reporting in MediaLog

Today in media/ we can report rappor using MediaClient. However, this
requires the caller to know about the URL to report. This works at
high levels (e.g. WebMediaPlayer*), but doesn't work well at lower
levels, e.g. demuxers, decoders, where URL knowledge is not passed down.

On the other hand, MediaLog has been passed around in media code in a
lot of places. This CL addes rappor reporting to MediaLog so that we can
easily repport rappor in a lot of places.

In the future we may want to rename MediaLog to MediaMetrics.

BUG=499949
TEST=Tested the new metric added.

Committed: https://crrev.com/2178f439e04043d88cef11310e7fd45d26095fa6
Cr-Commit-Position: refs/heads/master@{#400302}

[xhwang, 2016-06-15 00:19:06]

xhwang@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[xhwang, 2016-06-15 00:19:07]

PTAL

After this, we can add a lot of Rappor metrics within our pipeline, e.g. what
sites are using VP9 etc.

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/base/media...
File media/base/media_log.h (right):

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/base/media...
media/base/media_log.h:53: virtual void RecordRapporWithSecurityOrigin(const
std::string& metric);
Reporting security origin is consistent with our existing rappor metrics. We can
also report the WebMediaPlayerSource URL, but I am not sure how useful that will
be.

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/filters/gp...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/filters/gp...
media/filters/gpu_video_decoder.cc:129: static void
ReportGpuVideoDecoderInitializeStatusToUMAAndRunCB(
Today this is called for all init cases. When a failure is reported, it could be
due to a lot of reasons, e.g. config not supported (WebM), profile changed
during config change, and so on.

I feel the purpose of this UMA is to catch real VDA init failures (maybe I am
wrong). Does it make sense to have a separate UMA/Rappor that that case?

[xhwang, 2016-06-15 00:33:20]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[DaleCurtis, 2016-06-15 00:33:47]

lgtm

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/base/media...
File media/base/media_log.h (right):

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/base/media...
media/base/media_log.h:53: virtual void RecordRapporWithSecurityOrigin(const
std::string& metric);
On 2016/06/15 at 00:19:07, xhwang wrote:
> Reporting security origin is consistent with our existing rappor metrics. We
can also report the WebMediaPlayerSource URL, but I am not sure how useful that
will be.

I think this is better for now, as the player url is often a cdn redirect which
may not be useful in tracking down the main page.

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/filters/gp...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/filters/gp...
media/filters/gpu_video_decoder.cc:82: media_log_(media_log),
Need to use std::move() if you drop const& I thought.

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/filters/gp...
media/filters/gpu_video_decoder.cc:129: static void
ReportGpuVideoDecoderInitializeStatusToUMAAndRunCB(
On 2016/06/15 at 00:19:07, xhwang wrote:
> Today this is called for all init cases. When a failure is reported, it could
be due to a lot of reasons, e.g. config not supported (WebM), profile changed
during config change, and so on.
> 
> I feel the purpose of this UMA is to catch real VDA init failures (maybe I am
wrong). Does it make sense to have a separate UMA/Rappor that that case?

Well, the problem is we can't tell the difference between a bad configuration /
vda bug / and codec normally not supported... actually, we could rescan the
supported profiles and only log error_not_supported if the decoder should have
been supported?

[commit-bot: I haz the power, 2016-06-15 00:33:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2064083002/40001

[commit-bot: I haz the power, 2016-06-15 00:49:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 00:49:38]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xhwang, 2016-06-15 04:48:49]

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/filters/gp...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/filters/gp...
media/filters/gpu_video_decoder.cc:82: media_log_(media_log),
On 2016/06/15 00:33:47, DaleCurtis wrote:
> Need to use std::move() if you drop const& I thought.

https://www.chromium.org/developers/coding-style

If the function (at least sometimes) takes a ref on a refcounted object, declare
the param as scoped_refptr<T>. The caller can decide whether it wishes to
transfer ownership (by calling std::move(t) when passing t) or retain its ref
(by simply passing t directly).

So it's up to the caller to decide whether to use std::move(). In our case, the
caller is DefaultRendererFactory, which needs to keep it's own |media_log_| for
other uses, so the caller isn't using std::move either.

https://chromiumcodereview.appspot.com/2064083002/diff/40001/media/filters/gp...
media/filters/gpu_video_decoder.cc:129: static void
ReportGpuVideoDecoderInitializeStatusToUMAAndRunCB(
On 2016/06/15 00:33:47, DaleCurtis wrote:
> On 2016/06/15 at 00:19:07, xhwang wrote:
> > Today this is called for all init cases. When a failure is reported, it
could
> be due to a lot of reasons, e.g. config not supported (WebM), profile changed
> during config change, and so on.
> > 
> > I feel the purpose of this UMA is to catch real VDA init failures (maybe I
am
> wrong). Does it make sense to have a separate UMA/Rappor that that case?
> 
> Well, the problem is we can't tell the difference between a bad configuration
/
> vda bug / and codec normally not supported... actually, we could rescan the
> supported profiles and only log error_not_supported if the decoder should have
> been supported?

Yeah, I feel it's nicer if we can at least distinguish between not_supported and
VDA::Init( ) error, where we has passed all kinds of tests and assume the VDA
should support the config.

[xhwang, 2016-06-15 04:50:30]

xhwang@chromium.org changed reviewers:
+ holte@chromium.org

[xhwang, 2016-06-15 04:50:31]

holte@chromium.org: Please OWNERS review rappor.xml change!

[xhwang, 2016-06-15 04:50:49]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 04:51:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2064083002/60001

[commit-bot: I haz the power, 2016-06-15 05:57:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 05:57:10]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2016-06-16 00:16:38]

lgtm

[xhwang, 2016-06-16 04:14:05]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-06-16 04:14:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2064083002/#ps60001
(title: "fix test")

[commit-bot: I haz the power, 2016-06-16 04:14:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2064083002/60001

[commit-bot: I haz the power, 2016-06-16 04:20:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-16 04:20:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xhwang, 2016-06-16 04:25:36]

xhwang@chromium.org changed reviewers:
+ nasko@chromium.org

[xhwang, 2016-06-16 04:25:37]

nasko: Please OWNERS review two-line changes in
content/renderer/render_frame_impl.cc. Thanks!

[xhwang, 2016-06-16 16:27:56]

xhwang@chromium.org changed reviewers:
+ jam@chromium.org

[xhwang, 2016-06-16 16:27:57]

It seems nasko is busy.

jam: Could you please OWNERS review two-line changes in
content/renderer/render_frame_impl.cc. Thanks!

[jam, 2016-06-16 18:20:21]

lgtm

[xhwang, 2016-06-16 21:34:20]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2016-06-16 21:35:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2064083002/60001

[commit-bot: I haz the power, 2016-06-16 23:45:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-16 23:47:07]

Description was changed from

==========
media: Add Rappor reporting in MediaLog

Today in media/ we can report rappor using MediaClient. However, this
requires the caller to know about the URL to report. This works at
high levels (e.g. WebMediaPlayer*), but doesn't work well at lower
levels, e.g. demuxers, decoders, where URL knowledge is not passed down.

On the other hand, MediaLog has been passed around in media code in a
lot of places. This CL addes rappor reporting to MediaLog so that we can
easily repport rappor in a lot of places.

In the future we may want to rename MediaLog to MediaMetrics.

BUG=499949
TEST=Tested the new metric added.
==========

to

==========
media: Add Rappor reporting in MediaLog

Today in media/ we can report rappor using MediaClient. However, this
requires the caller to know about the URL to report. This works at
high levels (e.g. WebMediaPlayer*), but doesn't work well at lower
levels, e.g. demuxers, decoders, where URL knowledge is not passed down.

On the other hand, MediaLog has been passed around in media code in a
lot of places. This CL addes rappor reporting to MediaLog so that we can
easily repport rappor in a lot of places.

In the future we may want to rename MediaLog to MediaMetrics.

BUG=499949
TEST=Tested the new metric added.

Committed: https://crrev.com/2178f439e04043d88cef11310e7fd45d26095fa6
Cr-Commit-Position: refs/heads/master@{#400302}
==========

[commit-bot: I haz the power, 2016-06-16 23:47:08]

Patchset 4 (id:??) landed as
https://crrev.com/2178f439e04043d88cef11310e7fd45d26095fa6
Cr-Commit-Position: refs/heads/master@{#400302}