Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2247403002:
Assign color spaces to video frames. (Closed)
|
Created:
4 years, 4 months ago by hubbe Modified:
4 years, 4 months ago CC:
ccameron, chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAssign color spaces to video frames.
BUG=622133
Committed: https://crrev.com/9e40ea52847b6303c81c6e2e64566791a2c960c8
Cr-Commit-Position: refs/heads/master@{#412429}
Patch Set 1 #
Total comments: 4
Patch Set 2 : test added #
Total comments: 4
Patch Set 3 : use static_assert #
Messages
Total messages: 20 (10 generated)
hubbe@chromium.org changed reviewers: + dalecurtis@google.com
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
https://codereview.chromium.org/2247403002/diff/1/media/filters/ffmpeg_video_... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/2247403002/diff/1/media/filters/ffmpeg_video_... media/filters/ffmpeg_video_decoder.cc:148: static_cast<gfx::ColorSpace::PrimaryID>(codec_context->color_primaries), Hmm, are you static_assert'ing that these values always line up? Are they standardized elsewhere? FFmpeg has a tendency to move such values around. https://codereview.chromium.org/2247403002/diff/1/media/filters/vpx_video_dec... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/2247403002/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder.cc:598: switch (vpx_image->cs) { Worth caching?
https://codereview.chromium.org/2247403002/diff/1/media/filters/ffmpeg_video_... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/2247403002/diff/1/media/filters/ffmpeg_video_... media/filters/ffmpeg_video_decoder.cc:148: static_cast<gfx::ColorSpace::PrimaryID>(codec_context->color_primaries), On 2016/08/16 18:58:03, DaleCurtis wrote: > Hmm, are you static_assert'ing that these values always line up? Are they > standardized elsewhere? FFmpeg has a tendency to move such values around. Added a test for it. The are standardized in h264 and h265, and they are apparently breaking them out into a new, standalone standard as well. The test will make sure ffmpeg actually follows the standard. :) https://codereview.chromium.org/2247403002/diff/1/media/filters/vpx_video_dec... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/2247403002/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder.cc:598: switch (vpx_image->cs) { On 2016/08/16 18:58:03, DaleCurtis wrote: > Worth caching? No, gfx::ColorSpace is super-lightweight unless you use custom transfer functions or primaries (which are not even supported yet.)
watk@chromium.org changed reviewers: + watk@chromium.org
https://codereview.chromium.org/2247403002/diff/20001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/2247403002/diff/20001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:587: ->SetInteger(VideoFrameMetadata::COLOR_SPACE, color_space); Is this stuff going to be removed in a follow up?
https://codereview.chromium.org/2247403002/diff/20001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/2247403002/diff/20001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:587: ->SetInteger(VideoFrameMetadata::COLOR_SPACE, color_space); On 2016/08/16 20:35:52, watk wrote: > Is this stuff going to be removed in a follow up? Yes. That will be quite a while though. (after benchmarks, experiments, launch and bake time...)
lgtm % switching to static_assert if possible. https://codereview.chromium.org/2247403002/diff/20001/media/filters/ffmpeg_vi... File media/filters/ffmpeg_video_decoder_unittest.cc (right): https://codereview.chromium.org/2247403002/diff/20001/media/filters/ffmpeg_vi... media/filters/ffmpeg_video_decoder_unittest.cc:366: TEST(GfxColorSpaceEnumTest, MakeSureEnumsLineUpWithFFMPEG) { You should be able to just do this with a static_assert() in ffmpeg_common.cc where we have the rest of the checks of this type IIRC.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2247403002/diff/20001/media/filters/ffmpeg_vi... File media/filters/ffmpeg_video_decoder_unittest.cc (right): https://codereview.chromium.org/2247403002/diff/20001/media/filters/ffmpeg_vi... media/filters/ffmpeg_video_decoder_unittest.cc:366: TEST(GfxColorSpaceEnumTest, MakeSureEnumsLineUpWithFFMPEG) { On 2016/08/16 22:22:17, DaleCurtis wrote: > You should be able to just do this with a static_assert() in ffmpeg_common.cc > where we have the rest of the checks of this type IIRC. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2247403002/#ps40001 (title: "use static_assert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Assign color spaces to video frames. BUG=622133 ========== to ========== Assign color spaces to video frames. BUG=622133 Committed: https://crrev.com/9e40ea52847b6303c81c6e2e64566791a2c960c8 Cr-Commit-Position: refs/heads/master@{#412429} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9e40ea52847b6303c81c6e2e64566791a2c960c8 Cr-Commit-Position: refs/heads/master@{#412429} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
