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

Issue 779417 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Nov 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocking:
issue 769391



Sign in to add a comment

WebrtcFrameSchedulerSimple::EncoderBitrateFilter is designed for VP8 only

Project Member Reported by zijiehe@chromium.org, Oct 29 2017

Issue description

The |minimum_bitrate_| is way too high.
(2500 * 1920 * 1080) / 1000000 = 5184. i.e. 5.2Mbps.
 
Blocking: 769391
Components: Blink>WebRTC
Components: -Blink>WebRTC Services>Chromoting
Owner: zijiehe@chromium.org
Status: Assigned (was: Untriaged)
Zijie, can you provide a bit more context here? It's not clear from your description what needs to change or how.
Sorry, my bad. This bug is tracking the effort to address the TODO at https://cs.chromium.org/chromium/src/remoting/protocol/webrtc_frame_scheduler_simple.cc?rcl=1b569774f891dbdb8a1ec781ca779cefeb3b7ef4&l=98.

Currently EncoderBitrateFilter is embedded inside of the WebrtcFrameSchedulerSimple. The main disadvantage is that WebrtcFrameSchedulerSimple knows nothing about the encoder, it can only blindly apply the filter as if the encoder is vp8.
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a63371a24dad13a4385f9e321ff31e0c7444269

commit 6a63371a24dad13a4385f9e321ff31e0c7444269
Author: Zijie He <zijiehe@chromium.org>
Date: Fri Nov 03 23:06:41 2017

[Chromoting] Implement EncoderBitrateFilter as a standalone component

This change moves EncoderBitrateFilter into a standalone component rather than
embedding in WebrtcFrameSchedulerSimple. The new implementation also provides a
parameter to control the minimum-bitrate. So each encoder implementation can use
a well-defined value to control the behavior of EncoderBitrateFilter.

It solves two problems:
1. Now, the minimum-bitrate is for VP8. The value is way to large for other
encoders.
2. Some encoder may not need "filter" at all, such as H264.

Besides the structure change, this CL also uses DescendingSamples to replace the
time-based average currently used in WebrtcFrameSchedulerSimple. It can help to
smooth the bitrate we have received from WebRTC and avoid a sudden change. See
the test cases for details.

As noted in the comments of encoder_bitrate_filter.cc file, we are still using
FPS to control the bandwidth usage, so smoothing the bitrate here is reasonable
to avoid a sudden change to the image quality.

Bug:  chromium:779417 , chromium:769391
Change-Id: I2936c50e39a4ba72d61364b72a94587b6ad7a57c
Reviewed-on: https://chromium-review.googlesource.com/750269
Commit-Queue: Zijie He <zijiehe@chromium.org>
Reviewed-by: Lambros Lambrou <lambroslambrou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513941}
[modify] https://crrev.com/6a63371a24dad13a4385f9e321ff31e0c7444269/remoting/base/BUILD.gn
[add] https://crrev.com/6a63371a24dad13a4385f9e321ff31e0c7444269/remoting/base/weighted_samples.cc
[add] https://crrev.com/6a63371a24dad13a4385f9e321ff31e0c7444269/remoting/base/weighted_samples.h
[add] https://crrev.com/6a63371a24dad13a4385f9e321ff31e0c7444269/remoting/base/weighted_samples_unittest.cc
[modify] https://crrev.com/6a63371a24dad13a4385f9e321ff31e0c7444269/remoting/codec/BUILD.gn
[add] https://crrev.com/6a63371a24dad13a4385f9e321ff31e0c7444269/remoting/codec/encoder_bitrate_filter.cc
[add] https://crrev.com/6a63371a24dad13a4385f9e321ff31e0c7444269/remoting/codec/encoder_bitrate_filter.h
[add] https://crrev.com/6a63371a24dad13a4385f9e321ff31e0c7444269/remoting/codec/encoder_bitrate_filter_unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c8529cb79a96b090fdb1178b17ec45a5842d4fdd

commit c8529cb79a96b090fdb1178b17ec45a5842d4fdd
Author: Zijie He <zijiehe@chromium.org>
Date: Wed Nov 08 04:45:09 2017

[Chromoting] Use EncoderBitrateFilter in encoder implementations

This change is straightforward, it uses EncoderBitrateFilter inside of each
WebrtcVideoEncoder implementation to give them more controls.
WebrtcFrameSchedulerSimple does not need to handle the 33% change rate anymore.

Bug: chromium:769391,  chromium:779417 
Change-Id: I4fa3f438646cdf9a2b6dc423d7d1909ebcb75d8f
Reviewed-on: https://chromium-review.googlesource.com/755459
Commit-Queue: Zijie He <zijiehe@chromium.org>
Reviewed-by: Lambros Lambrou <lambroslambrou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514745}
[modify] https://crrev.com/c8529cb79a96b090fdb1178b17ec45a5842d4fdd/remoting/codec/encoder_bitrate_filter.h
[modify] https://crrev.com/c8529cb79a96b090fdb1178b17ec45a5842d4fdd/remoting/codec/webrtc_video_encoder_gpu.cc
[modify] https://crrev.com/c8529cb79a96b090fdb1178b17ec45a5842d4fdd/remoting/codec/webrtc_video_encoder_gpu.h
[modify] https://crrev.com/c8529cb79a96b090fdb1178b17ec45a5842d4fdd/remoting/codec/webrtc_video_encoder_vpx.cc
[modify] https://crrev.com/c8529cb79a96b090fdb1178b17ec45a5842d4fdd/remoting/codec/webrtc_video_encoder_vpx.h
[modify] https://crrev.com/c8529cb79a96b090fdb1178b17ec45a5842d4fdd/remoting/protocol/webrtc_frame_scheduler_simple.cc
[modify] https://crrev.com/c8529cb79a96b090fdb1178b17ec45a5842d4fdd/remoting/protocol/webrtc_frame_scheduler_simple.h

Status: Fixed (was: Assigned)

Sign in to add a comment