Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 4 months ago by perkj_webrtc Modified:
4 years, 3 months ago Reviewers:
åsapersson, mflodman CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@reland_taskq_in_encoder Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionChange OverUseFrameDetector to use a task queue instead of ProcessThread to periodically check for overuse. It is made to only operate on a single task queue.
With this cl, all methods are called on the video encoder task queue.
BUG=webrtc:5687, webrtc:6289
TBR=mflodman@webrtc.org
Committed: https://crrev.com/d52063fb071b1d8cbe698ea0f5de6e6dddfacd72
Cr-Commit-Position: refs/heads/master@{#14107}
Patch Set 1 #Patch Set 2 : add Start #Patch Set 3 : More self review. #Patch Set 4 : Fix event.h include in unittest #
Total comments: 22
Patch Set 5 : Addressed code review comments. #Patch Set 6 : Addressed review comments. #
Created: 4 years, 3 months ago
Depends on Patchset: Messages
Total messages: 29 (14 generated)
Description was changed from ========== Change OverUseFrameDetector to be created on a task queue and use the task queue to process. BUG= ========== to ========== Change OverUseFrameDetector to use a task queue instead of ProcessThread to periodically check for overuse. It is made to only operate on a single task queue. With this cl, all methods are called on the video encoder task queue. BUG=webrtc:5687 ==========
perkj@webrtc.org changed reviewers: + asapersson@webrtc.org
please, when you have the time.
https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... File webrtc/video/overuse_frame_detector.cc (right): https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:34: const int64_t ktCheckForOveruseIntervalMs = 5000; kCheck..? https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:231: RTC_DCHECK(!check_over_use_task_) << "StopCheckForOverUse must be called."; nit: OverUse->Overuse https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:255: RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_); Is the check needed for private methods? https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:281: int64_t time_when_first_seen) { time_when_first_seen_ms https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:348: CpuOveruseMetrics current_metrics = *metrics_; remove current_metrics and use metrics_ instead? https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... File webrtc/video/overuse_frame_detector.h (right): https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:76: // OverUseFrameDetector::StartCheckForOverUse must be called to periodically nit: OverUse->Overuse https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:90: // StopCheckForOverUse must be called before destruction if nit: OverUse->Overuse https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:95: void FrameCaptured(const VideoFrame& frame, int64_t time_when_first_seen); time_when_first_seen_ms https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:129: CheckOveruseTask* check_over_use_task_; maybe check_overuse_task_ https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... File webrtc/video/overuse_frame_detector_unittest.cc (right): https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector_unittest.cc:326: rtc::TaskQueue queue("OveruseFrameDetectorTestQueu"); Queue https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:224: const int64_t time_when_posted_; time_when_posted_ms_
ptal https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... File webrtc/video/overuse_frame_detector.cc (right): https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:34: const int64_t ktCheckForOveruseIntervalMs = 5000; On 2016/08/24 08:59:04, åsapersson wrote: > kCheck..? Done. https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:231: RTC_DCHECK(!check_over_use_task_) << "StopCheckForOverUse must be called."; On 2016/08/24 08:59:04, åsapersson wrote: > nit: OverUse->Overuse Done. https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:255: RTC_DCHECK_CALLED_SEQUENTIALLY(&task_checker_); On 2016/08/24 08:59:04, åsapersson wrote: > Is the check needed for private methods? Yes- , if you use GUARDED_BY(..) this is needed on all methods that access that variable. https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:281: int64_t time_when_first_seen) { On 2016/08/24 08:59:04, åsapersson wrote: > time_when_first_seen_ms Done. https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.cc:348: CpuOveruseMetrics current_metrics = *metrics_; On 2016/08/24 08:59:04, åsapersson wrote: > remove current_metrics and use metrics_ instead? Done. https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... File webrtc/video/overuse_frame_detector.h (right): https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:76: // OverUseFrameDetector::StartCheckForOverUse must be called to periodically On 2016/08/24 08:59:05, åsapersson wrote: > nit: OverUse->Overuse Done. https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:90: // StopCheckForOverUse must be called before destruction if On 2016/08/24 08:59:05, åsapersson wrote: > nit: OverUse->Overuse Done. https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:95: void FrameCaptured(const VideoFrame& frame, int64_t time_when_first_seen); On 2016/08/24 08:59:05, åsapersson wrote: > time_when_first_seen_ms Done. https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector.h:129: CheckOveruseTask* check_over_use_task_; On 2016/08/24 08:59:05, åsapersson wrote: > maybe check_overuse_task_ Done. https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... File webrtc/video/overuse_frame_detector_unittest.cc (right): https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/overuse_fram... webrtc/video/overuse_frame_detector_unittest.cc:326: rtc::TaskQueue queue("OveruseFrameDetectorTestQueu"); On 2016/08/24 08:59:05, åsapersson wrote: > Queue Done. https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2255463002/diff/60001/webrtc/video/vie_encoder.... webrtc/video/vie_encoder.cc:224: const int64_t time_when_posted_; On 2016/08/24 08:59:05, åsapersson wrote: > time_when_posted_ms_ Done.
Description was changed from ========== Change OverUseFrameDetector to use a task queue instead of ProcessThread to periodically check for overuse. It is made to only operate on a single task queue. With this cl, all methods are called on the video encoder task queue. BUG=webrtc:5687 ========== to ========== Change OverUseFrameDetector to use a task queue instead of ProcessThread to periodically check for overuse. It is made to only operate on a single task queue. With this cl, all methods are called on the video encoder task queue. BUG=webrtc:5687,webrtc:6289 ==========
lgtm
The CQ bit was checked by perkj@webrtc.org
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8006)
perkj@webrtc.org changed reviewers: + mflodman@webrtc.org
Need you again Magnus.
Description was changed from ========== Change OverUseFrameDetector to use a task queue instead of ProcessThread to periodically check for overuse. It is made to only operate on a single task queue. With this cl, all methods are called on the video encoder task queue. BUG=webrtc:5687,webrtc:6289 ========== to ========== Change OverUseFrameDetector to use a task queue instead of ProcessThread to periodically check for overuse. It is made to only operate on a single task queue. With this cl, all methods are called on the video encoder task queue. BUG=webrtc:5687,webrtc:6289 TBR=mflodman@webrtc.org ==========
mflodman - please take a look at this when you can.
The CQ bit was checked by perkj@webrtc.org
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
Try jobs failed on following builders: linux_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by perkj@webrtc.org
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
Try jobs failed on following builders: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, no build URL)
The CQ bit was checked by perkj@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Change OverUseFrameDetector to use a task queue instead of ProcessThread to periodically check for overuse. It is made to only operate on a single task queue. With this cl, all methods are called on the video encoder task queue. BUG=webrtc:5687,webrtc:6289 TBR=mflodman@webrtc.org ========== to ========== Change OverUseFrameDetector to use a task queue instead of ProcessThread to periodically check for overuse. It is made to only operate on a single task queue. With this cl, all methods are called on the video encoder task queue. BUG=webrtc:5687,webrtc:6289 TBR=mflodman@webrtc.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Change OverUseFrameDetector to use a task queue instead of ProcessThread to periodically check for overuse. It is made to only operate on a single task queue. With this cl, all methods are called on the video encoder task queue. BUG=webrtc:5687,webrtc:6289 TBR=mflodman@webrtc.org ========== to ========== Change OverUseFrameDetector to use a task queue instead of ProcessThread to periodically check for overuse. It is made to only operate on a single task queue. With this cl, all methods are called on the video encoder task queue. BUG=webrtc:5687,webrtc:6289 TBR=mflodman@webrtc.org Committed: https://crrev.com/d52063fb071b1d8cbe698ea0f5de6e6dddfacd72 Cr-Commit-Position: refs/heads/master@{#14107} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d52063fb071b1d8cbe698ea0f5de6e6dddfacd72 Cr-Commit-Position: refs/heads/master@{#14107} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
