Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1125303005:
cc: VideoLayerImpl VideoFrame::TEXTURE_YUV_420 support. (Closed)
|
Created:
5 years, 7 months ago by Daniele Castagna Modified:
5 years, 7 months ago CC:
cc-bugs_chromium.org, chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: VideoLayerImpl VideoFrame::TEXTURE_YUV_420 support.
VideoFrame can carry YUV native textures.
After crrev.com/1127423006 VideoResoucesUpdater will create the
appropriate VideoFrameExternalResources.
This change makes VideoLayerImpl deal with a VideoFrame with a
NATIVE_TEXTURE format and a TEXTURE_YUV_420 texture format.
BUG=485859
Committed: https://crrev.com/ea0f35165bfac778916a3e567f31516b12b8fe9d
Cr-Commit-Position: refs/heads/master@{#330017}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address hendrikw@'s comments. #
Total comments: 10
Patch Set 3 : Address danakj@'s comments. #Patch Set 4 : No alpha for native textures. DCHECK_IMPLIES. #Messages
Total messages: 19 (5 generated)
dcastagna@chromium.org changed reviewers: + reveman@chromium.org
dcastagna@chromium.org changed reviewers: + hendrikw@chromium.org
+hendrikw
https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl.cc File cc/layers/video_layer_impl.cc (right): https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl.... cc/layers/video_layer_impl.cc:232: gfx::Size ya_tex_size = coded_size; Not too important, but you could leave ya_tex_size constant. https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl_... File cc/layers/video_layer_impl_unittest.cc (right): https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl_... cc/layers/video_layer_impl_unittest.cc:280: } Should we also check that uv*2==ya here?
Thank you for the review! PTAL. https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl.cc File cc/layers/video_layer_impl.cc (right): https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl.... cc/layers/video_layer_impl.cc:232: gfx::Size ya_tex_size = coded_size; On 2015/05/14 at 22:11:19, hendrikw wrote: > Not too important, but you could leave ya_tex_size constant. Done. https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl_... File cc/layers/video_layer_impl_unittest.cc (right): https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl_... cc/layers/video_layer_impl_unittest.cc:280: } On 2015/05/14 at 22:11:19, hendrikw wrote: > Should we also check that uv*2==ya here? Sure, we have to downcast though, are you ok with that?
On 2015/05/14 22:23:04, Daniele Castagna wrote: > Thank you for the review! > > PTAL. > > https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl.cc > File cc/layers/video_layer_impl.cc (right): > > https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl.... > cc/layers/video_layer_impl.cc:232: gfx::Size ya_tex_size = coded_size; > On 2015/05/14 at 22:11:19, hendrikw wrote: > > Not too important, but you could leave ya_tex_size constant. > > Done. > > https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl_... > File cc/layers/video_layer_impl_unittest.cc (right): > > https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl_... > cc/layers/video_layer_impl_unittest.cc:280: } > On 2015/05/14 at 22:11:19, hendrikw wrote: > > Should we also check that uv*2==ya here? > > Sure, we have to downcast though, are you ok with that? Yeah, I am.
hendrikw@chromium.org changed reviewers: + danakj@chromium.org
On 2015/05/14 22:32:34, hendrikw wrote: > On 2015/05/14 22:23:04, Daniele Castagna wrote: > > Thank you for the review! > > > > PTAL. > > > > > https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl.cc > > File cc/layers/video_layer_impl.cc (right): > > > > > https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl.... > > cc/layers/video_layer_impl.cc:232: gfx::Size ya_tex_size = coded_size; > > On 2015/05/14 at 22:11:19, hendrikw wrote: > > > Not too important, but you could leave ya_tex_size constant. > > > > Done. > > > > > https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl_... > > File cc/layers/video_layer_impl_unittest.cc (right): > > > > > https://codereview.chromium.org/1125303005/diff/1/cc/layers/video_layer_impl_... > > cc/layers/video_layer_impl_unittest.cc:280: } > > On 2015/05/14 at 22:11:19, hendrikw wrote: > > > Should we also check that uv*2==ya here? > > > > Sure, we have to downcast though, are you ok with that? > > Yeah, I am. LGTM % danakj || reveman
https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... File cc/layers/video_layer_impl.cc (right): https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... cc/layers/video_layer_impl.cc:239: uv_tex_size.SetSize(ya_tex_size.width() / 2, ya_tex_size.height() / 2); is this right if coded_size is odd? https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... cc/layers/video_layer_impl.cc:246: if (frame_resources_.size() > 3) { should this not also be true for NATIVE_TEXTURE? Y and A should be the same size too? https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... File cc/layers/video_layer_impl_unittest.cc (right): https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... cc/layers/video_layer_impl_unittest.cc:251: TEST(VideoLayerImplTest, NativeYUVFrameGeneratesYUVQuad) { whitespace https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... cc/layers/video_layer_impl_unittest.cc:262: media::VideoFrame::WrapYUV420NativeTextures( mind adding a test for the non-native path too while you're doing this?
https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... File cc/layers/video_layer_impl.cc (right): https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... cc/layers/video_layer_impl.cc:239: uv_tex_size.SetSize(ya_tex_size.width() / 2, ya_tex_size.height() / 2); On 2015/05/14 at 23:08:33, danakj wrote: > is this right if coded_size is odd? No. Done. https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... cc/layers/video_layer_impl.cc:246: if (frame_resources_.size() > 3) { On 2015/05/14 at 23:08:33, danakj wrote: > should this not also be true for NATIVE_TEXTURE? Y and A should be the same size too? NATIVE_TEXTURE doesn't support alpha channel right now. :/ The only texture format is media::VideoFrame::TEXTURE_YUV_420 that has no alpha. https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... File cc/layers/video_layer_impl_unittest.cc (right): https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... cc/layers/video_layer_impl_unittest.cc:251: TEST(VideoLayerImplTest, NativeYUVFrameGeneratesYUVQuad) { On 2015/05/14 at 23:08:33, danakj wrote: > whitespace Blank line missing? Done. https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... cc/layers/video_layer_impl_unittest.cc:262: media::VideoFrame::WrapYUV420NativeTextures( On 2015/05/14 at 23:08:33, danakj wrote: > mind adding a test for the non-native path too while you're doing this? Done.
https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... File cc/layers/video_layer_impl.cc (right): https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... cc/layers/video_layer_impl.cc:246: if (frame_resources_.size() > 3) { On 2015/05/14 23:26:20, Daniele Castagna wrote: > On 2015/05/14 at 23:08:33, danakj wrote: > > should this not also be true for NATIVE_TEXTURE? Y and A should be the same > size too? > > NATIVE_TEXTURE doesn't support alpha channel right now. :/ > The only texture format is media::VideoFrame::TEXTURE_YUV_420 that has no alpha. How about move this out of the else, and DCHECK that frame_resources_.size() == 3 in the other block with a comment that alpha isn't supported. Also you could make this a DCHECK_IMPLIES and get rid of the if() code outside of the DCHECK.
https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... File cc/layers/video_layer_impl.cc (right): https://codereview.chromium.org/1125303005/diff/20001/cc/layers/video_layer_i... cc/layers/video_layer_impl.cc:246: if (frame_resources_.size() > 3) { On 2015/05/14 at 23:33:56, danakj wrote: > On 2015/05/14 23:26:20, Daniele Castagna wrote: > > On 2015/05/14 at 23:08:33, danakj wrote: > > > should this not also be true for NATIVE_TEXTURE? Y and A should be the same > > size too? > > > > NATIVE_TEXTURE doesn't support alpha channel right now. :/ > > The only texture format is media::VideoFrame::TEXTURE_YUV_420 that has no alpha. > > How about move this out of the else, and DCHECK that frame_resources_.size() == 3 in the other block with a comment that alpha isn't supported. > > Also you could make this a DCHECK_IMPLIES and get rid of the if() code outside of the DCHECK. Added DCHECK_EQ(3u, frame_resources_.size()) Replaced the if with DCHECK_IMPLIES. Then DCHECK_IMPLIES wouldn't work out of the else since VideoFrame::NumPlanes() is not defined for NATIVE_TEXTURE, VideoFrame::NumTextures() is.
LGTM
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hendrikw@chromium.org Link to the patchset: https://codereview.chromium.org/1125303005/#ps60001 (title: "No alpha for native textures. DCHECK_IMPLIES.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1125303005/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ea0f35165bfac778916a3e567f31516b12b8fe9d Cr-Commit-Position: refs/heads/master@{#330017} |
