New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 609816 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

15.7%-66.5% improvement in webrtc_perf_tests at 12630:12630

Project Member Reported by ivoc@chromium.org, May 6 2016

Issue description

See graphs below.
 

Comment 2 by ivoc@chromium.org, May 6 2016

Labels: -M-50 M-52
Owner: perkj@chromium.org
There are many affected perf stats that seem to be caused by this CL: https://codereview.webrtc.org/1947873002 . Some are improvements, but there are also many regressions. Per, could you have a look at this? 

Comment 3 by perkj@chromium.org, May 9 2016

Status: Started (was: Assigned)

Comment 4 by perkj@chromium.org, May 10 2016

Cc: ivoc@chromium.org stefan@webrtc.org

Comment 5 by perkj@chromium.org, May 10 2016

It turns out that the failing tests is due to a bug in the current code that runs the proper at 300kbit regardless of configured start bitrate.
With https://codereview.webrtc.org/1958113003/ , the prober will run at the rate configured. 

Project Member

Comment 6 by bugdroid1@chromium.org, May 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/ec81bcd5198cc09e4332ddeee195a1c992b6a780

commit ec81bcd5198cc09e4332ddeee195a1c992b6a780
Author: perkj <perkj@webrtc.org>
Date: Wed May 11 13:01:13 2016

Remove SendPacer from ViEEncoder and make sure SendPacer starts at a valid bitrate

This reverts commit e30c27205148b34ba421184efe65f6a0780b436d (https://codereview.webrtc.org/1958053002/)

Original reverted cl is in patch set #1.
Changes in following patch sets.

The cl now also make sure SendPacer starts with the configured bitrate provided in a call to CongestionController::SetBweBitrates)()

It turns out that the failing tests in 609816 is due to a bug in the current code that runs the proper at 300kbit regardless of configured start bitrate.

Original cl description:
Remove SendPacer from ViEEncoder
This CL moves the logic where the ViEEncoder pause if the pacer is full to the BitrateController. If the queue is full, the controller reports a bitrate of zero to  Call (and BitrateAllocator)

BUG= chromium:609816 ,  webrtc:5687 
TBR=mflodman@webrtc.org
NOTRY=True  // Due to bug  in android_x86 cq builder....

Review-Url: https://codereview.webrtc.org/1958113003
Cr-Commit-Position: refs/heads/master@{#12688}

[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/audio/audio_receive_stream_unittest.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/audio/audio_send_stream_unittest.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/call/bitrate_allocator.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/call/bitrate_allocator.h
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/call/bitrate_allocator_unittest.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/call/call.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/bitrate_controller/bitrate_controller_impl.h
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/bitrate_controller/include/bitrate_controller.h
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/congestion_controller/congestion_controller.cc
[add] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/congestion_controller/congestion_controller_unittest.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/congestion_controller/include/congestion_controller.h
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/modules.gyp
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/pacing/mock/mock_paced_sender.h
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/pacing/paced_sender.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/pacing/paced_sender.h
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/pacing/paced_sender_unittest.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/pacing/packet_router.h
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/remote_bitrate_estimator/test/packet_sender.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/modules/remote_bitrate_estimator/test/packet_sender.h
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/video/encoder_state_feedback_unittest.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/video/video_send_stream.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/video/vie_encoder.cc
[modify] https://crrev.com/ec81bcd5198cc09e4332ddeee195a1c992b6a780/webrtc/video/vie_encoder.h

Comment 7 by ivoc@chromium.org, May 11 2016

Great, thanks Per!

Comment 8 by perkj@chromium.org, May 11 2016

Cc: kjellander@chromium.org
The expected alerts fired again after the reland in #6 and I associated them with this bug.

Comment 10 by perkj@chromium.org, May 14 2016

Status: Fixed (was: Started)

Sign in to add a comment