Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968153002:
Refactor timing frame logic to work with encoders with internal sources (Closed)
|
Created:
3 years, 5 months ago by ilnik Modified:
3 years, 5 months ago Reviewers:
sprang_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRefactor timing frame logic to work with encoders with internal sources
BUG=webrtc:7594, webrtc:7893
Review-Url: https://codereview.webrtc.org/2968153002
Cr-Commit-Position: refs/heads/master@{#18955}
Committed: https://chromium.googlesource.com/external/webrtc/+/a7a4535a35e2fccfab06c79d8704c1ca92970a6e
Patch Set 1 #
Total comments: 5
Patch Set 2 : Implement Sprang@ comments #Messages
Total messages: 15 (8 generated)
The CQ bit was checked by ilnik@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.
ilnik@webrtc.org changed reviewers: + sprang@webrtc.org
https://codereview.webrtc.org/2968153002/diff/1/webrtc/modules/video_coding/g... File webrtc/modules/video_coding/generic_encoder.cc (right): https://codereview.webrtc.org/2968153002/diff/1/webrtc/modules/video_coding/g... webrtc/modules/video_coding/generic_encoder.cc:233: size_t outlier_frame_size = encoded_image._length + 1; Won't this cause all frames from an encoder with internal source to be traced? https://codereview.webrtc.org/2968153002/diff/1/webrtc/modules/video_coding/g... webrtc/modules/video_coding/generic_encoder.cc:257: encode_start_map->end()); encode_start_map.clear()
https://codereview.webrtc.org/2968153002/diff/1/webrtc/modules/video_coding/g... File webrtc/modules/video_coding/generic_encoder.cc (right): https://codereview.webrtc.org/2968153002/diff/1/webrtc/modules/video_coding/g... webrtc/modules/video_coding/generic_encoder.cc:233: size_t outlier_frame_size = encoded_image._length + 1; On 2017/07/10 16:12:04, sprang_webrtc wrote: > Won't this cause all frames from an encoder with internal source to be traced? No, this does exactly the opposite - |outlier_frame_size| is more that current frame length, so it will never be detected as an outlier if outlier size is not recalculated. Normal rate-throttling logic is not changed. Comment is with typo. Fixed. https://codereview.webrtc.org/2968153002/diff/1/webrtc/modules/video_coding/g... webrtc/modules/video_coding/generic_encoder.cc:257: encode_start_map->end()); On 2017/07/10 16:12:04, sprang_webrtc wrote: > encode_start_map.clear() Done.
The CQ bit was checked by sprang@webrtc.org
lgtm https://codereview.webrtc.org/2968153002/diff/1/webrtc/modules/video_coding/g... File webrtc/modules/video_coding/generic_encoder.cc (right): https://codereview.webrtc.org/2968153002/diff/1/webrtc/modules/video_coding/g... webrtc/modules/video_coding/generic_encoder.cc:233: size_t outlier_frame_size = encoded_image._length + 1; On 2017/07/10 16:24:07, ilnik wrote: > On 2017/07/10 16:12:04, sprang_webrtc wrote: > > Won't this cause all frames from an encoder with internal source to be traced? > > No, this does exactly the opposite - |outlier_frame_size| is more that current > frame length, so it will never be detected as an outlier if outlier size is not > recalculated. Normal rate-throttling logic is not changed. Comment is with typo. > Fixed. Ah, of course :)
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": 20001, "attempt_start_ts": 1499704755392790,
"parent_rev": "5ce5a4751caeddc48cc127102df96233167976f0", "commit_rev":
"a7a4535a35e2fccfab06c79d8704c1ca92970a6e"}
Message was sent while issue was closed.
Description was changed from ========== Refactor timing frame logic to work with encoders with internal sources BUG=webrtc:7594,webrtc:7893 ========== to ========== Refactor timing frame logic to work with encoders with internal sources BUG=webrtc:7594,webrtc:7893 Review-Url: https://codereview.webrtc.org/2968153002 Cr-Commit-Position: refs/heads/master@{#18955} Committed: https://chromium.googlesource.com/external/webrtc/+/a7a4535a35e2fccfab06c79d8... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/a7a4535a35e2fccfab06c79d8...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.webrtc.org/2980533002/ by ilnik@webrtc.org. The reason for reverting is: Failing chromoting tests. |
