Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2254893009:
Add histogram for percentage of incoming frames that are limited in resolution due to cpu (Closed)
|
Created:
4 years, 4 months ago by åsapersson Modified:
4 years, 4 months ago Reviewers:
perkj_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd histogram for percentage of incoming frames that are limited in resolution due to cpu:
- "WebRTC.Video.CpuLimitedResolutionInPercent"
BUG=webrtc:6235
Committed: https://crrev.com/0d1ad326a391a8c4c53d7ccfc8100c5d9f4115a2
Cr-Commit-Position: refs/heads/master@{#13858}
Patch Set 1 #Patch Set 2 #
Total comments: 7
Patch Set 3 : address comments #
Messages
Total messages: 18 (8 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add histogram for percentage of incoming frames that are limited in resolution by the video adapter and due to cpu: - "WebRTC.Video.AdapterLimitedResolutionInPercent" - "WebRTC.Video.CpuLimitedResolutionInPercent" BUG= ========== to ========== Add histogram for percentage of incoming frames that are limited in resolution due to cpu: - "WebRTC.Video.CpuLimitedResolutionInPercent" BUG= ==========
asapersson@webrtc.org changed reviewers: + mflodman@webrtc.org
asapersson@webrtc.org changed reviewers: + perkj@webrtc.org - mflodman@webrtc.org
https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1692: if (cpu_restricted_counter_ > 0) Would it be possible to do this in ViEEncoder instead? I am trying to remove all dependencies to frames from this class and express CPU adaptation in ViEEncoder as VideoSinkWants.
https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1692: if (cpu_restricted_counter_ > 0) On 2016/08/19 13:14:46, perkj_webrtc wrote: > Would it be possible to do this in ViEEncoder instead? I am trying to remove all > dependencies to frames from this class and express CPU adaptation in ViEEncoder > as VideoSinkWants. Could it be done in a separate/follow up CL when the cpu adaptation is expressed in ViEEncoder?
https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1635: cpu_restricted_frames_ * 100 / frames_); suggest rename frames_ to frame_count_. Suggest to change cpu_restricted_frames to cpu_restricted_frame_count_ https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1691: ++frames_; frames_ and cpu_restricted_frames_ will be written by the camera thread and and read by the worker thread in thi s cl. Easiest way that is consistent with the rest in this file is probably just to declare frames_ cpu_restricted_frames_ as GUARDED_BY(&lock_) and take lock in UpdateHistograms if that is safe. https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1692: if (cpu_restricted_counter_ > 0) On 2016/08/22 07:56:29, åsapersson wrote: > On 2016/08/19 13:14:46, perkj_webrtc wrote: > > Would it be possible to do this in ViEEncoder instead? I am trying to remove > all > > dependencies to frames from this class and express CPU adaptation in > ViEEncoder > > as VideoSinkWants. > > Could it be done in a separate/follow up CL when the cpu adaptation is expressed > in ViEEncoder? sure.
On 2016/08/22 09:06:43, perkj_webrtc wrote: > https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.cc:1635: cpu_restricted_frames_ * 100 / > frames_); > suggest rename frames_ to frame_count_. Suggest to change cpu_restricted_frames > to cpu_restricted_frame_count_ > > https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.cc:1691: ++frames_; > frames_ and cpu_restricted_frames_ will be written by the camera thread and and > read by the worker thread in thi s cl. > Easiest way that is consistent with the rest in this file is probably just to > declare frames_ cpu_restricted_frames_ as GUARDED_BY(&lock_) and take lock in > UpdateHistograms if that is safe. > > https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvideoengine2.cc:1692: if (cpu_restricted_counter_ > 0) > On 2016/08/22 07:56:29, åsapersson wrote: > > On 2016/08/19 13:14:46, perkj_webrtc wrote: > > > Would it be possible to do this in ViEEncoder instead? I am trying to remove > > all > > > dependencies to frames from this class and express CPU adaptation in > > ViEEncoder > > > as VideoSinkWants. > > > > Could it be done in a separate/follow up CL when the cpu adaptation is > expressed > > in ViEEncoder? > > sure. oh, and a bug number ?
Description was changed from ========== Add histogram for percentage of incoming frames that are limited in resolution due to cpu: - "WebRTC.Video.CpuLimitedResolutionInPercent" BUG= ========== to ========== Add histogram for percentage of incoming frames that are limited in resolution due to cpu: - "WebRTC.Video.CpuLimitedResolutionInPercent" BUG=webrtc:6235 ==========
https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1635: cpu_restricted_frames_ * 100 / frames_); On 2016/08/22 09:06:42, perkj_webrtc wrote: > suggest rename frames_ to frame_count_. Suggest to change cpu_restricted_frames > to cpu_restricted_frame_count_ Done. https://codereview.webrtc.org/2254893009/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1691: ++frames_; On 2016/08/22 09:06:42, perkj_webrtc wrote: > frames_ and cpu_restricted_frames_ will be written by the camera thread and and > read by the worker thread in thi s cl. > Easiest way that is consistent with the rest in this file is probably just to > declare frames_ cpu_restricted_frames_ as GUARDED_BY(&lock_) and take lock in > UpdateHistograms if that is safe. Added guarded_by (and exclusive_lock_required for UpdateHistograms since called from destructor).
lgtm
The CQ bit was checked by asapersson@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 ========== Add histogram for percentage of incoming frames that are limited in resolution due to cpu: - "WebRTC.Video.CpuLimitedResolutionInPercent" BUG=webrtc:6235 ========== to ========== Add histogram for percentage of incoming frames that are limited in resolution due to cpu: - "WebRTC.Video.CpuLimitedResolutionInPercent" BUG=webrtc:6235 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add histogram for percentage of incoming frames that are limited in resolution due to cpu: - "WebRTC.Video.CpuLimitedResolutionInPercent" BUG=webrtc:6235 ========== to ========== Add histogram for percentage of incoming frames that are limited in resolution due to cpu: - "WebRTC.Video.CpuLimitedResolutionInPercent" BUG=webrtc:6235 Committed: https://crrev.com/0d1ad326a391a8c4c53d7ccfc8100c5d9f4115a2 Cr-Commit-Position: refs/heads/master@{#13858} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0d1ad326a391a8c4c53d7ccfc8100c5d9f4115a2 Cr-Commit-Position: refs/heads/master@{#13858} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
