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

Issue 2697863003: color: Clarify default behaviors (Closed)

Created:
3 years, 10 months ago by ccameron
Modified:
3 years, 10 months ago
CC:
chromium-reviews, cc-bugs_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

color: Clarify default behaviors Change YUVDrawFrame to use REC709 if no valid color space is specified. Make ColorTransform do nothing if the source color space is invalid. Add a method for constructing video color spaces, CreateVideo, which does translation from h264 spec values to gfx::ColorSpace values. For now, leave all values in existence, though they can likely be removed in the future. Change the media/ callers that used to static-cast h264 values to use this constructor. Change the ColorSpace PrimaryID/TransferID/MatrixID to not exactly match the h264 spec (offset by 1000) to discourage doing static casts. Add a method to query the h264 values for casting. Change vp9 to always specify a color space to its frame, even when one is not known. BUG=691522 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2697863003 Cr-Commit-Position: refs/heads/master@{#451151} Committed: https://chromium.googlesource.com/chromium/src/+/172d451b1f1d429ce496c0bb4c57e8e4874cdc43

Patch Set 1 #

Total comments: 11

Patch Set 2 : More INVALI #

Patch Set 3 : color: Remove redundant PrimaryID/TransferID/MatrixID values #

Total comments: 4

Patch Set 4 : Do hubbe's suggestion #

Patch Set 5 : Do less #

Total comments: 3

Patch Set 6 : Remove the redundant values #

Patch Set 7 : Remove more refs from chromecast #

Total comments: 5

Patch Set 8 : Incorporate review feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+269 lines, -395 lines) Patch
M cc/ipc/struct_traits_unittest.cc View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M cc/output/gl_renderer.cc View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M chromecast/media/cma/base/decoder_config_adapter.cc View 1 2 3 4 5 1 chunk +0 lines, -57 lines 0 comments Download
M media/ffmpeg/ffmpeg_common.cc View 1 2 1 chunk +0 lines, -58 lines 0 comments Download
M media/filters/ffmpeg_video_decoder.cc View 1 2 3 1 chunk +6 lines, -6 lines 0 comments Download
M media/filters/h264_parser.cc View 1 2 3 4 5 6 7 1 chunk +7 lines, -7 lines 0 comments Download
M media/filters/vpx_video_decoder.cc View 1 2 3 4 5 6 7 2 chunks +5 lines, -6 lines 0 comments Download
M media/formats/webm/webm_colour_parser.cc View 1 2 3 4 5 2 chunks +17 lines, -99 lines 0 comments Download
M ui/gfx/color_space.h View 1 2 3 4 5 4 chunks +57 lines, -91 lines 0 comments Download
M ui/gfx/color_space.cc View 1 2 3 4 5 6 7 11 chunks +145 lines, -52 lines 0 comments Download
M ui/gfx/color_space_win.cc View 1 2 3 4 5 5 chunks +4 lines, -12 lines 0 comments Download
M ui/gfx/color_transform.cc View 1 2 3 4 5 2 chunks +5 lines, -1 line 0 comments Download
M ui/gfx/color_transform_unittest.cc View 1 2 3 4 5 4 chunks +18 lines, -5 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 47 (24 generated)
ccameron
Rec709 is the right thing to do for not-well-specified video. sRGB is the right thing ...
3 years, 10 months ago (2017-02-15 07:39:37 UTC) #3
hubbe
I feel like this CL is doing a lot of different things, is there a ...
3 years, 10 months ago (2017-02-15 08:48:57 UTC) #4
ccameron
Urgh, I switched UNSPECIFIED and UNKNOWN in my head, which probably made this patch confusing. ...
3 years, 10 months ago (2017-02-15 09:34:28 UTC) #5
hubbe
https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#newcode499 ui/gfx/color_space.cc:499: switch (matrix_) { On 2017/02/15 09:34:28, ccameron wrote: > ...
3 years, 10 months ago (2017-02-15 18:47:45 UTC) #6
hubbe
On 2017/02/15 18:47:45, hubbe wrote: > https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc > File ui/gfx/color_space.cc (right): > > https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#newcode499 > ...
3 years, 10 months ago (2017-02-15 21:15:18 UTC) #7
ccameron
On 2017/02/15 21:15:18, hubbe wrote: > On 2017/02/15 18:47:45, hubbe wrote: > > https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc > ...
3 years, 10 months ago (2017-02-15 21:24:59 UTC) #8
ccameron
On 2017/02/15 21:24:59, ccameron wrote: > I'm putting together a patch that makes the media ...
3 years, 10 months ago (2017-02-15 21:25:27 UTC) #9
ccameron
ptal -- this gets rid of the static-cast-h264-values-to-ColorSpace-values behavior, and removes the ambiguous RESERVED/UNSPECIFIED/UNKNOWN values. ...
3 years, 10 months ago (2017-02-15 21:57:24 UTC) #11
ccameron
On 2017/02/15 21:57:24, ccameron wrote: > ptal -- this gets rid of the static-cast-h264-values-to-ColorSpace-values > ...
3 years, 10 months ago (2017-02-15 23:07:24 UTC) #16
hubbe
> Btw, another approach that seems reasonable to me is to > leave the media ...
3 years, 10 months ago (2017-02-15 23:30:43 UTC) #17
ccameron
Sounds good -- I'll let you know when I have a new version uploaded. On ...
3 years, 10 months ago (2017-02-15 23:39:40 UTC) #18
chromium-reviews
One more possibility: We might not need the UNSPECIFIED part if we make sure that ...
3 years, 10 months ago (2017-02-15 23:44:38 UTC) #19
ccameron
I took the approach from the last comment -- added a CreateVideo method, which takes ...
3 years, 10 months ago (2017-02-16 02:01:46 UTC) #23
hubbe
> I took the approach from the last comment -- added a > CreateVideo method, ...
3 years, 10 months ago (2017-02-16 05:02:39 UTC) #28
ccameron
Updated -- ptal. On 2017/02/16 05:02:39, hubbe wrote: > > I took the approach from ...
3 years, 10 months ago (2017-02-16 07:09:10 UTC) #29
ccameron
halliwell@, can you take a look at the chromecast/ parts? I deleted some enum checks ...
3 years, 10 months ago (2017-02-16 07:11:29 UTC) #31
hubbe
LGTM > On 2017/02/16 05:02:39, hubbe wrote: > > > I took the approach from ...
3 years, 10 months ago (2017-02-16 08:20:50 UTC) #32
halliwell
On 2017/02/16 08:20:50, hubbe wrote: > LGTM > > > On 2017/02/16 05:02:39, hubbe wrote: ...
3 years, 10 months ago (2017-02-16 15:03:06 UTC) #34
servolk
Please don't remove chromecast enums, this will break our vendor interface. We do need those ...
3 years, 10 months ago (2017-02-16 17:35:06 UTC) #36
ccameron
Thanks -- updated the chromecast parts to just remove the checks https://codereview.chromium.org/2697863003/diff/120001/media/filters/h264_parser.cc File media/filters/h264_parser.cc (right): ...
3 years, 10 months ago (2017-02-16 20:45:27 UTC) #37
halliwell
On 2017/02/16 20:45:27, ccameron wrote: > Thanks -- updated the chromecast parts to just remove ...
3 years, 10 months ago (2017-02-16 21:08:45 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2697863003/140001
3 years, 10 months ago (2017-02-16 22:25:02 UTC) #44
commit-bot: I haz the power
3 years, 10 months ago (2017-02-17 00:16:29 UTC) #47
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/172d451b1f1d429ce496c0bb4c57...

Powered by Google App Engine
This is Rietveld 408576698