Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2368573002:
[Chromecast] Add color space and HDR data to Chromecast VideoConfig (Closed)
|
Created:
4 years, 2 months ago by servolk Modified:
4 years, 2 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, gfhuang+watch_chromium.org, halliwell+watch_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Add color space and HDR data to Chromecast VideoConfig
BUG=b/31399384
Committed: https://crrev.com/6926d672f6291d4e2d3730fa7b6453f7b566ab6c
Cr-Commit-Position: refs/heads/master@{#420674}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added static asserts #
Messages
Total messages: 27 (12 generated)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Description was changed from ========== [Chromecast] Add color space and HDR data to Chromecast VideoConfig BUG=b/31399384 ========== to ========== [Chromecast] Add color space and HDR data to Chromecast VideoConfig BUG=b/31399384 ==========
servolk@chromium.org changed reviewers: + halliwell@chromium.org, mbjorge@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/23 16:37:11, servolk wrote: The changes in decoder_config.h are necessary to fix the build break of cast_shell on master branch. So even though we can't copy color space info in decoder_config_adapter.cc atm due to not having gfx::ColorSpace getters, it's still worth landing this for now. Mojo IPC is not implemented for gfx::ColorSpace in Chromium master yet (crbug.com/649419), so HDR won't work on cast_shell master for now anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... chromecast/media/cma/base/decoder_config_adapter.cc:241: // space components. We'll need to way to fix this. Can you file a bug to not forget this and add the bug ID here? https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... chromecast/public/media/decoder_config.h:179: SMPTEST432_1 = 12, Do we need all these values? It seems like a lot of copy/paste. Also is there a risk of getting out of sync with color_space.h ?
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... File chromecast/media/cma/base/decoder_config_adapter.cc (right): https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... chromecast/media/cma/base/decoder_config_adapter.cc:241: // space components. We'll need to way to fix this. On 2016/09/23 17:31:05, halliwell wrote: > Can you file a bug to not forget this and add the bug ID here? Done. https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... File chromecast/public/media/decoder_config.h (right): https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... chromecast/public/media/decoder_config.h:179: SMPTEST432_1 = 12, On 2016/09/23 17:31:05, halliwell wrote: > Do we need all these values? It seems like a lot of copy/paste. Also is there > a risk of getting out of sync with color_space.h ? Strictly speaking we don't need all those values atm, but strobe@ mentioned that we might need other values besides just bt2020 and 709 in the future. Plus it's actually easier to maintain it this way, by simply copy-pasting, rather than cherry-picking some values. Similarly e.g. we don't need kCodecVC1 or kCodecMPEG2 values defined in the VideoCodec enum in this file, but still define them for consistency with Chromium. We can use status asserts to ensure the values match (at least the values that we care about, i.e. non Chrome-specific values defined in WebM spec. Added those asserts in .cpp file.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/23 18:28:08, servolk wrote: > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > File chromecast/media/cma/base/decoder_config_adapter.cc (right): > > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > chromecast/media/cma/base/decoder_config_adapter.cc:241: // space components. > We'll need to way to fix this. > On 2016/09/23 17:31:05, halliwell wrote: > > Can you file a bug to not forget this and add the bug ID here? > > Done. > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > File chromecast/public/media/decoder_config.h (right): > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > chromecast/public/media/decoder_config.h:179: SMPTEST432_1 = 12, > On 2016/09/23 17:31:05, halliwell wrote: > > Do we need all these values? It seems like a lot of copy/paste. Also is > there > > a risk of getting out of sync with color_space.h ? > > Strictly speaking we don't need all those values atm, but strobe@ mentioned that > we might need other values besides just bt2020 and 709 in the future. Plus it's > actually easier to maintain it this way, by simply copy-pasting, rather than > cherry-picking some values. > Similarly e.g. we don't need kCodecVC1 or kCodecMPEG2 values defined in the > VideoCodec enum in this file, but still define them for consistency with > Chromium. > We can use status asserts to ensure the values match (at least the values that > we care about, i.e. non Chrome-specific values defined in WebM spec. Added those > asserts in .cpp file. Thanks, lgtm :)
The CQ bit was unchecked by servolk@chromium.org
The CQ bit was checked by servolk@chromium.org
On 2016/09/23 18:30:38, halliwell wrote: > On 2016/09/23 18:28:08, servolk wrote: > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > > File chromecast/media/cma/base/decoder_config_adapter.cc (right): > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > > chromecast/media/cma/base/decoder_config_adapter.cc:241: // space components. > > We'll need to way to fix this. > > On 2016/09/23 17:31:05, halliwell wrote: > > > Can you file a bug to not forget this and add the bug ID here? > > > > Done. > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > > File chromecast/public/media/decoder_config.h (right): > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > > chromecast/public/media/decoder_config.h:179: SMPTEST432_1 = 12, > > On 2016/09/23 17:31:05, halliwell wrote: > > > Do we need all these values? It seems like a lot of copy/paste. Also is > > there > > > a risk of getting out of sync with color_space.h ? > > > > Strictly speaking we don't need all those values atm, but strobe@ mentioned > that > > we might need other values besides just bt2020 and 709 in the future. Plus > it's > > actually easier to maintain it this way, by simply copy-pasting, rather than > > cherry-picking some values. > > Similarly e.g. we don't need kCodecVC1 or kCodecMPEG2 values defined in the > > VideoCodec enum in this file, but still define them for consistency with > > Chromium. > > We can use status asserts to ensure the values match (at least the values that > > we care about, i.e. non Chrome-specific values defined in WebM spec. Added > those > > asserts in .cpp file. > > Thanks, lgtm :) How long do we expect this patch to be? Remember that once this public/ change is in 1.22 It cannot be removed until next system update 2-3 quarters.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/23 18:32:50, gfhuang wrote: > On 2016/09/23 18:30:38, halliwell wrote: > > On 2016/09/23 18:28:08, servolk wrote: > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > > > File chromecast/media/cma/base/decoder_config_adapter.cc (right): > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > > > chromecast/media/cma/base/decoder_config_adapter.cc:241: // space > components. > > > We'll need to way to fix this. > > > On 2016/09/23 17:31:05, halliwell wrote: > > > > Can you file a bug to not forget this and add the bug ID here? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > > > File chromecast/public/media/decoder_config.h (right): > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > > > chromecast/public/media/decoder_config.h:179: SMPTEST432_1 = 12, > > > On 2016/09/23 17:31:05, halliwell wrote: > > > > Do we need all these values? It seems like a lot of copy/paste. Also is > > > there > > > > a risk of getting out of sync with color_space.h ? > > > > > > Strictly speaking we don't need all those values atm, but strobe@ mentioned > > that > > > we might need other values besides just bt2020 and 709 in the future. Plus > > it's > > > actually easier to maintain it this way, by simply copy-pasting, rather than > > > cherry-picking some values. > > > Similarly e.g. we don't need kCodecVC1 or kCodecMPEG2 values defined in the > > > VideoCodec enum in this file, but still define them for consistency with > > > Chromium. > > > We can use status asserts to ensure the values match (at least the values > that > > > we care about, i.e. non Chrome-specific values defined in WebM spec. Added > > those > > > asserts in .cpp file. > > > > Thanks, lgtm :) > > How long do we expect this patch to be? > > Remember that once this public/ change is in 1.22 > It cannot be removed until next system update 2-3 quarters. I don't think we'll ever want to remove this. We'll need this for HDR support in the foreseeable future.
On 2016/09/23 18:35:17, servolk wrote: > On 2016/09/23 18:32:50, gfhuang wrote: > > On 2016/09/23 18:30:38, halliwell wrote: > > > On 2016/09/23 18:28:08, servolk wrote: > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > > > > File chromecast/media/cma/base/decoder_config_adapter.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > > > > chromecast/media/cma/base/decoder_config_adapter.cc:241: // space > > components. > > > > We'll need to way to fix this. > > > > On 2016/09/23 17:31:05, halliwell wrote: > > > > > Can you file a bug to not forget this and add the bug ID here? > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > > > > File chromecast/public/media/decoder_config.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > > > > chromecast/public/media/decoder_config.h:179: SMPTEST432_1 = 12, > > > > On 2016/09/23 17:31:05, halliwell wrote: > > > > > Do we need all these values? It seems like a lot of copy/paste. Also > is > > > > there > > > > > a risk of getting out of sync with color_space.h ? > > > > > > > > Strictly speaking we don't need all those values atm, but strobe@ > mentioned > > > that > > > > we might need other values besides just bt2020 and 709 in the future. Plus > > > it's > > > > actually easier to maintain it this way, by simply copy-pasting, rather > than > > > > cherry-picking some values. > > > > Similarly e.g. we don't need kCodecVC1 or kCodecMPEG2 values defined in > the > > > > VideoCodec enum in this file, but still define them for consistency with > > > > Chromium. > > > > We can use status asserts to ensure the values match (at least the values > > that > > > > we care about, i.e. non Chrome-specific values defined in WebM spec. Added > > > those > > > > asserts in .cpp file. > > > > > > Thanks, lgtm :) > > > > How long do we expect this patch to be? > > > > Remember that once this public/ change is in 1.22 > > It cannot be removed until next system update 2-3 quarters. > > I don't think we'll ever want to remove this. We'll need this for HDR support in > the foreseeable future. Ok. if the chromium source code you copy/paste from changed, will it cause any issue if this code is out of sync? since we cannot change public/ freely
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add color space and HDR data to Chromecast VideoConfig BUG=b/31399384 ========== to ========== [Chromecast] Add color space and HDR data to Chromecast VideoConfig BUG=b/31399384 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
On 2016/09/23 18:36:35, gfhuang wrote: > On 2016/09/23 18:35:17, servolk wrote: > > On 2016/09/23 18:32:50, gfhuang wrote: > > > On 2016/09/23 18:30:38, halliwell wrote: > > > > On 2016/09/23 18:28:08, servolk wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > > > > > File chromecast/media/cma/base/decoder_config_adapter.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > > > > > chromecast/media/cma/base/decoder_config_adapter.cc:241: // space > > > components. > > > > > We'll need to way to fix this. > > > > > On 2016/09/23 17:31:05, halliwell wrote: > > > > > > Can you file a bug to not forget this and add the bug ID here? > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > > > > > File chromecast/public/media/decoder_config.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > > > > > chromecast/public/media/decoder_config.h:179: SMPTEST432_1 = 12, > > > > > On 2016/09/23 17:31:05, halliwell wrote: > > > > > > Do we need all these values? It seems like a lot of copy/paste. Also > > is > > > > > there > > > > > > a risk of getting out of sync with color_space.h ? > > > > > > > > > > Strictly speaking we don't need all those values atm, but strobe@ > > mentioned > > > > that > > > > > we might need other values besides just bt2020 and 709 in the future. > Plus > > > > it's > > > > > actually easier to maintain it this way, by simply copy-pasting, rather > > than > > > > > cherry-picking some values. > > > > > Similarly e.g. we don't need kCodecVC1 or kCodecMPEG2 values defined in > > the > > > > > VideoCodec enum in this file, but still define them for consistency with > > > > > Chromium. > > > > > We can use status asserts to ensure the values match (at least the > values > > > that > > > > > we care about, i.e. non Chrome-specific values defined in WebM spec. > Added > > > > those > > > > > asserts in .cpp file. > > > > > > > > Thanks, lgtm :) > > > > > > How long do we expect this patch to be? > > > > > > Remember that once this public/ change is in 1.22 > > > It cannot be removed until next system update 2-3 quarters. > > > > I don't think we'll ever want to remove this. We'll need this for HDR support > in > > the foreseeable future. > > Ok. if the chromium source code you copy/paste from changed, > will it cause any issue if this code is out of sync? > since we cannot change public/ freely The values that we care about are defined in public WebM spec (www.webmproject.org/docs/container/#colour), so they should never change.
Message was sent while issue was closed.
On 2016/09/23 18:42:21, servolk wrote: > On 2016/09/23 18:36:35, gfhuang wrote: > > On 2016/09/23 18:35:17, servolk wrote: > > > On 2016/09/23 18:32:50, gfhuang wrote: > > > > On 2016/09/23 18:30:38, halliwell wrote: > > > > > On 2016/09/23 18:28:08, servolk wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > > > > > > File chromecast/media/cma/base/decoder_config_adapter.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/media/cma/base/d... > > > > > > chromecast/media/cma/base/decoder_config_adapter.cc:241: // space > > > > components. > > > > > > We'll need to way to fix this. > > > > > > On 2016/09/23 17:31:05, halliwell wrote: > > > > > > > Can you file a bug to not forget this and add the bug ID here? > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > > > > > > File chromecast/public/media/decoder_config.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2368573002/diff/1/chromecast/public/media/dec... > > > > > > chromecast/public/media/decoder_config.h:179: SMPTEST432_1 = 12, > > > > > > On 2016/09/23 17:31:05, halliwell wrote: > > > > > > > Do we need all these values? It seems like a lot of copy/paste. > Also > > > is > > > > > > there > > > > > > > a risk of getting out of sync with color_space.h ? > > > > > > > > > > > > Strictly speaking we don't need all those values atm, but strobe@ > > > mentioned > > > > > that > > > > > > we might need other values besides just bt2020 and 709 in the future. > > Plus > > > > > it's > > > > > > actually easier to maintain it this way, by simply copy-pasting, > rather > > > than > > > > > > cherry-picking some values. > > > > > > Similarly e.g. we don't need kCodecVC1 or kCodecMPEG2 values defined > in > > > the > > > > > > VideoCodec enum in this file, but still define them for consistency > with > > > > > > Chromium. > > > > > > We can use status asserts to ensure the values match (at least the > > values > > > > that > > > > > > we care about, i.e. non Chrome-specific values defined in WebM spec. > > Added > > > > > those > > > > > > asserts in .cpp file. > > > > > > > > > > Thanks, lgtm :) > > > > > > > > How long do we expect this patch to be? > > > > > > > > Remember that once this public/ change is in 1.22 > > > > It cannot be removed until next system update 2-3 quarters. > > > > > > I don't think we'll ever want to remove this. We'll need this for HDR > support > > in > > > the foreseeable future. > > > > Ok. if the chromium source code you copy/paste from changed, > > will it cause any issue if this code is out of sync? > > since we cannot change public/ freely > > The values that we care about are defined in public WebM spec > (http://www.webmproject.org/docs/container/#colour), so they should never change. And these values are also in various ISO/IEC specs as well (ISO/IEC 23001-8:2013/DCOR1).
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add color space and HDR data to Chromecast VideoConfig BUG=b/31399384 ========== to ========== [Chromecast] Add color space and HDR data to Chromecast VideoConfig BUG=b/31399384 Committed: https://crrev.com/6926d672f6291d4e2d3730fa7b6453f7b566ab6c Cr-Commit-Position: refs/heads/master@{#420674} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6926d672f6291d4e2d3730fa7b6453f7b566ab6c Cr-Commit-Position: refs/heads/master@{#420674}
Message was sent while issue was closed.
This breaks dragonfly build. You should remove "#if defined(CHROMECAST_BUILD)" check putting this check in .h header is bad that you now need every OEMs to define this marco.
Message was sent while issue was closed.
On 2016/09/23 21:13:47, gfhuang wrote: > This breaks dragonfly build. > > You should remove > "#if defined(CHROMECAST_BUILD)" check > > putting this check in .h header is bad that you now need every OEMs to define > this marco. sorry, I mean the unfork_m52 patch |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
