Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1672173002:
Add initial bitrate and frame resolution parameters to quality scaler. (Closed)
|
Created:
4 years, 10 months ago by AlexG Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, perkj_webrtc, jackychen Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd initial bitrate and frame resolution parameters to quality scaler.
- Scale down to VGA immediately if call starts with HD resolution
and bitrate below 500 kbps.
- Adjust QP threshold for HW VP8 encoder to scale down faster.
BUG=b/26504665
R=mflodman@webrtc.org, pbos@webrtc.org, sprang@google.com, stefan@webrtc.org
Committed: https://crrev.com/a9d08929463e915f078860c206f2706c0284cba6
Cr-Commit-Position: refs/heads/master@{#11692}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments #
Total comments: 7
Patch Set 3 : Add test #Patch Set 4 : Add TODO item #
Total comments: 2
Patch Set 5 : Adding constants #
Total comments: 4
Patch Set 6 : Address comments - 3 #
Messages
Total messages: 49 (18 generated)
glaznev@webrtc.org changed reviewers: + pbos@webrtc.org
PTAL
ping :)
pbos@webrtc.org changed reviewers: + sprang@webrtc.org
Adding sprang@ so he can start getting familiar with this part of the code. :) https://codereview.webrtc.org/1672173002/diff/1/talk/app/webrtc/java/jni/andr... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1672173002/diff/1/talk/app/webrtc/java/jni/andr... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:403: const int kLowQpThresholdDenominator = 3; Let's just put a constant here without the denominator. https://codereview.webrtc.org/1672173002/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/quality_scaler.h (right): https://codereview.webrtc.org/1672173002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/quality_scaler.h:31: int initial_bitrate_kbps = 0, No default parameters.
On 2016/02/09 20:31:29, pbos-webrtc wrote: > Adding sprang@ so he can start getting familiar with this part of the code. :) > > https://codereview.webrtc.org/1672173002/diff/1/talk/app/webrtc/java/jni/andr... > File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): > > https://codereview.webrtc.org/1672173002/diff/1/talk/app/webrtc/java/jni/andr... > talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:403: const int > kLowQpThresholdDenominator = 3; > Let's just put a constant here without the denominator. > > https://codereview.webrtc.org/1672173002/diff/1/webrtc/modules/video_coding/u... > File webrtc/modules/video_coding/utility/quality_scaler.h (right): > > https://codereview.webrtc.org/1672173002/diff/1/webrtc/modules/video_coding/u... > webrtc/modules/video_coding/utility/quality_scaler.h:31: int > initial_bitrate_kbps = 0, > No default parameters. Overall I think it looks good btw.
PTAL https://codereview.webrtc.org/1672173002/diff/1/talk/app/webrtc/java/jni/andr... File talk/app/webrtc/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1672173002/diff/1/talk/app/webrtc/java/jni/andr... talk/app/webrtc/java/jni/androidmediaencoder_jni.cc:403: const int kLowQpThresholdDenominator = 3; On 2016/02/09 20:31:29, pbos-webrtc wrote: > Let's just put a constant here without the denominator. Done. https://codereview.webrtc.org/1672173002/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/quality_scaler.h (right): https://codereview.webrtc.org/1672173002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/quality_scaler.h:31: int initial_bitrate_kbps = 0, On 2016/02/09 20:31:29, pbos-webrtc wrote: > No default parameters. Done.
https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right): https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:604: kDisabledBadQpThreshold, false, 0, 0, 0); I'd like to have this here as well, as I think the issue impacts all platforms. https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:41: if (initial_bitrate_kbps > 0 && initial_bitrate_kbps < 500) { Add a TODO here to investigate other thresholds. https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right): https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:50: 0, 0, 0); Add tests for this with non-zero values.
PTAL https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right): https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:604: kDisabledBadQpThreshold, false, 0, 0, 0); On 2016/02/10 13:43:21, pbos-webrtc wrote: > I'd like to have this here as well, as I think the issue impacts all platforms. I think it is too early to enable this on all platforms - I guess BWE estimation will converge pretty fast in typical use cases resulting in unnecessary 5 seconds delay when switching to HD. I think to postpone this change in either follow up CL or until we can get configurable windows. https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:41: if (initial_bitrate_kbps > 0 && initial_bitrate_kbps < 500) { On 2016/02/10 13:43:21, pbos-webrtc wrote: > Add a TODO here to investigate other thresholds. Done. https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler_unittest.cc (right): https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler_unittest.cc:50: 0, 0, 0); On 2016/02/10 13:43:21, pbos-webrtc wrote: > Add tests for this with non-zero values. Done.
https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right): https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:604: kDisabledBadQpThreshold, false, 0, 0, 0); On 2016/02/10 19:07:16, AlexG wrote: > On 2016/02/10 13:43:21, pbos-webrtc wrote: > > I'd like to have this here as well, as I think the issue impacts all > platforms. > > I think it is too early to enable this on all platforms - I guess BWE estimation > will converge pretty fast in typical use cases resulting in unnecessary 5 > seconds delay when switching to HD. > I think to postpone this change in either follow up CL or until we can get > configurable windows. Initial bitrates, including keyframes, look bad at the initial 300k but VGA at HD bitrate looks really good. The only thing I'm a bit curious of is if the good QP thresholds always allow us to go back up. I think you might postpone it but please put a TODO here. sprang@: Consider checking this out. :)
On 2016/02/10 19:12:27, pbos-webrtc wrote: > https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right): > > https://codereview.webrtc.org/1672173002/diff/20001/webrtc/modules/video_codi... > webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:604: kDisabledBadQpThreshold, > false, 0, 0, 0); > On 2016/02/10 19:07:16, AlexG wrote: > > On 2016/02/10 13:43:21, pbos-webrtc wrote: > > > I'd like to have this here as well, as I think the issue impacts all > > platforms. > > > > I think it is too early to enable this on all platforms - I guess BWE > estimation > > will converge pretty fast in typical use cases resulting in unnecessary 5 > > seconds delay when switching to HD. > > I think to postpone this change in either follow up CL or until we can get > > configurable windows. > > Initial bitrates, including keyframes, look bad at the initial 300k but VGA at > HD bitrate looks really good. The only thing I'm a bit curious of is if the good > QP thresholds always allow us to go back up. > > I think you might postpone it but please put a TODO here. sprang@: Consider > checking this out. :) Done
lgtm but I'd like sprang to take a look (and you need owners I think)
Description was changed from ========== Add initial bitrate and frame resolution parameters to quality scaler. - Scale down to VGA immediately if call starts with HD resolution and bitrate below 500 kbps. - Adjust QP threshold for HW VP8 encoder to scale down faster. BUG=b/26504665 ========== to ========== Add initial bitrate and frame resolution parameters to quality scaler. - Scale down to VGA immediately if call starts with HD resolution and bitrate below 500 kbps. - Adjust QP threshold for HW VP8 encoder to scale down faster. BUG=b/26504665 ==========
glaznev@webrtc.org changed reviewers: + mflodman@webrtc.org
On 2016/02/10 19:31:06, pbos-webrtc wrote: > lgtm but I'd like sprang to take a look (and you need owners I think) +Magnus for owner approval
The CQ bit was checked by glaznev@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1672173002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672173002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/02/10 22:11:22, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Ping :) Magnus, can you please do owner approval
sprang@google.com changed reviewers: + sprang@google.com
lgtm with nit Sorry for the late reply, was sick a few days last week... https://codereview.webrtc.org/1672173002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/1672173002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:42: // ot threshold tables. s/investigate/Investigare s/ot/or Could we name these constants in the meantime? Or derive from current ones (kWidthVga * kHeightVga + 15% for instance).
lgtm
https://codereview.webrtc.org/1672173002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/1672173002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:42: // ot threshold tables. On 2016/02/15 13:20:21, sprang wrote: > s/investigate/Investigare s/ot/or > Could we name these constants in the meantime? Or derive from current ones > (kWidthVga * kHeightVga + 15% for instance). Done.
The CQ bit was checked by glaznev@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@google.com, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1672173002/#ps80001 (title: "Adding constants")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1672173002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672173002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3473)
Magnus, ping one more time for owner approval.
On 2016/02/17 18:29:49, AlexG wrote: > Magnus, ping one more time for owner approval. Stefan, as VCM owner, will take a look.
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
lgtm with nit fixed. https://codereview.webrtc.org/1672173002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/1672173002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:18: static const int kHDBitrateThresholdKbps = 500; kHd... (lower case "d") for both these constants.
LGTM CC Jacky as FTI.
Please fix the below. https://codereview.webrtc.org/1672173002/diff/80001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1672173002/diff/80001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediaencoder_jni.cc:401: codec_settings->width, Can you make sure that these get called with the new width/height if updated by the quality scaler so that we don't reconfigure this on the first encode call?
https://codereview.webrtc.org/1672173002/diff/80001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/1672173002/diff/80001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidmediaencoder_jni.cc:401: codec_settings->width, On 2016/02/19 19:17:09, pbos-webrtc wrote: > Can you make sure that these get called with the new width/height if updated by > the quality scaler so that we don't reconfigure this on the first encode call? Done. https://codereview.webrtc.org/1672173002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/quality_scaler.cc (right): https://codereview.webrtc.org/1672173002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/quality_scaler.cc:18: static const int kHDBitrateThresholdKbps = 500; On 2016/02/19 10:29:32, stefan-webrtc (holmer) wrote: > kHd... (lower case "d") for both these constants. Done.
The CQ bit was checked by glaznev@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@google.com, pbos@webrtc.org, stefan@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1672173002/#ps100001 (title: "Address comments - 3")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1672173002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672173002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/11065)
The CQ bit was checked by glaznev@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1672173002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672173002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/11067)
Description was changed from ========== Add initial bitrate and frame resolution parameters to quality scaler. - Scale down to VGA immediately if call starts with HD resolution and bitrate below 500 kbps. - Adjust QP threshold for HW VP8 encoder to scale down faster. BUG=b/26504665 ========== to ========== Add initial bitrate and frame resolution parameters to quality scaler. - Scale down to VGA immediately if call starts with HD resolution and bitrate below 500 kbps. - Adjust QP threshold for HW VP8 encoder to scale down faster. BUG=b/26504665 R=mflodman@webrtc.org, pbos@webrtc.org, sprang@google.com, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/a9d08929463e915f078860c20... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as a9d08929463e915f078860c206f2706c0284cba6 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Add initial bitrate and frame resolution parameters to quality scaler. - Scale down to VGA immediately if call starts with HD resolution and bitrate below 500 kbps. - Adjust QP threshold for HW VP8 encoder to scale down faster. BUG=b/26504665 R=mflodman@webrtc.org, pbos@webrtc.org, sprang@google.com, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/a9d08929463e915f078860c20... ========== to ========== Add initial bitrate and frame resolution parameters to quality scaler. - Scale down to VGA immediately if call starts with HD resolution and bitrate below 500 kbps. - Adjust QP threshold for HW VP8 encoder to scale down faster. BUG=b/26504665 R=mflodman@webrtc.org, pbos@webrtc.org, sprang@google.com, stefan@webrtc.org Committed: https://crrev.com/a9d08929463e915f078860c206f2706c0284cba6 Cr-Commit-Position: refs/heads/master@{#11692} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a9d08929463e915f078860c206f2706c0284cba6 Cr-Commit-Position: refs/heads/master@{#11692} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
