Add VideoRotation UMA Logging

Added UMA logging for Video Rotation metadata.

BUG=47554
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283962

[suderman, 2014-07-17 00:07:40]

Enum logging for the Video Rotation information. At this point it only logs via
the ffmpeg demuxer stream. We can change the logging location to log for all
video types however as the rotation metadata is an mp4 feature they are
guaranteed to be 0 degree rotations.

If we want to express its only occurs with mp4 files we could change the name
appropriately.

[scherkus (not reviewing), 2014-07-17 01:26:18]

https://codereview.chromium.org/394373002/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/394373002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:139: "Media.VideoRotation", video_rotation_,
VIDEO_ROTATION_MAX + 1);
I believe we'll want this logged inside of webmediaplayer_impl.cc inside of the
metadata callback

otherwise this path may end up logging rotation values for videos we don't end
up playing

[scherkus (not reviewing), 2014-07-17 01:27:05]

also your CL description is a bit funky

you'll also want to update the BUG= number

https://codereview.chromium.org/394373002/diff/1/media/base/video_rotation.h
File media/base/video_rotation.h (right):

https://codereview.chromium.org/394373002/diff/1/media/base/video_rotation.h#...
media/base/video_rotation.h:17: VIDEO_ROTATION_MAX = VIDEO_ROTATION_270  // MAX
VALUE
nit: do you need the // MAX VALUE stuff?

[suderman, 2014-07-17 16:49:28]

Changelist description updated and logging moved to the webmediaplayer.

[scherkus (not reviewing), 2014-07-17 17:57:45]

lgtm

[suderman, 2014-07-17 18:16:42]

asvitkine@chromium.org: Please review changes in 

histograms.xml

[Alexei Svitkine (slow), 2014-07-17 18:20:59]

lgtm % comments

https://codereview.chromium.org/394373002/diff/20001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/394373002/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:968: media::VIDEO_ROTATION_MAX +
1);
Optional nit:

General convention for UMA histograms is to have the MAX entry be 1 higher than
the last entry, so that the +1 here would be unnecessary (once you update the
enum) and also makes it so you don't need to change what the MAX ='s to each
time you add a new entry.

https://codereview.chromium.org/394373002/diff/20001/media/base/video_rotation.h
File media/base/video_rotation.h (right):

https://codereview.chromium.org/394373002/diff/20001/media/base/video_rotatio...
media/base/video_rotation.h:11: // where it can be rotated by 90 degree
intervals.
Expand comment to mention not to renumber entries and only append at the end,
since this is used in UMA.

[scherkus (not reviewing), 2014-07-17 18:51:48]

https://codereview.chromium.org/394373002/diff/20001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/394373002/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:968: media::VIDEO_ROTATION_MAX +
1);
On 2014/07/17 18:20:59, Alexei Svitkine wrote:
> Optional nit:
> 
> General convention for UMA histograms is to have the MAX entry be 1 higher
than
> the last entry, so that the +1 here would be unnecessary (once you update the
> enum) and also makes it so you don't need to change what the MAX ='s to each
> time you add a new entry.

Us media folk decided a while back that defining _MAX as LAST_VALUE + 1 isn't
technically correct as "max" is typically defined as an inclusive endpoint.
Using something like _NUM (which typically is LAST_VALUE + 1) isn't good either
since over the long run, enum values can get deprecated leaving holes in the
enum, leading to _NUM being misleading. Also, introducing an extra enum for the
sake of UMA pollutes all non-UMA switch/case statements.

So we standardized on _MAX and wrote a PRESUBMIT.py to verify that enums, _MAX,
and UMA usage is correct:
http://src.chromium.org/viewvc/chrome/trunk/src/media/PRESUBMIT.py

[Alexei Svitkine (slow), 2014-07-17 19:00:18]

Good to know, thanks.


On Thu, Jul 17, 2014 at 2:51 PM, <scherkus@chromium.org> wrote:

>
> https://codereview.chromium.org/394373002/diff/20001/
> content/renderer/media/webmediaplayer_impl.cc
> File content/renderer/media/webmediaplayer_impl.cc (right):
>
> https://codereview.chromium.org/394373002/diff/20001/
> content/renderer/media/webmediaplayer_impl.cc#newcode968
> content/renderer/media/webmediaplayer_impl.cc:968:
> media::VIDEO_ROTATION_MAX + 1);
> On 2014/07/17 18:20:59, Alexei Svitkine wrote:
>
>> Optional nit:
>>
>
>  General convention for UMA histograms is to have the MAX entry be 1
>>
> higher than
>
>> the last entry, so that the +1 here would be unnecessary (once you
>>
> update the
>
>> enum) and also makes it so you don't need to change what the MAX ='s
>>
> to each
>
>> time you add a new entry.
>>
>
> Us media folk decided a while back that defining _MAX as LAST_VALUE + 1
> isn't technically correct as "max" is typically defined as an inclusive
> endpoint. Using something like _NUM (which typically is LAST_VALUE + 1)
> isn't good either since over the long run, enum values can get
> deprecated leaving holes in the enum, leading to _NUM being misleading.
> Also, introducing an extra enum for the sake of UMA pollutes all non-UMA
> switch/case statements.
>
> So we standardized on _MAX and wrote a PRESUBMIT.py to verify that
> enums, _MAX, and UMA usage is correct:
> http://src.chromium.org/viewvc/chrome/trunk/src/media/PRESUBMIT.py
>
> https://codereview.chromium.org/394373002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[suderman, 2014-07-17 19:05:14]

The CQ bit was checked by suderman@chromium.org

[commit-bot: I haz the power, 2014-07-17 19:12:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/suderman@chromium.org/394373002/20001

[commit-bot: I haz the power, 2014-07-17 22:32:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-17 22:56:06]

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

[commit-bot: I haz the power, 2014-07-17 22:56:07]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[suderman, 2014-07-18 01:19:41]

The CQ bit was checked by suderman@chromium.org

[commit-bot: I haz the power, 2014-07-18 01:23:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/suderman@chromium.org/394373002/20001

[commit-bot: I haz the power, 2014-07-18 01:27:14]

Change committed as 283962