Repository navigation
Start published video at 90% of its target bitrate (x-google-start-bitrate)
#1121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| minor type="changed" "Published video starts at 90% of its target bitrate (capped at 1 Mbps unless it is a screen share) instead of libwebrtc's ~300 kbps default, by declaring `x-google-start-bitrate` on the video codecs' fmtp in the publisher's offer. The hint is one connection-level value — the largest among the sending video tracks — written to every video section, and only on the first offer that carries local video, since libwebrtc applies it to the whole peer connection and retains it across network route changes. Same behaviour as client-sdk-js, rust-sdks and client-sdk-android" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,6 +73,11 @@ struct SDPMediaSection { | |
|
|
||
| var mid: String? { attributeValue("mid") } | ||
|
|
||
| /// Track id of the section's `a=msid:<stream id> <track id>` line (RFC 8830 §2), if any. | ||
| var msidTrackId: String? { | ||
| attributeValue("msid")?.split(separator: " ").dropFirst().first.map(String.init) | ||
| } | ||
|
|
||
| /// The section's first direction attribute, if any. | ||
| var direction: SDPDirection? { | ||
| lines.dropFirst().lazy.compactMap(SDPDirection.init(line:)).first | ||
|
|
@@ -122,6 +127,31 @@ struct SDPMediaSection { | |
| return false | ||
| } | ||
|
|
||
| /// Sets `name=value` in the fmtp config of `payload`: replaces the parameter's value | ||
| /// if present, appends it to an existing fmtp line otherwise, and inserts a new | ||
| /// `a=fmtp:<payload> <name>=<value>` line at the end of the section when the payload | ||
| /// has none (e.g. VP8, which libwebrtc offers without an fmtp line). Returns `true` if | ||
| /// the section was modified; `false` when the parameter already carries `value`. | ||
| @discardableResult | ||
| mutating func setFmtpParameter(_ name: String, value: String, forPayload payload: String) -> Bool { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method always return true. We should update it so it doesn't have a return value, and make sure its comment is accurate.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It returns |
||
| let parameter = "\(name)=\(value)" | ||
| for (index, line) in lines.enumerated() { | ||
| guard let fmtp = Self.fmtp(fromLine: line), fmtp.payload == payload else { continue } | ||
| // Split without trimming so every other parameter is written back verbatim. | ||
| var pieces = fmtp.config.split(separator: ";", omittingEmptySubsequences: false).map(String.init) | ||
| if let existing = pieces.firstIndex(where: { $0.trimmingCharacters(in: .whitespaces).hasPrefix("\(name)=") }) { | ||
| guard pieces[existing].trimmingCharacters(in: .whitespaces) != parameter else { return false } | ||
| pieces[existing] = parameter | ||
| } else { | ||
| pieces.append(parameter) | ||
| } | ||
| lines[index] = "a=fmtp:\(payload) \(pieces.joined(separator: ";"))" | ||
| return true | ||
| } | ||
| lines.append("a=fmtp:\(payload) \(parameter)") | ||
| return true | ||
| } | ||
|
|
||
| /// Whether an `a=rtcp-fb:<payload> <value>` line with exactly `value` exists for | ||
| /// `payload`. Exact-value match: `nack` does not match `nack pli` (RFC 4585 §4.2 | ||
| /// — they are distinct feedback parameters). | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Bundled video loses start bitrate
When bundled send and receive sections share payload types,
mungeVideoStartBitrateedits only send sections. Their fmtp parameters then conflict, so libwebrtc can reject the hint and retry without it.Learn more
BUNDLE uses one RTP payload-type namespace across its media sections. Sections sharing a payload type must advertise identical codec parameters. This loop adds
x-google-start-bitrateonly to sending sections, while single-PC and pre-populated publisher layouts can contain receive-only video sections with the same payload types. The resulting descriptions disagree on fmtp for one payload type. The fallback inset(localDescription:munging:)then drops the start-bitrate munge if libwebrtc rejects that description, leaving the affected publication at the default ramp-up behavior.Example: A bundled send-only camera section and recv-only placeholder both advertise VP8 as payload 96. The munge adds
a=fmtp:96 x-google-start-bitrate=1000only to the camera section. The placeholder still has no fmtp for payload 96, so the bundled payload configurations differ and the hint can be rejected.Recommended fix: After computing the connection-level value, conform matching video codec fmtp across every non-rejected section in the BUNDLE group, including recv-only and inactive placeholders. Keep placeholder sections excluded only from choosing the bitrate. Add a real-peer-connection test containing one sending and one recv-only video section with a shared payload type.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x-google-start-bitrate is only a local encoder hint, so it shouldn't be needed for recv-only section.