Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(243)

Issue 1230593005: Reland: Change the video color space default. (Closed)

Created:
5 years, 5 months ago by watk
Modified:
5 years, 5 months ago
Reviewers:
bbudge, DaleCurtis, gunsch
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, imcheng+watch_chromium.org, hclam+watch_chromium.org, hguihot+watch_chromium.org, avayvod+watch_chromium.org, lcwu+watch_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, feature-media-reviews_chromium.org, Aaron Boodman, darin-cc_chromium.org, jasonroberts+watch_google.com, mkwst+moarreviews-renderer_chromium.org, gunsch+watch_chromium.org, darin (slow to review), ben+mojo_chromium.org, hubbe+watch_chromium.org, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reland: Change the video color space default. Reverted by: e735fb0c4b13b72561b8f9931a8788e97deced33 Original CL: https://codereview.chromium.org/1228843003 The new change is to media/cast/sender/h264_vt_encoder_unittest.cc. The rest of the changes are due to rebasing on top of 11edd34b171b78462c1a27ac6587d977833122ea, which moves the color space enum. 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/7cb4525761b68e41671af9006920d568be3c5405 Cr-Commit-Position: refs/heads/master@{#338607}

Patch Set 1 #

Patch Set 2 : big rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+149 lines, -103 lines) Patch
M chromecast/common/media/cma_param_traits.cc View 1 2 chunks +5 lines, -3 lines 0 comments Download
M chromecast/common/media/cma_param_traits_macros.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M chromecast/media/cma/ipc_streamer/video_decoder_config_marshaller.cc View 1 5 chunks +6 lines, -1 line 0 comments Download
M chromecast/media/cma/pipeline/audio_video_pipeline_impl_unittest.cc View 1 1 chunk +3 lines, -2 lines 0 comments Download
M chromecast/media/cma/test/demuxer_stream_for_test.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M chromecast/media/cma/test/mock_frame_provider.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M content/renderer/pepper/video_decoder_shim.cc View 1 2 chunks +15 lines, -18 lines 0 comments Download
M media/base/test_helpers.cc View 1 1 chunk +3 lines, -2 lines 0 comments Download
M media/base/video_decoder_config.h View 1 2 chunks +15 lines, -8 lines 0 comments Download
M media/base/video_decoder_config.cc View 1 3 chunks +4 lines, -35 lines 0 comments Download
M media/base/video_types.h View 1 1 chunk +3 lines, -3 lines 0 comments Download
M media/cast/sender/h264_vt_encoder_unittest.cc View 1 1 chunk +3 lines, -2 lines 0 comments Download
M media/ffmpeg/ffmpeg_common.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M media/ffmpeg/ffmpeg_common.cc View 1 2 chunks +28 lines, -6 lines 0 comments Download
M media/filters/decrypting_demuxer_stream.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M media/filters/ffmpeg_video_decoder.cc View 1 1 chunk +8 lines, -4 lines 0 comments Download
M media/filters/ffmpeg_video_decoder_unittest.cc View 1 7 chunks +10 lines, -3 lines 0 comments Download
M media/filters/vpx_video_decoder.cc View 1 2 chunks +6 lines, -1 line 0 comments Download
M media/formats/mp2t/es_adapter_video_unittest.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M media/formats/mp2t/es_parser_h264.cc View 1 1 chunk +3 lines, -2 lines 0 comments Download
M media/formats/mp4/mp4_stream_parser.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M media/formats/webm/webm_video_client.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M media/mojo/interfaces/media_types.mojom View 1 3 chunks +11 lines, -1 line 0 comments Download
M media/mojo/services/media_type_converters.cc View 1 3 chunks +13 lines, -2 lines 0 comments Download
A media/test/data/blackwhite_yuv420p_rec709.mp4 View Binary file 0 comments Download

Messages

Total messages: 18 (8 generated)
watk
PTAL. The previous CL was reverted because it failed to compile an official build, cast ...
5 years, 5 months ago (2015-07-10 21:36:44 UTC) #3
gunsch
lgtm again (chromecast/)
5 years, 5 months ago (2015-07-10 21:37:42 UTC) #4
bbudge
lgtm
5 years, 5 months ago (2015-07-10 21:41:32 UTC) #5
DaleCurtis
lgtm, typically we upload the original patch as PS#1 and the fixed one as PS#2 ...
5 years, 5 months ago (2015-07-13 02:01:23 UTC) #6
watk
On 2015/07/13 02:01:23, DaleCurtis wrote: > lgtm, typically we upload the original patch as PS#1 ...
5 years, 5 months ago (2015-07-13 17:34:31 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230593005/1
5 years, 5 months ago (2015-07-13 17:35:43 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_32_ng/builds/72588) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, ...
5 years, 5 months ago (2015-07-13 17:38:31 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230593005/20001
5 years, 5 months ago (2015-07-14 00:09:51 UTC) #16
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 5 months ago (2015-07-14 00:46:13 UTC) #17
commit-bot: I haz the power
5 years, 5 months ago (2015-07-14 00:47:28 UTC) #18
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/7cb4525761b68e41671af9006920d568be3c5405
Cr-Commit-Position: refs/heads/master@{#338607}

Powered by Google App Engine
This is Rietveld 408576698