Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1913503002:
Memory copy the VideoFrame to match the requirement for HW encoders. (Closed)
|
Created:
4 years, 8 months ago by xjz Modified:
4 years, 7 months ago CC:
chromium-reviews, imcheng+watch_chromium.org, jasonroberts+watch_google.com, avayvod+watch_chromium.org, feature-media-reviews_chromium.org, xjz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMemory copy the VideoFrame to match the requirement for HW encoders.
BUG=578938
Committed: https://crrev.com/957fd159c32d6a62718993e9758a9a5c276e000a
Cr-Commit-Position: refs/heads/master@{#392098}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Addressed comments. #
Total comments: 4
Patch Set 3 : Used shared memory for encoder input buffer. #
Total comments: 36
Patch Set 4 : Addressed comments. #
Total comments: 12
Patch Set 5 : Addressed comments and rebased. #
Total comments: 4
Patch Set 6 : Addressed comments. #
Total comments: 10
Patch Set 7 : Add unit test and addressed Xiaohan's comments. #
Total comments: 14
Patch Set 8 : Addressed Xiaohan's comments. #
Messages
Total messages: 34 (13 generated)
Patchset #1 (id:1) has been deleted
xjz@chromium.org changed reviewers: + miu@chromium.org
PTAL
https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:76: bool StridedFrameCopy(const scoped_refptr<media::VideoFrame> src_frame, Oh! Oops, I forgot to mention these routines already exist. Sorry I didn't mention this before you wrote them. Suggestion: #include "third_party/libyuv/include/libyuv/convert.h" ... libyuv::I420Copy(...) Reference: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:220: VLOG(1) << "ExternalVideoEncoder copy required. " nit: Maybe make these logging statements VLOG(2) since they can occur at high rates (>1 log lines per second)? Also, perhaps these should be DVLOG(2) (i.e., only built into debug builds)? https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:271: for (size_t j = 0; j < kOutputBufferCount; ++j) { BTW--In a follow-up change, perhaps you could take a look at this? My gut says we should be respecting |input_count| instead of using our own hard-coded one.
Addressed comments. PTAL. https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:76: bool StridedFrameCopy(const scoped_refptr<media::VideoFrame> src_frame, On 2016/04/25 23:39:39, miu wrote: > Oh! Oops, I forgot to mention these routines already exist. Sorry I didn't > mention this before you wrote them. > > Suggestion: > > #include "third_party/libyuv/include/libyuv/convert.h" > > ... > > libyuv::I420Copy(...) > > Reference: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... Done. Added dependency on libyuv. This copy does not do any padding. So I now initialize the new frame with 0 when it is created. https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:220: VLOG(1) << "ExternalVideoEncoder copy required. " On 2016/04/25 23:39:39, miu wrote: > nit: Maybe make these logging statements VLOG(2) since they can occur at high > rates (>1 log lines per second)? Also, perhaps these should be DVLOG(2) (i.e., > only built into debug builds)? Removed this log. https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:271: for (size_t j = 0; j < kOutputBufferCount; ++j) { On 2016/04/25 23:39:39, miu wrote: > BTW--In a follow-up change, perhaps you could take a look at this? My gut says > we should be respecting |input_count| instead of using our own hard-coded one. |input_count| is how many frames the encoder will hold. It is for input frames. We ignore this value here as we don't use fixed number of buffers for input frames.|kOutputBufferCount| is how many output buffers to be allocated. This is for the encoder output. Removed this TODO comment.
https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:76: bool StridedFrameCopy(const scoped_refptr<media::VideoFrame> src_frame, On 2016/04/26 17:42:30, xjz wrote: > On 2016/04/25 23:39:39, miu wrote: > > Oh! Oops, I forgot to mention these routines already exist. Sorry I didn't > > mention this before you wrote them. > > > > Suggestion: > > > > #include "third_party/libyuv/include/libyuv/convert.h" > > > > ... > > > > libyuv::I420Copy(...) > > > > Reference: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... > > Done. Added dependency on libyuv. This copy does not do any padding. So I now > initialize the new frame with 0 when it is created. Oh. IMO, the padding was a nice thing to have. Also, it would be useful to place this function (I420 copy + padding) in src/media/base/video_util.cc so others can use it. https://codereview.chromium.org/1913503002/diff/40001/media/cast/cast.gyp File media/cast/cast.gyp (right): https://codereview.chromium.org/1913503002/diff/40001/media/cast/cast.gyp#new... media/cast/cast.gyp:149: '<(DEPTH)/third_party/libyuv/libyuv.gyp:libyuv', Please update BUILD.gn as well. https://codereview.chromium.org/1913503002/diff/40001/media/cast/sender/exter... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/40001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:154: if (video_frame->coded_size() != frame_coded_size_) { For safety, we should probably: DCHECK_GE(frame_coded_size_.width, visible_rect.width); DCHECK_GE(frame_coded_size_.height, visible_rect.height); -or- I'm not sure whether there are transient conditions where the frame_coded_size_ might be wrong, but we should just drop frames and expect frame_coded_size_ to be set correctly in the near future.
PTAL. Changes in PS#3: 1. Copy input frames to shared memory, as the encoder requires the input be in shared memory. 2. Add padding and moved the copy/padding functions to media/base/video_util.*. https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/20001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:76: bool StridedFrameCopy(const scoped_refptr<media::VideoFrame> src_frame, On 2016/04/26 19:20:17, miu wrote: > On 2016/04/26 17:42:30, xjz wrote: > > On 2016/04/25 23:39:39, miu wrote: > > > Oh! Oops, I forgot to mention these routines already exist. Sorry I didn't > > > mention this before you wrote them. > > > > > > Suggestion: > > > > > > #include "third_party/libyuv/include/libyuv/convert.h" > > > > > > ... > > > > > > libyuv::I420Copy(...) > > > > > > Reference: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv... > > > > Done. Added dependency on libyuv. This copy does not do any padding. So I now > > initialize the new frame with 0 when it is created. > > Oh. IMO, the padding was a nice thing to have. Also, it would be useful to > place this function (I420 copy + padding) in src/media/base/video_util.cc so > others can use it. Done. Padding is nice to improve coding efficiency. However I am not sure if it might cause performance issue, as it increases encoding time and may result in lower quality (by zero configuring). https://codereview.chromium.org/1913503002/diff/40001/media/cast/cast.gyp File media/cast/cast.gyp (right): https://codereview.chromium.org/1913503002/diff/40001/media/cast/cast.gyp#new... media/cast/cast.gyp:149: '<(DEPTH)/third_party/libyuv/libyuv.gyp:libyuv', On 2016/04/26 19:20:17, miu wrote: > Please update BUILD.gn as well. Not applicable. Moved the copy and padding to media/base/video_util.*. https://codereview.chromium.org/1913503002/diff/40001/media/cast/sender/exter... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/40001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:154: if (video_frame->coded_size() != frame_coded_size_) { On 2016/04/26 19:20:17, miu wrote: > For safety, we should probably: > > DCHECK_GE(frame_coded_size_.width, visible_rect.width); > DCHECK_GE(frame_coded_size_.height, visible_rect.height); > > -or- > > I'm not sure whether there are transient conditions where the frame_coded_size_ > might be wrong, but we should just drop frames and expect frame_coded_size_ to > be set correctly in the near future. Done.
https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.cc File media/base/video_util.cc (right): https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:344: void Padding(uint8_t* frame, Please put this function in an anonymous namespace. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:344: void Padding(uint8_t* frame, naming nit: ApplyPaddingOutsideVisibleRegion() or something a bit more descriptive? https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:347: if (!frame) This should go at the top of I420CopyWithPadding() (and return false from there). https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:351: if (visible_size.width() < stride) { To avoid the possibility of copying from an empty visible region, consider adding: if (... && !visible_size.IsEmpty()) { https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:358: if (visible_size.height() < frame_size.height()) { Here too: if (... && !visible_size.IsEmpty()) { https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:369: DCHECK_GE(dst_frame->coded_size().height(), Looks like this code assumes dst_frame->visible_rect().origin() is [0,0]. This is reasonable, but you may want to add a DCHECK for that. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:372: if (libyuv::I420Copy(src_frame->data(media::VideoFrame::kYPlane), Please replace all data() calls with visible_data() calls for the I420Copy() args, since it's the visible region we intend to copy here. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:391: const gfx::Size uv_visible_size(src_frame->visible_rect().width() / 2, Is this correct for odd widths/heights? The logic in media::VideoFrame rounds up. Either way, I'd suggest you call the media::VideoFrame::PlaneSize() static function to calculate this. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.h File media/base/video_util.h (right): https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.h... media/base/video_util.h:101: // with the last column / row. Comment should also mention: 1) The visible region in both frames should be identical; 2) When/why would this function would return true or false. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.h... media/base/video_util.h:102: MEDIA_EXPORT bool I420CopyWithPadding(const scoped_refptr<VideoFrame> src_frame, Type of the args should be: const VideoFrame& src_frame, // because it's read-only and a non-null frame is required VideoFrame* dst_frame, // because nothing will take ownership of dst_frame https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.h... media/base/video_util.h:103: scoped_refptr<VideoFrame> dst_frame); Consider adding WARN_UNUSED_RESULT in the function signature. This will force callers of this function to examine the return boolean. Meaning: MEDIA_EXPORT bool I420CopyWithPadding(...) WARN_UNUSED_RESULT; https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:147: void OnFinishEncodeFrame(int index) { naming nit: Since this is only used when we have to copy the VideoFrame, can we name this something more-specific, like ReturnInputBufferToPool() or MakeInputBufferAvailable(). https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:162: return; There are a bunch of early returns in this method. However, |frame_encoded_callback| must *always* be run in order for VideoSender to correctly account for the backlog in the encoder. This early return here is kind of bad, but acceptable because the encoder is permanently stopped (during shutdown). The rest that you've added will result in the |frame_encoded_callback| never being run. There's another issue here: VideoSender expects that, if the call to ExternalVideoEncoder::EncodeVideoFrame() returns true, then it will always get back an encoded frame, eventually. So, OTOH, we need to do two things to support the extra failure possibilities: 1. For the two early returns in this method, add the following before the return statement: std::unique_ptr<SenderEncodedFrame> no_result(nullptr); cast_environment_->PostTask(CastEnvironment::MAIN, FROM_HERE, base::Bind(in_progress_frame_encodes_.back().frame_encoded_callback, base::Passed(&no_result))); in_progress_frame_encodes_.pop_back(); return; Since that's a bit of code to duplicate, you should merge the two failure points into one, with: if (!frame || !media::I420CopyWithPadding(video_frame, frame)) { LOG(DFATAL) << "..."; <post task with nullptr result here> return; } 2. In VideoSender::OnEncodedVideoFrame(), if it is called with nullptr for the |encoded_frame|, it should return early (i.e., after it decrements the |frames_in_encoder_| counter. https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:185: VLOG(1) << "Error: ExternalVideoEncoder: CreateFrame failed."; This should probably be LOG(DFATAL) since either there's a coding bug or an OOM. For debug builds, we assume a coding bug; for release builds, an error log message is suitable to report out-of-memory. https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:191: VLOG(1) << "ERROR: ExternalVideoEncoder: Copy failed."; ditto: LOG(DFATAL). https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:234: for (size_t j = 0; j < input_count + kExtraInputBufferCount; ++j) { Instead of allocating these up-front, can we allocate on-demand, and only when the coded_size is not a match? Also, instead of allocating all of them, we should only allocate as many as are needed (i.e., one at a time, whenever the |free_input_buffer_index_| list is empty). We want to be as memory-efficient as possible. :) https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:538: ScopedVector<base::SharedMemory> input_buffers_; ScopedVector is deprecated. Please use std::vector<std::unique_ptr<base::SharedMemory>> instead. https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:621: if (!client_->IsInputBufferReady()) See comments above. If input buffers are allocated on-demand, this isn't needed anymore.
Patchset #4 (id:80001) has been deleted
Addressed comments. PTAL https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.cc File media/base/video_util.cc (right): https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:344: void Padding(uint8_t* frame, On 2016/04/30 00:41:14, miu wrote: > naming nit: ApplyPaddingOutsideVisibleRegion() or something a bit more > descriptive? Done. Renamed as FillRegionOutsideVisibleRect(). https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:344: void Padding(uint8_t* frame, On 2016/04/30 00:41:13, miu wrote: > Please put this function in an anonymous namespace. Done. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:347: if (!frame) On 2016/04/30 00:41:13, miu wrote: > This should go at the top of I420CopyWithPadding() (and return false from > there). Done. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:351: if (visible_size.width() < stride) { On 2016/04/30 00:41:13, miu wrote: > To avoid the possibility of copying from an empty visible region, consider > adding: > > if (... && !visible_size.IsEmpty()) { Done. Add early return when the visible region is empty. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:358: if (visible_size.height() < frame_size.height()) { On 2016/04/30 00:41:13, miu wrote: > Here too: > > if (... && !visible_size.IsEmpty()) { Add early return when the visible region is empty. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:369: DCHECK_GE(dst_frame->coded_size().height(), On 2016/04/30 00:41:14, miu wrote: > Looks like this code assumes dst_frame->visible_rect().origin() is [0,0]. This > is reasonable, but you may want to add a DCHECK for that. Done. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:372: if (libyuv::I420Copy(src_frame->data(media::VideoFrame::kYPlane), On 2016/04/30 00:41:13, miu wrote: > Please replace all data() calls with visible_data() calls for the I420Copy() > args, since it's the visible region we intend to copy here. Done. Though I think in our application src_frame->visible_rect().origion() should also be [0, 0]. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.c... media/base/video_util.cc:391: const gfx::Size uv_visible_size(src_frame->visible_rect().width() / 2, On 2016/04/30 00:41:14, miu wrote: > Is this correct for odd widths/heights? The logic in media::VideoFrame rounds > up. Either way, I'd suggest you call the media::VideoFrame::PlaneSize() static > function to calculate this. Done. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.h File media/base/video_util.h (right): https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.h... media/base/video_util.h:101: // with the last column / row. On 2016/04/30 00:41:14, miu wrote: > Comment should also mention: 1) The visible region in both frames should be > identical; 2) When/why would this function would return true or false. Done. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.h... media/base/video_util.h:102: MEDIA_EXPORT bool I420CopyWithPadding(const scoped_refptr<VideoFrame> src_frame, On 2016/04/30 00:41:14, miu wrote: > Type of the args should be: > > const VideoFrame& src_frame, // because it's read-only and a non-null frame is > required > VideoFrame* dst_frame, // because nothing will take ownership of dst_frame Done. https://codereview.chromium.org/1913503002/diff/60001/media/base/video_util.h... media/base/video_util.h:103: scoped_refptr<VideoFrame> dst_frame); On 2016/04/30 00:41:14, miu wrote: > Consider adding WARN_UNUSED_RESULT in the function signature. This will force > callers of this function to examine the return boolean. Meaning: > > MEDIA_EXPORT bool I420CopyWithPadding(...) WARN_UNUSED_RESULT; Done. https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:147: void OnFinishEncodeFrame(int index) { On 2016/04/30 00:41:14, miu wrote: > naming nit: Since this is only used when we have to copy the VideoFrame, can we > name this something more-specific, like ReturnInputBufferToPool() or > MakeInputBufferAvailable(). Done. https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:162: return; On 2016/04/30 00:41:14, miu wrote: > There are a bunch of early returns in this method. However, > |frame_encoded_callback| must *always* be run in order for VideoSender to > correctly account for the backlog in the encoder. > > This early return here is kind of bad, but acceptable because the encoder is > permanently stopped (during shutdown). The rest that you've added will result in > the |frame_encoded_callback| never being run. > > There's another issue here: VideoSender expects that, if the call to > ExternalVideoEncoder::EncodeVideoFrame() returns true, then it will always get > back an encoded frame, eventually. > > So, OTOH, we need to do two things to support the extra failure possibilities: > > 1. For the two early returns in this method, add the following before the return > statement: > > std::unique_ptr<SenderEncodedFrame> no_result(nullptr); > cast_environment_->PostTask(CastEnvironment::MAIN, FROM_HERE, > base::Bind(in_progress_frame_encodes_.back().frame_encoded_callback, > base::Passed(&no_result))); > in_progress_frame_encodes_.pop_back(); > return; > > Since that's a bit of code to duplicate, you should merge the two failure points > into one, with: > > if (!frame || !media::I420CopyWithPadding(video_frame, frame)) { > LOG(DFATAL) << "..."; > <post task with nullptr result here> > return; > } > > 2. In VideoSender::OnEncodedVideoFrame(), if it is called with nullptr for the > |encoded_frame|, it should return early (i.e., after it decrements the > |frames_in_encoder_| counter. Done. 1. Put all the necessary early return operation into a private function, since it needs to be called when the input buffer is not available too. 2. Also add early return to SizeAdaptableVideoEncoderBase::OnEncodedVideoFrame() when the input |encoded_frame| is nullptr. https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:185: VLOG(1) << "Error: ExternalVideoEncoder: CreateFrame failed."; On 2016/04/30 00:41:14, miu wrote: > This should probably be LOG(DFATAL) since either there's a coding bug or an OOM. > For debug builds, we assume a coding bug; for release builds, an error log > message is suitable to report out-of-memory. Done. https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:191: VLOG(1) << "ERROR: ExternalVideoEncoder: Copy failed."; On 2016/04/30 00:41:14, miu wrote: > ditto: LOG(DFATAL). Done. Combined this check with the above check on WrapExternalSharedMemory(). https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:234: for (size_t j = 0; j < input_count + kExtraInputBufferCount; ++j) { On 2016/04/30 00:41:14, miu wrote: > Instead of allocating these up-front, can we allocate on-demand, and only when > the coded_size is not a match? Also, instead of allocating all of them, we > should only allocate as many as are needed (i.e., one at a time, whenever the > |free_input_buffer_index_| list is empty). We want to be as memory-efficient as > possible. :) Done. https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:538: ScopedVector<base::SharedMemory> input_buffers_; On 2016/04/30 00:41:14, miu wrote: > ScopedVector is deprecated. Please use > std::vector<std::unique_ptr<base::SharedMemory>> instead. Done. https://codereview.chromium.org/1913503002/diff/60001/media/cast/sender/exter... media/cast/sender/external_video_encoder.cc:621: if (!client_->IsInputBufferReady()) On 2016/04/30 00:41:14, miu wrote: > See comments above. If input buffers are allocated on-demand, this isn't needed > anymore. Removed. The purpose of using this was actually trying to early exit the encoding when the buffer is not ready. This is not needed anymore since now the encoding is exited by calling |ExitEncodingWithErrors()|.
https://codereview.chromium.org/1913503002/diff/100001/media/base/video_util.cc File media/base/video_util.cc (right): https://codereview.chromium.org/1913503002/diff/100001/media/base/video_util.... media/base/video_util.cc:28: if (visible_size.IsEmpty()) This just occurred to me: If coded_size is non-empty, perhaps you should still memset(frame->data(x), 0, len) for each plane? https://codereview.chromium.org/1913503002/diff/100001/media/base/video_util.... media/base/video_util.cc:369: if (!dst_frame) Need to check for non-memory-backed VideoFrames: if (!dst_frame || !dst_frame->IsMappable()) return false; https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/exte... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/exte... media/cast/sender/external_video_encoder.cc:42: static const size_t kExtraInputBufferCount = 2; Note: They've just allowed the use of constexpr, a new C++11 construct. So, please use that for these constants: constexpr size_t kOutputBufferCount = 3; constexpr size_t kExtraInputBufferCount = 2; https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/exte... media/cast/sender/external_video_encoder.cc:175: if (input_buffers_.size() < max_allowed_input_buffers_) { It's unlikely, but possible, for this method to be called more than once before we get more input buffers created. This might cause the code to create more than the max allowed input buffers. https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/exte... media/cast/sender/external_video_encoder.cc:577: uint32_t max_allowed_input_buffers_; s/uint32_t/size_t/ https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/size... File media/cast/sender/size_adaptable_video_encoder_base.cc (right): https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/size... media/cast/sender/size_adaptable_video_encoder_base.cc:159: // Encoding was exited with errors. nit: Simplify: if (encoded_frame) last_frame_id_ = encoded_frame->frame_id; frame_encoded_callback.Run(std::move(encoded_frame));
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
PTAL https://codereview.chromium.org/1913503002/diff/100001/media/base/video_util.cc File media/base/video_util.cc (right): https://codereview.chromium.org/1913503002/diff/100001/media/base/video_util.... media/base/video_util.cc:28: if (visible_size.IsEmpty()) On 2016/05/03 19:05:43, miu wrote: > This just occurred to me: If coded_size is non-empty, perhaps you should still > memset(frame->data(x), 0, len) for each plane? Done. https://codereview.chromium.org/1913503002/diff/100001/media/base/video_util.... media/base/video_util.cc:369: if (!dst_frame) On 2016/05/03 19:05:43, miu wrote: > Need to check for non-memory-backed VideoFrames: > > if (!dst_frame || !dst_frame->IsMappable()) > return false; Done. https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/exte... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/exte... media/cast/sender/external_video_encoder.cc:42: static const size_t kExtraInputBufferCount = 2; On 2016/05/03 19:05:43, miu wrote: > Note: They've just allowed the use of constexpr, a new C++11 construct. So, > please use that for these constants: > > constexpr size_t kOutputBufferCount = 3; > constexpr size_t kExtraInputBufferCount = 2; Done. https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/exte... media/cast/sender/external_video_encoder.cc:175: if (input_buffers_.size() < max_allowed_input_buffers_) { On 2016/05/03 19:05:43, miu wrote: > It's unlikely, but possible, for this method to be called more than once before > we get more input buffers created. This might cause the code to create more than > the max allowed input buffers. Done. Changed to only allocate one buffer at one time. https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/exte... media/cast/sender/external_video_encoder.cc:577: uint32_t max_allowed_input_buffers_; On 2016/05/03 19:05:43, miu wrote: > s/uint32_t/size_t/ Done. https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/size... File media/cast/sender/size_adaptable_video_encoder_base.cc (right): https://codereview.chromium.org/1913503002/diff/100001/media/cast/sender/size... media/cast/sender/size_adaptable_video_encoder_base.cc:159: // Encoding was exited with errors. On 2016/05/03 19:05:43, miu wrote: > nit: Simplify: > > if (encoded_frame) > last_frame_id_ = encoded_frame->frame_id; > frame_encoded_callback.Run(std::move(encoded_frame)); Done.
lgtm % a couple minor things: https://codereview.chromium.org/1913503002/diff/160001/media/cast/sender/exte... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/160001/media/cast/sender/exte... media/cast/sender/external_video_encoder.cc:457: input_buffers_.push_back(std::move(memory)); Make sure to handle the case where |memory| is null (i.e., failed to alloc memory). You could probably just wrap this line and the next in an: if (memory.get()) { input_buffers... free_input_buffer_index... } allocate_input... = false; https://codereview.chromium.org/1913503002/diff/160001/media/cast/sender/exte... media/cast/sender/external_video_encoder.cc:584: bool allocate_input_buffer_in_process_; s/process/progress/ (and in the comment too, please)
xjz@chromium.org changed reviewers: + xhwang@chromium.org
xhwang: PTAL on media/base/*. https://codereview.chromium.org/1913503002/diff/160001/media/cast/sender/exte... File media/cast/sender/external_video_encoder.cc (right): https://codereview.chromium.org/1913503002/diff/160001/media/cast/sender/exte... media/cast/sender/external_video_encoder.cc:457: input_buffers_.push_back(std::move(memory)); On 2016/05/04 19:37:52, miu wrote: > Make sure to handle the case where |memory| is null (i.e., failed to alloc > memory). You could probably just wrap this line and the next in an: > > if (memory.get()) { > input_buffers... > free_input_buffer_index... > } > allocate_input... = false; Done. https://codereview.chromium.org/1913503002/diff/160001/media/cast/sender/exte... media/cast/sender/external_video_encoder.cc:584: bool allocate_input_buffer_in_process_; On 2016/05/04 19:37:52, miu wrote: > s/process/progress/ (and in the comment too, please) Done.
https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... File media/base/video_util.cc (right): https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... media/base/video_util.cc:378: DCHECK(dst_frame->visible_rect().origin().IsOrigin()); All these conditions should be documented in the header file. Otherwise people would have no idea how to use this API without looking at the implementation. https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... media/base/video_util.cc:380: if (libyuv::I420Copy(src_frame.visible_data(media::VideoFrame::kYPlane), Drop media:: since you are already in media namespace. https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... File media/base/video_util.h (right): https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... media/base/video_util.h:101: // outside visible rect repeatly with the last column / row. The required coded Could you please summarize why we want to convert a VideoFrame to one with larger coded size, and why we want to do the padding? https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... media/base/video_util.h:104: // successful. Also mention that this function could be expensive (performance-wise). https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... media/base/video_util.h:106: VideoFrame* dst_frame) WARN_UNUSED_RESULT; Could you please add some unittests?
Patchset #7 (id:200001) has been deleted
Add unit test and addressed Xiaohan's comments. PTAL. https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... File media/base/video_util.cc (right): https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... media/base/video_util.cc:378: DCHECK(dst_frame->visible_rect().origin().IsOrigin()); On 2016/05/04 21:08:29, xhwang wrote: > All these conditions should be documented in the header file. Otherwise people > would have no idea how to use this API without looking at the implementation. Done. https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... media/base/video_util.cc:380: if (libyuv::I420Copy(src_frame.visible_data(media::VideoFrame::kYPlane), On 2016/05/04 21:08:29, xhwang wrote: > Drop media:: since you are already in media namespace. Done. https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... File media/base/video_util.h (right): https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... media/base/video_util.h:101: // outside visible rect repeatly with the last column / row. The required coded On 2016/05/04 21:08:29, xhwang wrote: > Could you please summarize why we want to convert a VideoFrame to one with > larger coded size, and why we want to do the padding? Done. https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... media/base/video_util.h:104: // successful. On 2016/05/04 21:08:29, xhwang wrote: > Also mention that this function could be expensive (performance-wise). Done. https://chromiumcodereview.appspot.com/1913503002/diff/180001/media/base/vide... media/base/video_util.h:106: VideoFrame* dst_frame) WARN_UNUSED_RESULT; On 2016/05/04 21:08:29, xhwang wrote: > Could you please add some unittests? Done.
LGTM with nits https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... File media/base/video_util.cc (right): https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util.cc:25: void FillRegionOutsideVisibleRect(uint8_t* frame, This is a plane, not a frame. https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... File media/base/video_util_unittest.cc (right): https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:15: namespace { nit: move this whole anonymous namespace block into media namespace so you don't need "media::" in the code. https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:24: const gfx::Size& coded_size) { It's probably more readable if you s/visible_size/src_size and s/coded_size/dst_size. Then this function will become more like a VerifyPlaneCopyAndPadding or VerifyPlane https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:28: const size_t width = visible_size.width(); nit: src_width https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:43: } Use memcmp for this one? https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:44: uint8_t last_pixel = src_ptr[j - 1]; width - 1 might be a bit more readable. https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:48: } ditto about memcmp
Addressed Xiaohan's comments. Thanks for the review! https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... File media/base/video_util.cc (right): https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util.cc:25: void FillRegionOutsideVisibleRect(uint8_t* frame, On 2016/05/05 22:00:21, xhwang wrote: > This is a plane, not a frame. Done. Renamed it as |data|. https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... File media/base/video_util_unittest.cc (right): https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:15: namespace { On 2016/05/05 22:00:21, xhwang wrote: > nit: move this whole anonymous namespace block into media namespace so you don't > need "media::" in the code. These are helper functions. I think it might be better to keep them in the anonymous namespace. https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:24: const gfx::Size& coded_size) { On 2016/05/05 22:00:21, xhwang wrote: > It's probably more readable if you s/visible_size/src_size and > s/coded_size/dst_size. > > Then this function will become more like a VerifyPlaneCopyAndPadding or > VerifyPlane Done. https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:28: const size_t width = visible_size.width(); On 2016/05/05 22:00:21, xhwang wrote: > nit: src_width Done. https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:43: } On 2016/05/05 22:00:21, xhwang wrote: > Use memcmp for this one? Done. https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:44: uint8_t last_pixel = src_ptr[j - 1]; On 2016/05/05 22:00:21, xhwang wrote: > width - 1 might be a bit more readable. Done. https://chromiumcodereview.appspot.com/1913503002/diff/220001/media/base/vide... media/base/video_util_unittest.cc:48: } On 2016/05/05 22:00:21, xhwang wrote: > ditto about memcmp Done.
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, xhwang@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1913503002/#ps240001 (title: "Addressed Xiaohan's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913503002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913503002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by xjz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1913503002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1913503002/240001
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Memory copy the VideoFrame to match the requirement for HW encoders. BUG=578938 ========== to ========== Memory copy the VideoFrame to match the requirement for HW encoders. BUG=578938 Committed: https://crrev.com/957fd159c32d6a62718993e9758a9a5c276e000a Cr-Commit-Position: refs/heads/master@{#392098} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/957fd159c32d6a62718993e9758a9a5c276e000a Cr-Commit-Position: refs/heads/master@{#392098}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:240001) has been created in https://codereview.chromium.org/1955233002/ by thakis@chromium.org. The reason for reverting is: Turns msan red: https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Test... [ RUN ] VideoUtilTest.I420CopyWithPadding ==2446==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0xc369b7 in VerifyPlanCopyWithPadding media/base/video_util_unittest.cc:44:11 #1 0xc32802 in VerifyCopyWithPadding media/base/video_util_unittest.cc:62:8 #2 0xc307b5 in TestBody media/base/video_util_unittest.cc:468:3 .
Message was sent while issue was closed.
Patchset #9 (id:260001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
