Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740783006:
Support removing b=AS bandwidth constraints. (Closed)
|
Created:
3 years, 9 months ago by pbos-webrtc Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSupport removing b=AS bandwidth constraints.
In code this is represented as setting -1 to max_bandwidth_bps, but this
value was being ignored by webrtcvideoengine2.cc, so previous restrictions
would still apply.
BUG=webrtc:6202
TEST=Setting "unlimited" for Bandwidth in Chromium in https://webrtc.github.io/samples/src/content/peerconnection/bandwidth/.
R=deadbeef@webrtc.org,stefan@webrtc.org
Review-Url: https://codereview.webrtc.org/2740783006
Cr-Commit-Position: refs/heads/master@{#17175}
Committed: https://chromium.googlesource.com/external/webrtc/+/5c7760a3a2a72b905705e003f62b60eabfda7ff2
Patch Set 1 #
Total comments: 6
Patch Set 2 : do minimal change #Patch Set 3 : add back useful todo change #
Total comments: 2
Patch Set 4 : update comment #
Messages
Total messages: 31 (16 generated)
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
PTAL, I'd like to make sure you both agree that separating codec max bitrate from Call max bitrate is the right thing. I also think that bitrate estimator min bitrate shouldn't be set from VideoCodec settings, though I'm not sure if that's right to do within this CL too. All BWE limits affect both voice and video, so having them set even separately at all is weird. stefan@ I think you have a TODO in webrtcvideoengine2.cc to reconsider this. Can we look at that? This came up because a test started failing in WRVE2 when I made -1 reset bitrates, because the default (do not cap) came into conflict with codec max. I think it's sensible to cap codec with encoder config (stream max bitrate), and cap the bitrate estimator with b=AS. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Remove bandwidth constraints by removing b=AS. Also removes setting global call bitrate limitations based on codec max bitrate settings (they should be constrained within their encoder settings either way). BUG=webrtc:6202 R=deadbeef@webrtc.org,solenberg@webrtc.org,stefan@webrtc.org ========== to ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. Also removes setting global call bitrate limitations based on codec max bitrate settings (they should be constrained within their encoder settings either way). BUG=webrtc:6202 R=deadbeef@webrtc.org,solenberg@webrtc.org,stefan@webrtc.org ==========
lgtm. Though I wouldn't be surprised if some applications are depending on the old behavior of x-google-max-bitrate. https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcmed... File webrtc/media/engine/webrtcmediaengine.cc (right): https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcmed... webrtc/media/engine/webrtcmediaengine.cc:211: // bitrate config. nit: This seems independent enough to warrant being in a standalone CL. Also, this comment is a little confusing to me. Maybe something like this would be more clear: "The global max bitrate should only be restricted by 'b=AS', and not by codec-specific bitrate limits, so leave it as -1 here. The codec-specific bitrate limits will be enforced using the encoder configuration rather than the global BitrateConfig." https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:721: // auto-bitrate value. The "correct" behavior for b=AS:0 may be "set bitrate to 0, don't send any bits". I can't find any reference to 0 being a special value. But we should change that separately. https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2_unittest.cc:3866: // max_bandwidth_bps = -1 - remove the bandwidth limit Hey, one inconsistency fixed. :)
https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcmed... File webrtc/media/engine/webrtcmediaengine.cc (right): https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcmed... webrtc/media/engine/webrtcmediaengine.cc:211: // bitrate config. On 2017/03/09 23:48:38, Taylor Brandstetter wrote: > nit: This seems independent enough to warrant being in a standalone CL. > > Also, this comment is a little confusing to me. Maybe something like this would > be more clear: > > "The global max bitrate should only be restricted by 'b=AS', and not by > codec-specific bitrate limits, so leave it as -1 here. The codec-specific > bitrate limits will be enforced using the encoder configuration rather than the > global BitrateConfig." I agree that we should do this independently. And I'm pretty sure there are applications relying on this, which is why it hasn't been fixed yet... Let's talk offline about what needs to be done to fix this.
You'll have to do similar changes in webrtcvoiceengine.cc
solenberg@webrtc.org changed reviewers: + ossu@webrtc.org
Passing the buck to ossu@ who's been looking into the codec config logic lately.
solenberg@webrtc.org changed reviewers: - solenberg@webrtc.org
On 2017/03/10 11:56:49, the sun wrote: > Passing the buck to ossu@ who's been looking into the codec config logic lately. I'll update this to only accept unset b=AS, to avoid the more complex codec discussion.
Description was changed from ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. Also removes setting global call bitrate limitations based on codec max bitrate settings (they should be constrained within their encoder settings either way). BUG=webrtc:6202 R=deadbeef@webrtc.org,solenberg@webrtc.org,stefan@webrtc.org ========== to ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. BUG=webrtc:6202 R=deadbeef@webrtc.org,solenberg@webrtc.org,stefan@webrtc.org ==========
do minimal change
PTAL, this doesn't touch voice now.
Description was changed from ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. BUG=webrtc:6202 R=deadbeef@webrtc.org,solenberg@webrtc.org,stefan@webrtc.org ========== to ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. BUG=webrtc:6202 TEST=Setting unlimited for Bandwidth in https://webrtc.github.io/samples/src/content/peerconnection/bandwidth/. R=deadbeef@webrtc.org,solenberg@webrtc.org,stefan@webrtc.org ==========
Description was changed from ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. BUG=webrtc:6202 TEST=Setting unlimited for Bandwidth in https://webrtc.github.io/samples/src/content/peerconnection/bandwidth/. R=deadbeef@webrtc.org,solenberg@webrtc.org,stefan@webrtc.org ========== to ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. BUG=webrtc:6202 TEST=Setting "unlimited" for Bandwidth in Chrome in https://webrtc.github.io/samples/src/content/peerconnection/bandwidth/. R=deadbeef@webrtc.org,solenberg@webrtc.org,stefan@webrtc.org ==========
Description was changed from ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. BUG=webrtc:6202 TEST=Setting "unlimited" for Bandwidth in Chrome in https://webrtc.github.io/samples/src/content/peerconnection/bandwidth/. R=deadbeef@webrtc.org,solenberg@webrtc.org,stefan@webrtc.org ========== to ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. BUG=webrtc:6202 TEST=Setting "unlimited" for Bandwidth in Chromium in https://webrtc.github.io/samples/src/content/peerconnection/bandwidth/. R=deadbeef@webrtc.org,solenberg@webrtc.org,stefan@webrtc.org ==========
add back useful todo change
lgtm https://codereview.webrtc.org/2740783006/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2740783006/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:769: // bitrate should probably not affect global call max bitrate). nit: I think this comment would be unclear to someone not familiar with this code and what the different "max"es are. I'd suggest something like: // Unset the global max bitrate (max_bitrate_bps) if max_bandwidth_bps // is -1, which corresponds to no "b=AS" attribute in SDP. Note that the // global max bitrate may be set below in GetBitrateConfigForCodec, // from the codec max bitrate. // TODO(deadbeef): This should be reconsidered (codec max bitrate should // probably not affect global call max bitrate).
LGTM with comment improved as suggested by Taylor.
Description was changed from ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. BUG=webrtc:6202 TEST=Setting "unlimited" for Bandwidth in Chromium in https://webrtc.github.io/samples/src/content/peerconnection/bandwidth/. R=deadbeef@webrtc.org,solenberg@webrtc.org,stefan@webrtc.org ========== to ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. BUG=webrtc:6202 TEST=Setting "unlimited" for Bandwidth in Chromium in https://webrtc.github.io/samples/src/content/peerconnection/bandwidth/. R=deadbeef@webrtc.org,stefan@webrtc.org ==========
update comment
https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcmed... File webrtc/media/engine/webrtcmediaengine.cc (right): https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcmed... webrtc/media/engine/webrtcmediaengine.cc:211: // bitrate config. On 2017/03/10 08:14:53, stefan-webrtc wrote: > On 2017/03/09 23:48:38, Taylor Brandstetter wrote: > > nit: This seems independent enough to warrant being in a standalone CL. > > > > Also, this comment is a little confusing to me. Maybe something like this > would > > be more clear: > > > > "The global max bitrate should only be restricted by 'b=AS', and not by > > codec-specific bitrate limits, so leave it as -1 here. The codec-specific > > bitrate limits will be enforced using the encoder configuration rather than > the > > global BitrateConfig." > > I agree that we should do this independently. And I'm pretty sure there are > applications relying on this, which is why it hasn't been fixed yet... > > Let's talk offline about what needs to be done to fix this. Abandoned. https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2740783006/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:721: // auto-bitrate value. On 2017/03/09 23:48:38, Taylor Brandstetter wrote: > The "correct" behavior for b=AS:0 may be "set bitrate to 0, don't send any > bits". I can't find any reference to 0 being a special value. But we should > change that separately. TODO text updated. https://codereview.webrtc.org/2740783006/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2740783006/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:769: // bitrate should probably not affect global call max bitrate). On 2017/03/10 18:42:08, Taylor Brandstetter wrote: > nit: I think this comment would be unclear to someone not familiar with this > code and what the different "max"es are. I'd suggest something like: > > // Unset the global max bitrate (max_bitrate_bps) if max_bandwidth_bps > // is -1, which corresponds to no "b=AS" attribute in SDP. Note that the > // global max bitrate may be set below in GetBitrateConfigForCodec, > // from the codec max bitrate. > // TODO(deadbeef): This should be reconsidered (codec max bitrate should > // probably not affect global call max bitrate). Done. (But with my ldap.)
The CQ bit was checked by pbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2740783006/#ps60001 (title: "update comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489171937431530,
"parent_rev": "9eef3c737000edaabab639121d639fab56aa9cb8", "commit_rev":
"5c7760a3a2a72b905705e003f62b60eabfda7ff2"}
Message was sent while issue was closed.
Description was changed from ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. BUG=webrtc:6202 TEST=Setting "unlimited" for Bandwidth in Chromium in https://webrtc.github.io/samples/src/content/peerconnection/bandwidth/. R=deadbeef@webrtc.org,stefan@webrtc.org ========== to ========== Support removing b=AS bandwidth constraints. In code this is represented as setting -1 to max_bandwidth_bps, but this value was being ignored by webrtcvideoengine2.cc, so previous restrictions would still apply. BUG=webrtc:6202 TEST=Setting "unlimited" for Bandwidth in Chromium in https://webrtc.github.io/samples/src/content/peerconnection/bandwidth/. R=deadbeef@webrtc.org,stefan@webrtc.org Review-Url: https://codereview.webrtc.org/2740783006 Cr-Commit-Position: refs/heads/master@{#17175} Committed: https://chromium.googlesource.com/external/webrtc/+/5c7760a3a2a72b905705e003f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/5c7760a3a2a72b905705e003f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
