Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1822103003:
Allow padding in WrapVideoFrameInCVPixelBuffer (Closed)
DescriptionAllow padding in WrapVideoFrameInCVPixelBuffer
This CL allows visible_rect and coded_size differences in
WrapVideoFrameInCVPixelBuffer.
BUG=596674
Committed: https://crrev.com/57ad17882fe82d222ffc20d8d03140c1afa366a8
Cr-Commit-Position: refs/heads/master@{#383662}
Patch Set 1 #
Total comments: 8
Patch Set 2 : miu@ comments. #
Total comments: 10
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Messages
Total messages: 21 (7 generated)
Description was changed from ========== handle visible rect. BUG= ========== to ========== Allow padding in WrapVideoFrameInCVPixelBuffer This CL allows visible_rect and coded_size differences in WrapVideoFrameInCVPixelBuffer. BUG=596674 ==========
emircan@chromium.org changed reviewers: + jfroy@chromium.org, miu@chromium.org
PTAL.
Does this actually deal with the data correctly? I imagine when visible rect != coded rect, you need to use a different row stride (bytes per row). In any case, I'd very much love to see the test(s) expanded to use a random image (can be random data) and check each row of each plane for equality.
On 2016/03/23 16:53:15, jfroy wrote: > Does this actually deal with the data correctly? I imagine when visible rect != > coded rect, you need to use a different row stride (bytes per row). FWICT, the stride is provided correctly on line 75 (plane_bytes_per_row). > In any case, I'd very much love to see the test(s) expanded to use a random > image (can be random data) and check each row of each plane for equality. Agree. Create a VideoFrame, populate it with a predictable pattern (not random data), wrap it into a CV pixel buffer, then read the data back from the CV pixel buffer checking that the predictable pattern matches and is in the correct position.
https://codereview.chromium.org/1822103003/diff/1/media/base/mac/video_frame_... File media/base/mac/video_frame_mac_unittests.cc (right): https://codereview.chromium.org/1822103003/diff/1/media/base/mac/video_frame_... media/base/mac/video_frame_mac_unittests.cc:136: TEST(VideoFrameMac, CheckFrameWithVisibleRect) { nit: Please rename to something more meaningful. Suggestion: CorrectlyWrapsFramesWithPadding https://codereview.chromium.org/1822103003/diff/1/media/base/mac/video_frame_... media/base/mac/video_frame_mac_unittests.cc:137: const gfx::Size size(kWidth, kHeight); nit: Please name this coded_size for clarity. https://codereview.chromium.org/1822103003/diff/1/media/base/mac/video_frame_... media/base/mac/video_frame_mac_unittests.cc:162: } Also, you should check the value returned by CVPixelBufferGetBytesPerRowOfPlane() matches the coded size (for the Y plane; 1/2 size for U and V). https://codereview.chromium.org/1822103003/diff/1/media/base/mac/video_frame_... media/base/mac/video_frame_mac_unittests.cc:162: } Per earlier discussion, I think it would be good to read the bytes in the plane, checking for an expected pattern.
https://codereview.chromium.org/1822103003/diff/1/media/base/mac/video_frame_... File media/base/mac/video_frame_mac_unittests.cc (right): https://codereview.chromium.org/1822103003/diff/1/media/base/mac/video_frame_... media/base/mac/video_frame_mac_unittests.cc:136: TEST(VideoFrameMac, CheckFrameWithVisibleRect) { On 2016/03/24 20:29:00, miu wrote: > nit: Please rename to something more meaningful. Suggestion: > CorrectlyWrapsFramesWithPadding Done. https://codereview.chromium.org/1822103003/diff/1/media/base/mac/video_frame_... media/base/mac/video_frame_mac_unittests.cc:137: const gfx::Size size(kWidth, kHeight); On 2016/03/24 20:29:00, miu wrote: > nit: Please name this coded_size for clarity. Done. https://codereview.chromium.org/1822103003/diff/1/media/base/mac/video_frame_... media/base/mac/video_frame_mac_unittests.cc:162: } On 2016/03/24 20:29:00, miu wrote: > Also, you should check the value returned by > CVPixelBufferGetBytesPerRowOfPlane() matches the coded size (for the Y plane; > 1/2 size for U and V). Ok, that should equals stride as well. https://codereview.chromium.org/1822103003/diff/1/media/base/mac/video_frame_... media/base/mac/video_frame_mac_unittests.cc:162: } On 2016/03/24 20:29:00, miu wrote: > Per earlier discussion, I think it would be good to read the bytes in the plane, > checking for an expected pattern. Done.
https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... File media/base/mac/video_frame_mac_unittests.cc (right): https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:136: static void FillFrameWithColumnIndices(const scoped_refptr<VideoFrame>& frame) { nit: Please pass the argument as const VideoFrame&. https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:139: VideoFrame::PlaneSize(frame->format(), i, frame->coded_size()); You meant frame->visible_rect().size() as the 3rd arg to PlaneSize(), right? https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:142: const uint8_t index = h * frame->stride(i) + w; nit: The |h * frame->stride(i)| part could be moved above, before this inner loop to avoid redundant computations. Being pedantic about the performance of unit tests is important since they hold up the bots, CQ, etc. ;-) https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:143: frame->data(i)[index] = w; You meant to call visible_data() here, right? https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:184: const uint8_t index = h * stride + w; ditto: Please move |h * stride| part of the calculation to the outer loop.
https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... File media/base/mac/video_frame_mac_unittests.cc (right): https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:136: static void FillFrameWithColumnIndices(const scoped_refptr<VideoFrame>& frame) { On 2016/03/25 21:38:37, miu wrote: > nit: Please pass the argument as const VideoFrame&. Done. Note that I need to const_cast the data() ptrs in that case. https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:139: VideoFrame::PlaneSize(frame->format(), i, frame->coded_size()); On 2016/03/25 21:38:37, miu wrote: > You meant frame->visible_rect().size() as the 3rd arg to PlaneSize(), right? Actually, I wanted to fill intentionally the whole coded_size. Visible rect starts from kVisibleRectOffset and if there is an shift, it is very much visible from values. https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:142: const uint8_t index = h * frame->stride(i) + w; On 2016/03/25 21:38:37, miu wrote: > nit: The |h * frame->stride(i)| part could be moved above, before this inner > loop to avoid redundant computations. Being pedantic about the performance of > unit tests is important since they hold up the bots, CQ, etc. ;-) Done. https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:143: frame->data(i)[index] = w; On 2016/03/25 21:38:37, miu wrote: > You meant to call visible_data() here, right? I would rather fill up the whole coded_size() as I explained above. https://codereview.chromium.org/1822103003/diff/20001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:184: const uint8_t index = h * stride + w; On 2016/03/25 21:38:37, miu wrote: > ditto: Please move |h * stride| part of the calculation to the outer loop. Done.
lgtm % a few important things I missed the last time around: https://codereview.chromium.org/1822103003/diff/40001/media/base/mac/video_fr... File media/base/mac/video_frame_mac_unittests.cc (right): https://codereview.chromium.org/1822103003/diff/40001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:145: plane_ptr[index] = w; We should check vertical alignment as well. Can we make this: plane_ptr[index] = static_cast<uint8_t>(h ^ w); (and change the name of this function to something like FillFrameWithPredictableValues) https://codereview.chromium.org/1822103003/diff/40001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:157: visible_rect, coded_size, kTimestamp); Looks like the 4th argument should be visible_rect.size(). Natural size is almost always the visible size, unless the pixels in the video frame are not perfect squares (i.e., no aspect ratio or scaling adjustment). https://codereview.chromium.org/1822103003/diff/40001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:188: EXPECT_EQ(frame->visible_data(i)[index], The statement on line 180 means this LOC is essentially useless (you're comparing the values at identical locations in memory). Instead, can we make this: EXPECT_EQ(static_cast<uint8_t>((h + kVisibleRectOffset) ^ (w + kVisibleRectOffset)), plane_ptr[index]);
https://codereview.chromium.org/1822103003/diff/40001/media/base/mac/video_fr... File media/base/mac/video_frame_mac_unittests.cc (right): https://codereview.chromium.org/1822103003/diff/40001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:145: plane_ptr[index] = w; On 2016/03/25 23:53:49, miu wrote: > We should check vertical alignment as well. Can we make this: > > plane_ptr[index] = static_cast<uint8_t>(h ^ w); > > (and change the name of this function to something like > FillFrameWithPredictableValues) Done. https://codereview.chromium.org/1822103003/diff/40001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:157: visible_rect, coded_size, kTimestamp); On 2016/03/25 23:53:49, miu wrote: > Looks like the 4th argument should be visible_rect.size(). Natural size is > almost always the visible size, unless the pixels in the video frame are not > perfect squares (i.e., no aspect ratio or scaling adjustment). Done. https://codereview.chromium.org/1822103003/diff/40001/media/base/mac/video_fr... media/base/mac/video_frame_mac_unittests.cc:188: EXPECT_EQ(frame->visible_data(i)[index], On 2016/03/25 23:53:49, miu wrote: > The statement on line 180 means this LOC is essentially useless (you're > comparing the values at identical locations in memory). Instead, can we make > this: > > EXPECT_EQ(static_cast<uint8_t>((h + kVisibleRectOffset) ^ (w + > kVisibleRectOffset)), plane_ptr[index]); Done.
emircan@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis@, RS OWNERS review pls?
lgtm
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org Link to the patchset: https://codereview.chromium.org/1822103003/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1822103003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1822103003/60001
Message was sent while issue was closed.
Description was changed from ========== Allow padding in WrapVideoFrameInCVPixelBuffer This CL allows visible_rect and coded_size differences in WrapVideoFrameInCVPixelBuffer. BUG=596674 ========== to ========== Allow padding in WrapVideoFrameInCVPixelBuffer This CL allows visible_rect and coded_size differences in WrapVideoFrameInCVPixelBuffer. BUG=596674 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Allow padding in WrapVideoFrameInCVPixelBuffer This CL allows visible_rect and coded_size differences in WrapVideoFrameInCVPixelBuffer. BUG=596674 ========== to ========== Allow padding in WrapVideoFrameInCVPixelBuffer This CL allows visible_rect and coded_size differences in WrapVideoFrameInCVPixelBuffer. BUG=596674 Committed: https://crrev.com/57ad17882fe82d222ffc20d8d03140c1afa366a8 Cr-Commit-Position: refs/heads/master@{#383662} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/57ad17882fe82d222ffc20d8d03140c1afa366a8 Cr-Commit-Position: refs/heads/master@{#383662} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
