Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2711133002:
Step #1: Support pipelining codecs in VideoProcessor. (Closed)
DescriptionSupport pipelining codecs in VideoProcessor.
This CL removes most of the global frame state in VideoProcessor and
replaces that with a vector of frame states. This is useful for pipelining
codecs, where the encoded/decoded frame may not be immediately outputted
after it has been sent to the codec.
The callers (VideoProcessorIntegrationTest and video_quality_measurement)
still call VideoProcessor in a sequential fashion. A follow-up CL will be
submitted that enables batch mode in VideoProcessorIntegrationTest.
Note that VideoProcessor is still not thread safe. Currently, we can run
fairly well on Android due to the synchronicity of our MediaCodec wrapper,
but we still cannot run on iOS due to async issues. This will be fixed in
the future.
BUG=webrtc:6634
Review-Url: https://codereview.webrtc.org/2711133002
Cr-Commit-Position: refs/heads/master@{#17084}
Committed: https://chromium.googlesource.com/external/webrtc/+/17b958c041c519fef9d42113a556efc6931da87f
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 34
Patch Set 3 : clang-format #Patch Set 4 : asapersson comments 1. #Patch Set 5 : sprang comments 1. #Patch Set 6 : Be less liberal about CHECKs. #Patch Set 7 : Fix off-by-one error. #Patch Set 8 : Reduce number of calls to NumberOfFrames(). #Patch Set 9 : Rebase. #Patch Set 10 : Tidy. #
Dependent Patchsets: Messages
Total messages: 20 (10 generated)
Description was changed from ========== Support pipelining codecs in VideoProcessor. This CL removes most of the global frame state in VideoProcessor and replaces that with a vector of frame states. This is useful for pipelining codecs, where the encoded/decoded frame may not be immediately outputted after it has been sent to the codec. The callers (VideoProcessorIntegrationTest and video_quality_measurement) still call VideoProcessor in a sequential fashion. A follow-up CL will be submitted that enables batch mode in VideoProcessorIntegrationTest. BUG=webrtc:6634 ========== to ========== Support pipelining codecs in VideoProcessor. This CL removes most of the global frame state in VideoProcessor and replaces that with a vector of frame states. This is useful for pipelining codecs, where the encoded/decoded frame may not be immediately outputted after it has been sent to the codec. The callers (VideoProcessorIntegrationTest and video_quality_measurement) still call VideoProcessor in a sequential fashion. A follow-up CL will be submitted that enables batch mode in VideoProcessorIntegrationTest. Note that VideoProcessor is still not thread safe. Currently, we can run fairly well on Android due to the synchronicity of our MediaCodec wrapper, but we still cannot run on iOS due to async issues. This will be fixed in the future. BUG=webrtc:6634 ==========
brandtr@webrtc.org changed reviewers: + asapersson@webrtc.org, sprang@webrtc.org
Step #1.
Rebase.
https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:122: frame_infos_.reserve(kInitialFrameInfoSize); Could analysis_frame_reader_->NumberOfFrames() be used here. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:260: FrameStatistic& frame_stat = stats_->NewFrame(frame_number); maybe move closer to where it is used https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:420: RTC_CHECK_LE(frame_number, frame_infos_.size()); LT?
https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:37: // All foreman_* files in //resources are 300 frames long. Sounds hacky. Should we add a todo to fix this? https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:106: frame_infos_(), nit: usually omit default constructors in initializer https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:272: int32_t encode_result = encoder_->Encode(source_frame, nullptr, &frame_types); Think you drop encode_result and just use frame_stat.encode_return_code throughout? https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:297: int frame_number = encoded_image._timeStamp / k90khzTimestampFrameDiff - 1; nit: would prefer extra parenthesis for clarity = (encoded_image._timeStamp / k90khzTimestampFrameDiff) - 1 https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:300: RTC_CHECK_GE(last_encoded_frame_num_, 0); Why CHECK rather than DCHECK everywhere? Will you only be running these tests in release mode? https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:307: 1; parenthesis https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:307: 1; Sanity that encoded_image._timeStamp % k90khzTimestampFrameDiff == 0? Might be useful to add a TimestampToFrameNumber() helper method which can do that, as this seems to be done in a few places. ...and check that num_dropped_from_last_encode >= 0 https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:314: analysis_frame_writer_->FrameLength()); nit: git cl format https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:332: FrameInfo& frame_info = frame_infos_[frame_number]; Use pointer instead of non-const ref. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:335: FrameStatistic& frame_stat = stats_->stats_[frame_number]; dito https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:337: GetElapsedTimeMicroseconds(frame_info.encode_start_ns, encode_stop_ns); GetElapsedTimeMicroseconds(), but all the variables are suffixed _ns? https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:421: FrameInfo& frame_info = frame_infos_[frame_number]; pointer https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:426: GetElapsedTimeMicroseconds(frame_info.decode_start_ns, decode_stop_ns); units again?
Thanks for feedback. Please take a second look. I have verified the changes by diffing the output of 'out/Debug/modules_tests --gtest_filter="*VideoProcessor*"', before and after this CL. The only metrics that are not identical are the encode/decode times, which of course have some variance to them. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:37: // All foreman_* files in //resources are 300 frames long. On 2017/03/06 10:28:24, språng wrote: > Sounds hacky. Should we add a todo to fix this? Removed, thanks to suggestion from Åsa :) https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:106: frame_infos_(), On 2017/03/06 10:28:25, språng wrote: > nit: usually omit default constructors in initializer Done. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:122: frame_infos_.reserve(kInitialFrameInfoSize); On 2017/03/03 12:09:21, åsapersson wrote: > Could analysis_frame_reader_->NumberOfFrames() be used here. Good idea. Done. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:260: FrameStatistic& frame_stat = stats_->NewFrame(frame_number); On 2017/03/03 12:09:21, åsapersson wrote: > maybe move closer to where it is used Done. (Must be created before encode call, since it is used in the encode/decode callbacks.) https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:272: int32_t encode_result = encoder_->Encode(source_frame, nullptr, &frame_types); On 2017/03/06 10:28:25, språng wrote: > Think you drop encode_result and just use frame_stat.encode_return_code > throughout? Done. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:297: int frame_number = encoded_image._timeStamp / k90khzTimestampFrameDiff - 1; On 2017/03/06 10:28:24, språng wrote: > nit: would prefer extra parenthesis for clarity > = (encoded_image._timeStamp / k90khzTimestampFrameDiff) - 1 Agree. Done in helper function. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:300: RTC_CHECK_GE(last_encoded_frame_num_, 0); On 2017/03/06 10:28:24, språng wrote: > Why CHECK rather than DCHECK everywhere? Will you only be running these tests in > release mode? Since this test will often be run on devices in Release mode, I have been pretty liberal with CHECKs instead of DCHECKs. But the code here will actually be run on the bots as well (Debug/Release+DCHECKs), so I could probably make things cleaner by only using DCHECKs to verify invariants. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:307: 1; On 2017/03/06 10:28:25, språng wrote: > Sanity that encoded_image._timeStamp % k90khzTimestampFrameDiff == 0? > Might be useful to add a TimestampToFrameNumber() helper method which can do > that, as this seems to be done in a few places. > > ...and check that num_dropped_from_last_encode >= 0 > Good idea. Done. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:314: analysis_frame_writer_->FrameLength()); On 2017/03/06 10:28:24, språng wrote: > nit: git cl format Done. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:332: FrameInfo& frame_info = frame_infos_[frame_number]; On 2017/03/06 10:28:25, språng wrote: > Use pointer instead of non-const ref. Done. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:335: FrameStatistic& frame_stat = stats_->stats_[frame_number]; On 2017/03/06 10:28:24, språng wrote: > dito Done. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:337: GetElapsedTimeMicroseconds(frame_info.encode_start_ns, encode_stop_ns); On 2017/03/06 10:28:25, språng wrote: > GetElapsedTimeMicroseconds(), but all the variables are suffixed _ns? Only the input variables are _ns, output variable is _us. Not sure why rtc::TimeNanos() is called in the first place, when the end result is reported in microseconds however...? (This is existing code.) https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:420: RTC_CHECK_LE(frame_number, frame_infos_.size()); On 2017/03/03 12:09:21, åsapersson wrote: > LT? Yes! https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:421: FrameInfo& frame_info = frame_infos_[frame_number]; On 2017/03/06 10:28:24, språng wrote: > pointer Done. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:426: GetElapsedTimeMicroseconds(frame_info.decode_start_ns, decode_stop_ns); On 2017/03/06 10:28:25, språng wrote: > units again? Same rationale as above.
lgtm https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right): https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:300: RTC_CHECK_GE(last_encoded_frame_num_, 0); On 2017/03/06 15:27:31, brandtr wrote: > On 2017/03/06 10:28:24, språng wrote: > > Why CHECK rather than DCHECK everywhere? Will you only be running these tests > in > > release mode? > > Since this test will often be run on devices in Release mode, I have been pretty > liberal with CHECKs instead of DCHECKs. But the code here will actually be run > on the bots as well (Debug/Release+DCHECKs), so I could probably make things > cleaner by only using DCHECKs to verify invariants. Acknowledged. https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:337: GetElapsedTimeMicroseconds(frame_info.encode_start_ns, encode_stop_ns); On 2017/03/06 15:27:32, brandtr wrote: > On 2017/03/06 10:28:25, språng wrote: > > GetElapsedTimeMicroseconds(), but all the variables are suffixed _ns? > > Only the input variables are _ns, output variable is _us. > > Not sure why rtc::TimeNanos() is called in the first place, when the end result > is reported in microseconds however...? (This is existing code.) Right, I misread. Twice :) https://codereview.webrtc.org/2711133002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/test/videoprocessor.cc:426: GetElapsedTimeMicroseconds(frame_info.decode_start_ns, decode_stop_ns); On 2017/03/06 15:27:32, brandtr wrote: > On 2017/03/06 10:28:25, språng wrote: > > units again? > > Same rationale as above. Acknowledged.
lgtm
Rebase.
The CQ bit was checked by brandtr@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
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 brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from asapersson@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2711133002/#ps180001 (title: "Tidy.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1488879563611720,
"parent_rev": "ae9ba047c44a9aa295c61a666683a601b0cee8bd", "commit_rev":
"17b958c041c519fef9d42113a556efc6931da87f"}
Message was sent while issue was closed.
Description was changed from ========== Support pipelining codecs in VideoProcessor. This CL removes most of the global frame state in VideoProcessor and replaces that with a vector of frame states. This is useful for pipelining codecs, where the encoded/decoded frame may not be immediately outputted after it has been sent to the codec. The callers (VideoProcessorIntegrationTest and video_quality_measurement) still call VideoProcessor in a sequential fashion. A follow-up CL will be submitted that enables batch mode in VideoProcessorIntegrationTest. Note that VideoProcessor is still not thread safe. Currently, we can run fairly well on Android due to the synchronicity of our MediaCodec wrapper, but we still cannot run on iOS due to async issues. This will be fixed in the future. BUG=webrtc:6634 ========== to ========== Support pipelining codecs in VideoProcessor. This CL removes most of the global frame state in VideoProcessor and replaces that with a vector of frame states. This is useful for pipelining codecs, where the encoded/decoded frame may not be immediately outputted after it has been sent to the codec. The callers (VideoProcessorIntegrationTest and video_quality_measurement) still call VideoProcessor in a sequential fashion. A follow-up CL will be submitted that enables batch mode in VideoProcessorIntegrationTest. Note that VideoProcessor is still not thread safe. Currently, we can run fairly well on Android due to the synchronicity of our MediaCodec wrapper, but we still cannot run on iOS due to async issues. This will be fixed in the future. BUG=webrtc:6634 Review-Url: https://codereview.webrtc.org/2711133002 Cr-Commit-Position: refs/heads/master@{#17084} Committed: https://chromium.googlesource.com/external/webrtc/+/17b958c041c519fef9d42113a... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/external/webrtc/+/17b958c041c519fef9d42113a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
