Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1904063003:
Fixing the interaction between codec bitrate limit and "b=AS". (Closed)
|
Created:
4 years, 8 months ago by Taylor Brandstetter Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFixing the interaction between codec bitrate limit and "b=AS".
This fixes a problem where "b=AS" and "x-google-start-bitrate" can't
be used together. It also starts taking the minimum of "b=AS" and
"x-google-max-bitrate", instead of just letting "b=AS" win.
BUG=webrtc:5811
R=pbos@webrtc.org
Committed: https://crrev.com/58f2bd90f14f38af2280c7379fad7d12dc402c7e
Cr-Commit-Position: refs/heads/master@{#12519}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Make "b=AS" take priority over "x-google-max-bitrate" for BWE max bitrate. #
Total comments: 1
Messages
Total messages: 17 (5 generated)
deadbeef@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2_unittest.cc:2436: 400000); Previously, max_bandwidth_bps was allowed to increase the max above the codec maximum. Was this intentional? This doesn't happen any more now that I'm taking the minimum of the two maximums.
https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2_unittest.cc:2436: 400000); On 2016/04/21 18:44:35, Taylor Brandstetter wrote: > Previously, max_bandwidth_bps was allowed to increase the max above the codec > maximum. Was this intentional? This doesn't happen any more now that I'm taking > the minimum of the two maximums. Yep, this was intentional, since we can apply FEC above the codec target bitrate.
Description was changed from ========== Fixing the interation between codec bitrate limit and "b=AS". This fixes a problem where "b=AS" and "x-google-start-bitrate" can't be used together. It also starts taking the minimum of "b=AS" and "x-google-max-bitrate", instead of just letting "b=AS" win. BUG=webrtc:5811 ========== to ========== Fixing the interaction between codec bitrate limit and "b=AS". This fixes a problem where "b=AS" and "x-google-start-bitrate" can't be used together. It also starts taking the minimum of "b=AS" and "x-google-max-bitrate", instead of just letting "b=AS" win. BUG=webrtc:5811 ==========
On 2016/04/21 18:57:24, pbos-webrtc wrote: > https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... > File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): > > https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... > webrtc/media/engine/webrtcvideoengine2_unittest.cc:2436: 400000); > On 2016/04/21 18:44:35, Taylor Brandstetter wrote: > > Previously, max_bandwidth_bps was allowed to increase the max above the codec > > maximum. Was this intentional? This doesn't happen any more now that I'm > taking > > the minimum of the two maximums. > > Yep, this was intentional, since we can apply FEC above the codec target > bitrate. In that case, why does the codec max bitrate affect the BWE max bitrate at all? Does that mean if you use x-google-max-bitrate without b=AS, you get no FEC at all (since the codec itself is using all the available bits)?
pbos@webrtc.org changed reviewers: + stefan@webrtc.org
On 2016/04/26 00:48:52, Taylor Brandstetter wrote: > On 2016/04/21 18:57:24, pbos-webrtc wrote: > > > https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... > > File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): > > > > > https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... > > webrtc/media/engine/webrtcvideoengine2_unittest.cc:2436: 400000); > > On 2016/04/21 18:44:35, Taylor Brandstetter wrote: > > > Previously, max_bandwidth_bps was allowed to increase the max above the > codec > > > maximum. Was this intentional? This doesn't happen any more now that I'm > > taking > > > the minimum of the two maximums. > > > > Yep, this was intentional, since we can apply FEC above the codec target > > bitrate. > > In that case, why does the codec max bitrate affect the BWE max bitrate at all? > Does that mean if you use x-google-max-bitrate without b=AS, you get no FEC at > all (since the codec itself is using all the available bits)? +stefan@ to chime in a second opinion. I think this is here for historical reasons. I think x-google-max-bitrate should set codec max bitrate, whereas b=AS should set "global" max bitrate. In the current code that means that we're allowed to overshoot x-google-max-bitrate when applying FEC though, so I'm not 100% this'll be expected, but I think that's my current suggestion. Unset b=AS = unlimited global bitrate, but x-google-max-bitrate as max for encoder target bitrate (and FEC can be applied on top). When we refactored this code, if I remember correctly, there was no "global cap" but only per-stream settings.
On 2016/04/26 15:01:00, pbos-webrtc wrote: > On 2016/04/26 00:48:52, Taylor Brandstetter wrote: > > On 2016/04/21 18:57:24, pbos-webrtc wrote: > > > > > > https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... > > > File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): > > > > > > > > > https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... > > > webrtc/media/engine/webrtcvideoengine2_unittest.cc:2436: 400000); > > > On 2016/04/21 18:44:35, Taylor Brandstetter wrote: > > > > Previously, max_bandwidth_bps was allowed to increase the max above the > > codec > > > > maximum. Was this intentional? This doesn't happen any more now that I'm > > > taking > > > > the minimum of the two maximums. > > > > > > Yep, this was intentional, since we can apply FEC above the codec target > > > bitrate. > > > > In that case, why does the codec max bitrate affect the BWE max bitrate at > all? > > Does that mean if you use x-google-max-bitrate without b=AS, you get no FEC at > > all (since the codec itself is using all the available bits)? > > +stefan@ to chime in a second opinion. > > I think this is here for historical reasons. I think x-google-max-bitrate should > set codec max bitrate, whereas b=AS should set "global" max bitrate. In the > current code that means that we're allowed to overshoot x-google-max-bitrate > when applying FEC though, so I'm not 100% this'll be expected, but I think > that's my current suggestion. Unset b=AS = unlimited global bitrate, but > x-google-max-bitrate as max for encoder target bitrate (and FEC can be applied > on top). > > When we refactored this code, if I remember correctly, there was no "global cap" > but only per-stream settings. What do you mean by "When we refactored this code"? I think as far back as I can see (https://webrtc-codereview.appspot.com/26199004/diff/270001/talk/media/webrtc/...), x-google-max-bitrate sets the global cap. Anyway, for now I'll just leave things as they were, and leave the "TODO" comment.
lgtm
Description was changed from ========== Fixing the interaction between codec bitrate limit and "b=AS". This fixes a problem where "b=AS" and "x-google-start-bitrate" can't be used together. It also starts taking the minimum of "b=AS" and "x-google-max-bitrate", instead of just letting "b=AS" win. BUG=webrtc:5811 ========== to ========== Fixing the interaction between codec bitrate limit and "b=AS". This fixes a problem where "b=AS" and "x-google-start-bitrate" can't be used together. It also starts taking the minimum of "b=AS" and "x-google-max-bitrate", instead of just letting "b=AS" win. BUG=webrtc:5811 R=pbos@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/58f2bd90f14f38af2280c7379... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 58f2bd90f14f38af2280c7379fad7d12dc402c7e (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Fixing the interaction between codec bitrate limit and "b=AS". This fixes a problem where "b=AS" and "x-google-start-bitrate" can't be used together. It also starts taking the minimum of "b=AS" and "x-google-max-bitrate", instead of just letting "b=AS" win. BUG=webrtc:5811 R=pbos@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/58f2bd90f14f38af2280c7379... ========== to ========== Fixing the interaction between codec bitrate limit and "b=AS". This fixes a problem where "b=AS" and "x-google-start-bitrate" can't be used together. It also starts taking the minimum of "b=AS" and "x-google-max-bitrate", instead of just letting "b=AS" win. BUG=webrtc:5811 R=pbos@webrtc.org Committed: https://crrev.com/58f2bd90f14f38af2280c7379fad7d12dc402c7e Cr-Commit-Position: refs/heads/master@{#12519} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/58f2bd90f14f38af2280c7379fad7d12dc402c7e Cr-Commit-Position: refs/heads/master@{#12519}
Message was sent while issue was closed.
On 2016/04/27 00:15:45, commit-bot: I haz the power wrote: > Patchset 2 (id:??) landed as > https://crrev.com/58f2bd90f14f38af2280c7379fad7d12dc402c7e > Cr-Commit-Position: refs/heads/master@{#12519} Just realized I forgot to update the description of this CL after patch set 2. Is it worth reverting and relanding to change the description?
Message was sent while issue was closed.
On 2016/04/27 00:19:51, Taylor Brandstetter wrote: > On 2016/04/27 00:15:45, commit-bot: I haz the power wrote: > > Patchset 2 (id:??) landed as > > https://crrev.com/58f2bd90f14f38af2280c7379fad7d12dc402c7e > > Cr-Commit-Position: refs/heads/master@{#12519} > > Just realized I forgot to update the description of this CL after patch set 2. > Is it worth reverting and relanding to change the description? No, don't think so. Lesson for next time. :)
Message was sent while issue was closed.
On 2016/04/26 15:01:00, pbos-webrtc wrote: > On 2016/04/26 00:48:52, Taylor Brandstetter wrote: > > On 2016/04/21 18:57:24, pbos-webrtc wrote: > > > > > > https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... > > > File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): > > > > > > > > > https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... > > > webrtc/media/engine/webrtcvideoengine2_unittest.cc:2436: 400000); > > > On 2016/04/21 18:44:35, Taylor Brandstetter wrote: > > > > Previously, max_bandwidth_bps was allowed to increase the max above the > > codec > > > > maximum. Was this intentional? This doesn't happen any more now that I'm > > > taking > > > > the minimum of the two maximums. > > > > > > Yep, this was intentional, since we can apply FEC above the codec target > > > bitrate. > > > > In that case, why does the codec max bitrate affect the BWE max bitrate at > all? > > Does that mean if you use x-google-max-bitrate without b=AS, you get no FEC at > > all (since the codec itself is using all the available bits)? > > +stefan@ to chime in a second opinion. > > I think this is here for historical reasons. I think x-google-max-bitrate should > set codec max bitrate, whereas b=AS should set "global" max bitrate. In the > current code that means that we're allowed to overshoot x-google-max-bitrate > when applying FEC though, so I'm not 100% this'll be expected, but I think > that's my current suggestion. Unset b=AS = unlimited global bitrate, but > x-google-max-bitrate as max for encoder target bitrate (and FEC can be applied > on top). > > When we refactored this code, if I remember correctly, there was no "global cap" > but only per-stream settings. You still get FEC, but at the cost of media bitrate. The interpretation of b=AS is roughly as you describe (it's a session max bw).
Message was sent while issue was closed.
https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1904063003/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2_unittest.cc:2436: 400000); On 2016/04/21 18:57:24, pbos-webrtc wrote: > On 2016/04/21 18:44:35, Taylor Brandstetter wrote: > > Previously, max_bandwidth_bps was allowed to increase the max above the codec > > maximum. Was this intentional? This doesn't happen any more now that I'm > taking > > the minimum of the two maximums. > > Yep, this was intentional, since we can apply FEC above the codec target > bitrate. Yes, what would otherwise happen if you have two send streams, one with a max of 200 and one with a max of 400? We don't want the max_bandwidth_bps to be limited to 200 or 400. https://codereview.webrtc.org/1904063003/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1904063003/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:833: // TODO(pbos): Figure out whether b=AS means max bitrate for this b=AS means session bandwidth, so my interpretation is that this is correct. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
