Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903563002:
Compare adapt up/down request with sink_wants_ in VideoSourceProxy methods to make sure it is higher (Closed)
|
Created:
3 years, 7 months ago by åsapersson Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCompare adapt up/down request with sink_wants_ in VideoSourceProxy methods to make sure it is higher/lower than last request.
Add methods RestrictFramerate, IncreaseFramerate.
To be used by kBalanced mode.
This CL is split from: https://codereview.webrtc.org/2887303003/
BUG=webrtc:7607
Review-Url: https://codereview.webrtc.org/2903563002
Cr-Commit-Position: refs/heads/master@{#18463}
Committed: https://chromium.googlesource.com/external/webrtc/+/1387476dc684f54cbf8115113e510633eea269a3
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #Patch Set 3 : rebase #
Messages
Total messages: 18 (11 generated)
Description was changed from ========== Split.. BUG=webrtc:7607 ========== to ========== Compare adapt up/down request with sink_wants_ in VideoSourceProxy methods to make sure it is higher/lower than last request. Add methods RestrictFramerate, IncreaseFramerate. To be used by kBalanced mode. This CL is split from: https://codereview.webrtc.org/2887303003/ BUG=webrtc:7607 ==========
Description was changed from ========== Compare adapt up/down request with sink_wants_ in VideoSourceProxy methods to make sure it is higher/lower than last request. Add methods RestrictFramerate, IncreaseFramerate. To be used by kBalanced mode. This CL is split from: https://codereview.webrtc.org/2887303003/ BUG=webrtc:7607 ========== to ========== Compare adapt up/down request with sink_wants_ in VideoSourceProxy methods to make sure it is higher/lower than last request. Add methods RestrictFramerate, IncreaseFramerate. To be used by kBalanced mode. This CL is split from: https://codereview.webrtc.org/2887303003/ BUG=webrtc:7607 ==========
asapersson@webrtc.org changed reviewers: + kthelgason@webrtc.org, sprang@webrtc.org
https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc#n... webrtc/video/vie_encoder.cc:245: LOG(LS_INFO) << "Scaling down resolution."; Might be nice with the actual pixel counts in the log message too? Here and below. https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc#n... webrtc/video/vie_encoder.cc:253: // Called on the encoder task queue. Can we add thread/sequence checkers for this maybe? here and elsewhere https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc#n... webrtc/video/vie_encoder.cc:271: return false; When would this happen? Should we reset sink_wants_.max_pixel_count in that case?
On 2017/05/29 14:45:47, sprang_webrtc wrote: > https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc > File webrtc/video/vie_encoder.cc (right): > > https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc#n... > webrtc/video/vie_encoder.cc:245: LOG(LS_INFO) << "Scaling down resolution."; > Might be nice with the actual pixel counts in the log message too? Here and > below. > > https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc#n... > webrtc/video/vie_encoder.cc:253: // Called on the encoder task queue. > Can we add thread/sequence checkers for this maybe? here and elsewhere > > https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc#n... > webrtc/video/vie_encoder.cc:271: return false; > When would this happen? Should we reset sink_wants_.max_pixel_count in that > case? lgtm % sprang's comments
https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc#n... webrtc/video/vie_encoder.cc:245: LOG(LS_INFO) << "Scaling down resolution."; On 2017/05/29 14:45:47, sprang_webrtc wrote: > Might be nice with the actual pixel counts in the log message too? Here and > below. Done. https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc#n... webrtc/video/vie_encoder.cc:253: // Called on the encoder task queue. On 2017/05/29 14:45:47, sprang_webrtc wrote: > Can we add thread/sequence checkers for this maybe? here and elsewhere I can have a look but maybe in a separate cl? https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc#n... webrtc/video/vie_encoder.cc:271: return false; On 2017/05/29 14:45:47, sprang_webrtc wrote: > When would this happen? Should we reset sink_wants_.max_pixel_count in that > case? In balanced mode (https://codereview.webrtc.org/2887303003/), last_adaptation_request_ is not used. Equal pixel count can happen if AdaptUp/AdaptDown is called frequently. The intention is for it to work similar to last_adaptation_request_ (and maybe later remove last_adaptation_request_ if possible).
lgtm https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc File webrtc/video/vie_encoder.cc (right): https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc#n... webrtc/video/vie_encoder.cc:253: // Called on the encoder task queue. On 2017/05/30 13:57:30, åsapersson wrote: > On 2017/05/29 14:45:47, sprang_webrtc wrote: > > Can we add thread/sequence checkers for this maybe? here and elsewhere > > I can have a look but maybe in a separate cl? Sure, maybe you can add it to one of the upcoming ones? https://codereview.webrtc.org/2903563002/diff/1/webrtc/video/vie_encoder.cc#n... webrtc/video/vie_encoder.cc:271: return false; On 2017/05/30 13:57:31, åsapersson wrote: > On 2017/05/29 14:45:47, sprang_webrtc wrote: > > When would this happen? Should we reset sink_wants_.max_pixel_count in that > > case? > > In balanced mode (https://codereview.webrtc.org/2887303003/), > last_adaptation_request_ is not used. Equal pixel count can happen if > AdaptUp/AdaptDown is called frequently. The intention is for it to work similar > to last_adaptation_request_ (and maybe later remove last_adaptation_request_ if > possible). Ack. Maybe add comment about that?
The CQ bit was checked by asapersson@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 asapersson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2903563002/#ps40001 (title: "rebase")
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": 40001, "attempt_start_ts": 1496818743407530,
"parent_rev": "23ec19dbb9046bd00655b1c065f2400bcc3cf8da", "commit_rev":
"1387476dc684f54cbf8115113e510633eea269a3"}
Message was sent while issue was closed.
Description was changed from ========== Compare adapt up/down request with sink_wants_ in VideoSourceProxy methods to make sure it is higher/lower than last request. Add methods RestrictFramerate, IncreaseFramerate. To be used by kBalanced mode. This CL is split from: https://codereview.webrtc.org/2887303003/ BUG=webrtc:7607 ========== to ========== Compare adapt up/down request with sink_wants_ in VideoSourceProxy methods to make sure it is higher/lower than last request. Add methods RestrictFramerate, IncreaseFramerate. To be used by kBalanced mode. This CL is split from: https://codereview.webrtc.org/2887303003/ BUG=webrtc:7607 Review-Url: https://codereview.webrtc.org/2903563002 Cr-Commit-Position: refs/heads/master@{#18463} Committed: https://chromium.googlesource.com/external/webrtc/+/1387476dc684f54cbf8115113... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/1387476dc684f54cbf8115113... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
