New issue
Advanced search Search tips

Issue 903012 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Today
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

[remoting WebRTC] Upgrade host to Unified Plan SDP semantics

Project Member Reported by lambroslambrou@chromium.org, Nov 8

Issue description

The host process currently doesn't specify SDP semtics when creating the WebRTC PeerConnection.
This means that it currently uses "Plan B" semantics, which will soon be deprecated.

See the transition plan here:
https://webrtc.org/web-apis/chrome/unified-plan/
This includes links to migration guides for Javascript and native apps.

 
It requires moving to the track-based APIs, which appears to be straightforward. However, the SDP munging that we do is broken by Unified Plan (I haven't figured out why). For example, the x-google-max-bitrate setting is no longer applied.

It is much better to replace the SDP munging with newer APIs first, rather than spend time figuring out how to get the munging to work with Unified Plan. This should not be difficult, given that we already use the newer RtpSender::SetParameters() API to set max-bandwidth for relayed connections.

Blockedon: 833538
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 12

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

commit 28d99dc2341f1f7e30372c5ab1ff1d62a6813af1
Author: Lambros Lambrou <lambroslambrou@chromium.org>
Date: Mon Nov 12 20:07:20 2018

[remoting WebRTC] Specify Plan-B semantics for SDP.

Plan-B is deprecated, but some non-trivial work is needed to update to
Unified Plan. Specifying Plan-B explicitly will keep things working
when WebRTC changes the default to Unified Plan.

Bug:  903012 
Change-Id: I3afc86c87f79af068f93af384cc20229293be5c5
Reviewed-on: https://chromium-review.googlesource.com/c/1327546
Reviewed-by: Joe Downing <joedow@chromium.org>
Commit-Queue: Lambros Lambrou <lambroslambrou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607312}
[modify] https://crrev.com/28d99dc2341f1f7e30372c5ab1ff1d62a6813af1/remoting/protocol/webrtc_transport.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 16

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

commit 3fc40106b0c32d06ac3d5ebc59a78565263c6ec6
Author: Lambros Lambrou <lambroslambrou@chromium.org>
Date: Wed Jan 16 01:56:56 2019

Set bitrate caps using newer APIs

This removes the x-google-[min/max]-bitrate SDP parameters used to set
the bandwidth caps for the connection. Instead, the global caps are
set using PeerConnection::SetBitrate(), and the VideoSender caps are
set to the same values using RtpSender::SetParameters(), which was
already being used to set the max-bitrate cap for relay connections.
It appears that both global and per-sender caps are needed to
properly configure the b/w estimator and ALR probers, otherwise the
bitrate is severely constrained by WebRTC defaults (~600kbps).

This CL unblocks the UnifiedPlan migration, and ensures we are using
consistent APIs to set the bitrate caps for both direct and relayed
connections.

Bug:  903012 , 833538
Change-Id: Iefaddf877071f0fc3a010cf02a2b1fd2a92ad00c
Reviewed-on: https://chromium-review.googlesource.com/c/1409710
Reviewed-by: Joe Downing <joedow@chromium.org>
Commit-Queue: Lambros Lambrou <lambroslambrou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623014}
[modify] https://crrev.com/3fc40106b0c32d06ac3d5ebc59a78565263c6ec6/remoting/protocol/webrtc_transport.cc

Comment 5 by lambroslambrou@chromium.org, Jan 17 (6 days ago)

Blockedon: -833538
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 18 (5 days ago)

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

commit 51ce8f7f2e4c17d097880bab7b2a8ef4d03751f2
Author: Lambros Lambrou <lambroslambrou@chromium.org>
Date: Fri Jan 18 03:50:31 2019

Upgrade remoting WebRTC host to Unified Plan

This sets SdpSemantics::kUnifiedPlan for the PeerConnection, and updates
the code to use the PeerConnection [Add/Remove]Track() APIs, as the old
Stream-based APIs cannot be used with Unified Plan.

Verified (with Chrome browser) that b/w caps and estimation work as
expected for direct and relayed connections. Also verified that audio is
still encoded and received in stereo at ~160kbps.

Bug:  903012 
Change-Id: I1d165738664ef819b1ed7c52d016d95bc6f5e2fc
Reviewed-on: https://chromium-review.googlesource.com/c/1396839
Commit-Queue: Lambros Lambrou <lambroslambrou@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623997}
[modify] https://crrev.com/51ce8f7f2e4c17d097880bab7b2a8ef4d03751f2/remoting/protocol/webrtc_audio_stream.cc
[modify] https://crrev.com/51ce8f7f2e4c17d097880bab7b2a8ef4d03751f2/remoting/protocol/webrtc_audio_stream.h
[modify] https://crrev.com/51ce8f7f2e4c17d097880bab7b2a8ef4d03751f2/remoting/protocol/webrtc_transport.cc
[modify] https://crrev.com/51ce8f7f2e4c17d097880bab7b2a8ef4d03751f2/remoting/protocol/webrtc_video_stream.cc
[modify] https://crrev.com/51ce8f7f2e4c17d097880bab7b2a8ef4d03751f2/remoting/protocol/webrtc_video_stream.h

Comment 7 by lambroslambrou@chromium.org, Today (8 hours ago)

Status: Fixed (was: Started)

Sign in to add a comment