Repository navigation
Add tests that receive sections do not gate a sending section's codecs - #1165
xianshijing-lk wants to merge 2 commits into
Conversation
A `recvonly` section's codec list and a `sendonly` section's share one BUNDLE group, which invites the assumption that restricting the first restricts the second — so a device whose decoder lacks a codec could not publish in it either. Single PC makes the assumption tempting, since it carries receive sections on the publisher connection from the start. A rust-sdks Jetson port reached for it to explain H.265 publishing falling back to VP8. It does not hold, in either negotiation order: with the receive sections restricted to VP8, the sending section still offers the probe codec, both when every transceiver predates the first offer and when the receive sections are negotiated first and the sender is added by a later renegotiation. Worth pinning, because the reasoning is plausible, the layout makes it recur, and the answer is a property of libwebrtc rather than of any one SDK. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Devin Review: 2 flags
Not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pblazej
left a comment
pblazej
left a comment
There was a problem hiding this comment.
Requesting changes for the second test, which never applies an answer. The first test is solid, and both pass locally.
|
|
||
| // First description: receive sections only, no H264 anywhere in it. | ||
| let first = try await transport.createOffer() | ||
| try await transport.set(localDescription: first) |
There was a problem hiding this comment.
issue (blocking): The second offer is created in have-local-offer with no answer applied, but on main the SFU answers before a publish renegotiates (Transport.swift:307 queues renegotiation until then).
Answer with a second local Transport as writesTheStartBitrateOnceForTheConnection does, require stable, then add the sender.
| target: .publisher, | ||
| primary: true, | ||
| delegate: StubTransportDelegate()) | ||
| defer { Task { await transport.close() } } |
There was a problem hiding this comment.
issue (non-blocking): Nothing awaits this close (same at line 114). Use withTransport from TransportMungeFallbackTests, moved to a shared file with StubTransportDelegate.
| let allVideo = RTC.videoSenderCapabilities.codecs | ||
| let vp8Only = allVideo.filter { $0.name.uppercased() == "VP8" || $0.name.uppercased() == "RTX" } | ||
| guard let probe = allVideo.first(where: { $0.name.uppercased() == "H264" }) else { | ||
| Issue.record("this libwebrtc build has no H264 sender capability to probe with") |
There was a problem hiding this comment.
suggestion (non-blocking): This return exits only the RTC.run closure, so the test fails again at line 94. Return a Bool and #require it outside the closure (same at lines 136-137).
| // First description: receive sections only, no H264 anywhere in it. | ||
| let first = try await transport.createOffer() | ||
| try await transport.set(localDescription: first) | ||
| #expect(!videoSections(first.sdp).contains { $0.codecs.contains("H264") }) |
There was a problem hiding this comment.
suggestion (non-blocking): This also passes with zero video sections. Require three VP8-only receive sections first, as lines 97-99 do.
| } | ||
| } | ||
|
|
||
| @Test func receiveSectionCodecsDoNotConstrainTheSendingSection() async throws { |
There was a problem hiding this comment.
polish: One @Test(arguments: [false, true]) test can cover both negotiation orders.
| let sending = try #require(sections.first { $0.direction == .sendonly }) | ||
| let receiving = sections.filter { $0.direction == .recvonly } | ||
|
|
||
| #expect(receiving.count == 3) |
There was a problem hiding this comment.
praise: These expectations keep the first test from passing vacuously.
Why
A
recvonlysection's codec list and asendonlysection's share one BUNDLE group, which invites the assumption that restricting the first restricts the second — i.e. that a device whose decoder lacks a codec can't publish in it either. Single PC makes the assumption especially tempting, since it carries receive sections on the publisher connection from the start.That assumption came up concretely: a rust-sdks Jetson port saw H.265 publishing fall back to VP8 under
single_peer_connection = true, and the preallocated receive transceivers were the leading suspect.What the tests establish
It doesn't hold, in either negotiation order. With three
recvonlyvideo sections restricted to VP8 (standing in for a decoder that supports nothing else), thesendonlysection still offers the probe codec:setLocalDescription, with no probe codec anywhere in that description, then the sender added by a later renegotiation. Still unaffected.So a codec absent from the first negotiated description can still be introduced later, and receive sections don't constrain the sending section's codec list.
Why pin it in Swift
The behaviour is a property of libwebrtc, not of any one SDK, so this answers the question for rust and JS too. The reasoning is plausible enough that it will be reached for again, and the single-PC layout makes it recur. Two fast tests against a local offline peer connection are cheap insurance against re-litigating it.
(For the record, the Jetson symptom is therefore something else — most likely
options.video_codecdefaulting toVideoCodec::VP8, or H.265 being absent fromget_rtp_sender_capabilities, in which casematchedis empty andset_codec_preferences([])silently resets preferences to libwebrtc's default order.)🤖 Generated with Claude Code