Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1750493002:
Frame dropper improvements & cleanup (Closed)
|
Created:
4 years, 9 months ago by Irfan Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFrame dropper improvements & cleanup
1. Fix the case of key frame accumulation being incorrect due to the chunk
size being computed at the time of leak based on input frame rate. The issue
is that the count is computed based on key frame ratio and the actual chunk
size computed from current input frame rate. These can be wildly different
especially at the beginning of the stream (key frame ratio defaults based
on 30 fps) resulting in incorrect key frame accumulation causing large frame
drops when the input frame rate is low.
2. Add large delta frame compensation. The current code accounts for key frames
but not large delta frames. This is a common occurence in some application
(remote desktop as an example)
3. Fixes an issue identified by the unit tests. The accumulation of
key frames had an issue in the scenario of a high key frame ratio where
the full key frame was not being accounted for.
3. Removes fast mode and other methods that are mostly dead code.
4. Cleans up variable names as per chromium style.
Committed: https://crrev.com/7620be8492fc96ac8e3ca9fb28fd0d64b99c8e4a
Cr-Commit-Position: refs/heads/master@{#11884}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add unit tests #
Total comments: 10
Patch Set 3 : Addressed comments #
Messages
Total messages: 36 (15 generated)
Description was changed from ========== Frame dropper improvements & cleanup 1. Fix the case of key frame accumulation being incorrect due to the chunk size being computed at the time of leak based on input frame rate. The issue is that the count is computed based on key frame ratio and the actual chunk size computed from current input frame rate. These can be wildly different especially at the beginning of the stream (key frame ratio defaults based on 30 fps) resulting in incorrect key frame accumulation causing large frame drops when the input frame rate is low. 2. Add large delta frame compensation. The current code accounts for key frames but not large delta frames. This is a common occurence in some application (remote desktop as an example) 3. Removes fast mode and other methods that are mostly dead code. 4. Cleans up variable names as per chromium style. TODO: Need to add unit tests for frame dropper before submit. BUG= ========== to ========== Frame dropper improvements & cleanup 1. Fix the case of key frame accumulation being incorrect due to the chunk size being computed at the time of leak based on input frame rate. The issue is that the count is computed based on key frame ratio and the actual chunk size computed from current input frame rate. These can be wildly different especially at the beginning of the stream (key frame ratio defaults based on 30 fps) resulting in incorrect key frame accumulation causing large frame drops when the input frame rate is low. 2. Add large delta frame compensation. The current code accounts for key frames but not large delta frames. This is a common occurence in some application (remote desktop as an example) 3. Removes fast mode and other methods that are mostly dead code. 4. Cleans up variable names as per chromium style. TODO: Need to add unit tests for frame dropper before submit. BUG= ==========
isheriff@chromium.org changed reviewers: + mflodman@webrtc.org, niklase@chromium.org, sergeyu@chromium.org, stefan@webrtc.org
Frame dropper shows a few issues during testing with chromoting (captured in commit message) that I have tried to address besides clean up. I need feedback on how to make sure I do not cause regressions on other use cases. Any guidance on testing will be helpful. Thanks! This still fully does not address the issue of drops due to higher rate of traffic than the initial bandwidth assumption (300 kbps). I am looking at a follow up CL that provides per-packet VideoSinkWants feedback on adjusting frame rate to capturer.
stefan@webrtc.org changed reviewers: + sprang@webrtc.org
Looks like a good change. Adding sprang who may want to review the details https://codereview.webrtc.org/1750493002/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/frame_dropper.cc (right): https://codereview.webrtc.org/1750493002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/frame_dropper.cc:159: large_frame_accumulation_count_--; --large_frame...; https://codereview.webrtc.org/1750493002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/frame_dropper.cc:165: LOG(LS_VERBOSE) << " LEAK acc " << accumulator_ << " max " << accumulator_max_ Remove space before LEAK
And if you can add some unittests that would be much appreciated, especially since there are none at all today!
Erik, PTAL. Still need to work on unit tests.
Looks good. Ugh, this class is a prime example of some of our ugliest legacy code. Thanks for cleaning a bit. I'm wondering if there's any point in special casing both key frames and large frames? Could we just use large frames instead? https://codereview.webrtc.org/1750493002/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/frame_dropper.cc (right): https://codereview.webrtc.org/1750493002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/frame_dropper.cc:119: framesize_kbits > 3 * delta_frame_size_avg_kbits_.filtered() && Named constant for 3.
https://codereview.webrtc.org/1750493002/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/frame_dropper.cc (right): https://codereview.webrtc.org/1750493002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/frame_dropper.cc:159: large_frame_accumulation_count_--; On 2016/03/01 15:14:34, stefan-webrtc (holmer) wrote: > --large_frame...; Done. https://codereview.webrtc.org/1750493002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/frame_dropper.cc:165: LOG(LS_VERBOSE) << " LEAK acc " << accumulator_ << " max " << accumulator_max_ On 2016/03/01 15:14:34, stefan-webrtc (holmer) wrote: > Remove space before LEAK Done.
PTAL. I have added a few unit tests and fixed a couple of issues spotted by the tests. - The key frame accumulation had a bug where the averaging was resulting in extra bits being accumulated (when a given value was less than average) - delta frame averaging was incorrect
lgtm sprang, do you have any other comments? https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/frame_dropper.h (right): https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/frame_dropper.h:80: int32_t large_frame_accumulation_count_; Use a regular int here and at line 90.
sprang@google.com changed reviewers: + sprang@google.com
lgtm with nits thanks! https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/frame_dropper_unittest.cc (right): https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/frame_dropper_unittest.cc:33: virtual void SetUp() { void SetUp() override { https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/frame_dropper_unittest.cc:37: // virtual void TearDown() {} Remove https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/frame_dropper_unittest.cc:53: (large_frame_size_bytes * large_frame_rate) / kIncomingFrameRate); Remove outer parentheses or change to: int small_frame_size_bytes = kFrameSizeBytes - ((large_frame_size_bytes * large_frame_rate) / kIncomingFrameRate); https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/frame_dropper_unittest.cc:82: const int num_iterations = 1000; kNumIterations
Oh, and remove "TODO: Need to add unit tests for frame dropper before submit." from the CL description
https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/frame_dropper.h (right): https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/frame_dropper.h:80: int32_t large_frame_accumulation_count_; On 2016/03/04 08:41:37, stefan-webrtc (holmer) wrote: > Use a regular int here and at line 90. Done. https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/frame_dropper_unittest.cc (right): https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/frame_dropper_unittest.cc:33: virtual void SetUp() { On 2016/03/04 08:57:04, sprang wrote: > void SetUp() override { Done. https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/frame_dropper_unittest.cc:37: // virtual void TearDown() {} On 2016/03/04 08:57:04, sprang wrote: > Remove Done. https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/frame_dropper_unittest.cc:53: (large_frame_size_bytes * large_frame_rate) / kIncomingFrameRate); On 2016/03/04 08:57:04, sprang wrote: > Remove outer parentheses or change to: > int small_frame_size_bytes = > kFrameSizeBytes - > ((large_frame_size_bytes * large_frame_rate) / kIncomingFrameRate); Done. https://codereview.webrtc.org/1750493002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/frame_dropper_unittest.cc:82: const int num_iterations = 1000; On 2016/03/04 08:57:04, sprang wrote: > kNumIterations Done.
Description was changed from
==========
Frame dropper improvements & cleanup
1. Fix the case of key frame accumulation being incorrect due to the chunk
size being computed at the time of leak based on input frame rate. The issue
is that the count is computed based on key frame ratio and the actual chunk
size computed from current input frame rate. These can be wildly different
especially at the beginning of the stream (key frame ratio defaults based
on 30 fps) resulting in incorrect key frame accumulation causing large frame
drops when the input frame rate is low.
2. Add large delta frame compensation. The current code accounts for key frames
but not large delta frames. This is a common occurence in some application
(remote desktop as an example)
3. Removes fast mode and other methods that are mostly dead code.
4. Cleans up variable names as per chromium style.
TODO: Need to add unit tests for frame dropper before submit.
BUG=
==========
to
==========
Frame dropper improvements & cleanup
1. Fix the case of key frame accumulation being incorrect due to the chunk
size being computed at the time of leak based on input frame rate. The issue
is that the count is computed based on key frame ratio and the actual chunk
size computed from current input frame rate. These can be wildly different
especially at the beginning of the stream (key frame ratio defaults based
on 30 fps) resulting in incorrect key frame accumulation causing large frame
drops when the input frame rate is low.
2. Add large delta frame compensation. The current code accounts for key
frames
but not large delta frames. This is a common occurence in some application
(remote desktop as an example)
3. Fixes an issue identified by the unit tests. The accumulation of
key frames had an issue in the scenario of a high key frame ratio where
the full key frame was not being accounted for.
3. Removes fast mode and other methods that are mostly dead code.
4. Cleans up variable names as per chromium style.
==========
PTAL
The CQ bit was checked by isheriff@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@google.com, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1750493002/#ps40001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750493002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750493002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by isheriff@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750493002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750493002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by isheriff@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750493002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750493002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13284)
The CQ bit was checked by isheriff@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750493002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750493002/40001
Message was sent while issue was closed.
Description was changed from
==========
Frame dropper improvements & cleanup
1. Fix the case of key frame accumulation being incorrect due to the chunk
size being computed at the time of leak based on input frame rate. The issue
is that the count is computed based on key frame ratio and the actual chunk
size computed from current input frame rate. These can be wildly different
especially at the beginning of the stream (key frame ratio defaults based
on 30 fps) resulting in incorrect key frame accumulation causing large frame
drops when the input frame rate is low.
2. Add large delta frame compensation. The current code accounts for key
frames
but not large delta frames. This is a common occurence in some application
(remote desktop as an example)
3. Fixes an issue identified by the unit tests. The accumulation of
key frames had an issue in the scenario of a high key frame ratio where
the full key frame was not being accounted for.
3. Removes fast mode and other methods that are mostly dead code.
4. Cleans up variable names as per chromium style.
==========
to
==========
Frame dropper improvements & cleanup
1. Fix the case of key frame accumulation being incorrect due to the chunk
size being computed at the time of leak based on input frame rate. The issue
is that the count is computed based on key frame ratio and the actual chunk
size computed from current input frame rate. These can be wildly different
especially at the beginning of the stream (key frame ratio defaults based
on 30 fps) resulting in incorrect key frame accumulation causing large frame
drops when the input frame rate is low.
2. Add large delta frame compensation. The current code accounts for key
frames
but not large delta frames. This is a common occurence in some application
(remote desktop as an example)
3. Fixes an issue identified by the unit tests. The accumulation of
key frames had an issue in the scenario of a high key frame ratio where
the full key frame was not being accounted for.
3. Removes fast mode and other methods that are mostly dead code.
4. Cleans up variable names as per chromium style.
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
Frame dropper improvements & cleanup
1. Fix the case of key frame accumulation being incorrect due to the chunk
size being computed at the time of leak based on input frame rate. The issue
is that the count is computed based on key frame ratio and the actual chunk
size computed from current input frame rate. These can be wildly different
especially at the beginning of the stream (key frame ratio defaults based
on 30 fps) resulting in incorrect key frame accumulation causing large frame
drops when the input frame rate is low.
2. Add large delta frame compensation. The current code accounts for key
frames
but not large delta frames. This is a common occurence in some application
(remote desktop as an example)
3. Fixes an issue identified by the unit tests. The accumulation of
key frames had an issue in the scenario of a high key frame ratio where
the full key frame was not being accounted for.
3. Removes fast mode and other methods that are mostly dead code.
4. Cleans up variable names as per chromium style.
==========
to
==========
Frame dropper improvements & cleanup
1. Fix the case of key frame accumulation being incorrect due to the chunk
size being computed at the time of leak based on input frame rate. The issue
is that the count is computed based on key frame ratio and the actual chunk
size computed from current input frame rate. These can be wildly different
especially at the beginning of the stream (key frame ratio defaults based
on 30 fps) resulting in incorrect key frame accumulation causing large frame
drops when the input frame rate is low.
2. Add large delta frame compensation. The current code accounts for key
frames
but not large delta frames. This is a common occurence in some application
(remote desktop as an example)
3. Fixes an issue identified by the unit tests. The accumulation of
key frames had an issue in the scenario of a high key frame ratio where
the full key frame was not being accounted for.
3. Removes fast mode and other methods that are mostly dead code.
4. Cleans up variable names as per chromium style.
Committed: https://crrev.com/7620be8492fc96ac8e3ca9fb28fd0d64b99c8e4a
Cr-Commit-Position: refs/heads/master@{#11884}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7620be8492fc96ac8e3ca9fb28fd0d64b99c8e4a Cr-Commit-Position: refs/heads/master@{#11884} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
