VideoCapture: Sort VideoPixelFormat in order of preference

VideoCapture: Sort VideoPixelFormat in order of preference

Committed: https://crrev.com/c066b30dc37e4601ba16f42cbb938d45719af81f
Cr-Commit-Position: refs/heads/master@{#300315}

[magjed_chromium, 2014-10-15 18:27:11]

magjed@chromium.org changed reviewers:
+ perkj@chromium.org

[magjed_chromium, 2014-10-15 18:27:33]

perkj: Please take a look.

[perkj_chrome, 2014-10-16 06:56:59]

How about the UMA-stats that mcasas added that uses the format. Are they based
on the order?  Will they still be ok? 

If that is ok, lgtm.

[magjed_chromium, 2014-10-16 07:55:35]

I forgot about the UMA-stats, I have added them now. Please take another look.

[perkj_chrome, 2014-10-16 08:02:30]

perkj@chromium.org changed reviewers:
+ asvitkine@chromium.org

[perkj_chrome, 2014-10-16 08:02:31]

Alexei, can  the order in a UMA stat  enum be changed between Chrome revisions?

[Alexei Svitkine (slow), 2014-10-16 13:42:05]

On 2014/10/16 08:02:31, perkj wrote:
> Alexei, can  the order in a UMA stat  enum be changed between Chrome
revisions?

No, since that will break aggregation between versions. However, if you really
want to change the order, you can rename the metric using the enum.

[magjed_chromium, 2014-10-17 08:41:42]

magjed@chromium.org changed reviewers:
+ mcasas@chromium.org

[magjed_chromium, 2014-10-17 08:41:42]

perkj, mcasas: Please take another look.

[magjed_chromium, 2014-10-18 16:12:13]

Patchset #4 (id:60001) has been deleted

[mcasas, 2014-10-19 13:13:15]

Looks blergh but I guess we just earned it by
not keeping PixelFormat sorted from the beginning.
LGTM is asvitkine@ likes it and % my nit.

https://codereview.chromium.org/660493002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/660493002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:94: static int
GetUmaLabel(media::VideoPixelFormat format) {
const media::VideoPixelFormat format ?

[magjed_chromium, 2014-10-19 13:30:16]

The alternative is to deprecate the previous UMA enum and make a new one, but I
think we might want to change the order again in the future when either adding
new formats, or e.g. raising texture preference.
One other alternative though, is to deprecate the UMA enum, and stop logging
pixel format to UMA altogether, if you are ok with that mcasas?

https://codereview.chromium.org/660493002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/660493002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:94: static int
GetUmaLabel(media::VideoPixelFormat format) {
On 2014/10/19 13:13:14, mcasas wrote:
> const media::VideoPixelFormat format ?

I haven't seen anyone else make a const non-ref, I have only seen ints and enums
being passed as non-const values.

[mcasas, 2014-10-20 09:06:20]

https://codereview.chromium.org/660493002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/660493002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:94: static int
GetUmaLabel(media::VideoPixelFormat format) {
On 2014/10/19 13:30:16, magjed wrote:
> On 2014/10/19 13:13:14, mcasas wrote:
> > const media::VideoPixelFormat format ?
> 
> I haven't seen anyone else make a const non-ref, I have only seen ints and
enums
> being passed as non-const values.

Ah, true, VPF is an enum. That's the evil with typedef.
Your call.

https://codereview.chromium.org/660493002/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_controller.cc:109: switch
(format) {
Paraphrasing tommi@, I'd like to see this data driven instead of
multiple conditionals:

struct {
  VideoPixelFormat pixel_format;
  UmaPixelFormat uma_pixel_format;
} pixel_formats[] = {
  { PIXEL_FORMAT_UNKNOWN, UNKNOWN },
  { PIXEL_FORMAT_I420, I420 },
  ...
};

for (size_t i = 0; i < ARRAYSIZE_UNSAFE(pixel_formats); ++i) {
  if (format == pixel_formats[i].pixel_format)
    return pixel_formats[i].uma_pixel_format;
}

So things don't need to be updated in two places.

[Alexei Svitkine (slow), 2014-10-20 15:16:54]

lgtm

[magjed_chromium, 2014-10-20 18:09:04]

The CQ bit was checked by magjed@chromium.org

[commit-bot: I haz the power, 2014-10-20 18:10:23]

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

[commit-bot: I haz the power, 2014-10-20 19:31:58]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2014-10-20 19:32:55]

Patchset 5 (id:??) landed as
https://crrev.com/c066b30dc37e4601ba16f42cbb938d45719af81f
Cr-Commit-Position: refs/heads/master@{#300315}