Change the video color space default.

Change the video color space default.

Previously video without color space metadata was assumed to be in
Rec601. Now the default depends on the kind of playback. Normal src=
defaults to Rec601 for SD sized video (<720 pixels high), and Rec709 for
HD. MSE will always default to Rec709. Using a size based heuristic
doesn't make sense for MSE where it is common for the resolution to
change mid playback.

This CL doesn't change the meaning of COLOR_SPACE_UNSPECIFIED. Instead,
it adds a color space field to VideoDecoderConfig, and updates the video
decoders to use this as the default if they don't find a more
authoritative value in the bitstream.

This also fixes a (year old!) bug causing the blackwhite
tests to always succeed, renames the rec709 blackwhite test
file to match the name in blackwhite.html, and re-encodes it
to contain the color space metadata (previously it had none).

BUG=333619
Committed: https://crrev.com/4dc6c2ad0e595a5e0b543e8e1b8961ee0d742a32
Cr-Commit-Position: refs/heads/master@{#338110}

[watk, 2015-07-06 22:25:55]

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

[DaleCurtis, 2015-07-06 22:36:15]

https://codereview.chromium.org/1221903003/diff/20001/content/renderer/pepper...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://codereview.chromium.org/1221903003/diff/20001/content/renderer/pepper...
content/renderer/pepper/video_decoder_shim.cc:864: // XXX: Should we change this
default too?
Maybe bbudge@ knows.

https://codereview.chromium.org/1221903003/diff/20001/media/base/video_decode...
File media/base/video_decoder_config.cc (right):

https://codereview.chromium.org/1221903003/diff/20001/media/base/video_decode...
media/base/video_decoder_config.cc:178: VideoCodecProfile
VideoDecoderConfig::profile() const {
Can you move all of these one-line getters into the .h file? Style guide forbids
hacker_style() methods unless the impl is inlined. Don't worry about the
non-one-line ones since those would require a rename which is a larger task.

https://codereview.chromium.org/1221903003/diff/20001/media/base/video_decode...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/1221903003/diff/20001/media/base/video_decode...
media/base/video_decoder_config.h:74: static VideoFrame::ColorSpace
DefaultMseColorSpace();
How about just DefaultColorSpace() and MSE can hard code somewhere in MSE code?
We don't want to tie this class to a MSE/src name.

Actually SrcColorSpace is only used in one place, why not hard code it there
instead?

[watk, 2015-07-06 23:15:57]

watk@chromium.org changed reviewers:
+ bbudge@chromium.org, lcwu@chromium.org

[watk, 2015-07-06 23:15:58]

bbudge@, PTAL at video_decoder_shim.cc. I'm not sure whether we should be
changing the default for pepper. What do you think?

lcwu@, PTAL at chromecast/

https://codereview.chromium.org/1221903003/diff/20001/media/base/video_decode...
File media/base/video_decoder_config.cc (right):

https://codereview.chromium.org/1221903003/diff/20001/media/base/video_decode...
media/base/video_decoder_config.cc:178: VideoCodecProfile
VideoDecoderConfig::profile() const {
On 2015/07/06 22:36:14, DaleCurtis wrote:
> Can you move all of these one-line getters into the .h file? Style guide
forbids
> hacker_style() methods unless the impl is inlined. Don't worry about the
> non-one-line ones since those would require a rename which is a larger task.

Done.

https://codereview.chromium.org/1221903003/diff/20001/media/base/video_decode...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/1221903003/diff/20001/media/base/video_decode...
media/base/video_decoder_config.h:74: static VideoFrame::ColorSpace
DefaultMseColorSpace();
On 2015/07/06 22:36:14, DaleCurtis wrote:
> How about just DefaultColorSpace() and MSE can hard code somewhere in MSE
code?
> We don't want to tie this class to a MSE/src name.
> 
> Actually SrcColorSpace is only used in one place, why not hard code it there
> instead?

Done.

[bbudge, 2015-07-07 00:25:59]

bbudge@chromium.org changed reviewers:
+ codebythepound@gmail.com

[bbudge, 2015-07-07 00:26:00]

video_decoder_shim.cc LGTM but adding Mike to confirm.

[DaleCurtis, 2015-07-07 01:05:33]

lgtm

[watk, 2015-07-08 18:19:53]

Friendly ping lcwu@

[CodeByThePound, 2015-07-08 19:29:26]

Change to video_decoder_shim.cc LGTM also.

[lcwu1, 2015-07-08 21:23:43]

lcwu@chromium.org changed reviewers:
+ gunsch@chromium.org

[lcwu1, 2015-07-08 21:23:44]

Sorry for my delay in reviewing this CL. It's been hectic for me the last
several days. Adding gunsch@ as the chromecast/ reviewer.

[gunsch, 2015-07-08 21:26:13]

chromecast/ lgtm (only reviewed the changes in chromecast/)

[watk, 2015-07-08 21:27:08]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2015-07-08 21:29:19]

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

[commit-bot: I haz the power, 2015-07-08 21:32:35]

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

[commit-bot: I haz the power, 2015-07-08 21:32:37]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

(exceeded global retry quota)

[watk, 2015-07-08 22:35:51]

The CQ bit was checked by watk@chromium.org

[watk, 2015-07-08 22:35:51]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
CodeByThePound@gmail.com, gunsch@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1221903003/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2015-07-08 22:37:31]

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

[commit-bot: I haz the power, 2015-07-09 00:08:07]

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

[commit-bot: I haz the power, 2015-07-09 00:08:09]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

(exceeded global retry quota)

[watk, 2015-07-09 18:05:30]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2015-07-09 18:08:45]

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

[commit-bot: I haz the power, 2015-07-09 19:16:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-09 19:17:30]

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

[amineer_google, 2015-07-09 20:17:09]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1228843003/ by amineer@google.com.

The reason for reverting is: Causing official continuous builds to fail with the
following:

../../media/cast/sender/h264_vt_encoder_unittest.cc:308:22: error: no matching
constructor for initialization of 'media::VideoDecoderConfig'
  VideoDecoderConfig config(kCodecH264, H264PROFILE_MAIN, frame_->format(),
                     ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../media/base/video_decoder_config.h:78:3: note: candidate constructor not
viable: requires 10 arguments, but 9 were provided
  VideoDecoderConfig(VideoCodec codec,
  ^
../../media/base/video_decoder_config.h:70:20: note: candidate constructor (the
implicit copy constructor) not viable: requires 1 argument, but 9 were provided
class MEDIA_EXPORT VideoDecoderConfig {
                   ^
../../media/base/video_decoder_config.h:74:3: note: candidate constructor not
viable: requires 0 arguments, but 9 were provided
  VideoDecoderConfig();
  ^
1 error generated..