Repository navigation
Seed the E2EE sender cryptor with the publish codec - #2131
Conversation
🦋 Changeset detectedLatest commit: 0f9dd59 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lukasIO
changed the title
There was a problem hiding this comment.
Generally makes sense to me - note I have not tested this, only reviewed the code by eye. I can attempt to test it but you understand the context more so I think you'd probably be the better person.
Is the reason this is still in draft because it still needs to be tested?
| track.mediaStreamID, | ||
| undefined, | ||
| isVideoTrack(track) | ||
| publishedCodec === 'h264' || publishedCodec === 'h265' ? publishedCodec : undefined, |
There was a problem hiding this comment.
question: I'm not sure if I understand why this has to only be h264 / h265 - based on the issue description, wouldn't be ok to be able to pass in any codec here and let downstream code fall back to this always if this.getVideoCodec(frame) returns a nullish value?
There was a problem hiding this comment.
The seed never reaches processNALUsForEncryption. It lands in this.videoCodec, which getUnencryptedBytes reads at FrameCryptor.ts:869 as this.getVideoCodec(frame) ?? this.videoCodec and then branches on before any byte detection runs: av1 throws at :880, vp8 returns at :885, vp9 at :888, and none of the three reach the NALU path at :898.
The value available here is publishOptions.videoCodec — the codec the client asked for, which negotiation can contradict — so a wrong vp8 seed would silently skip NALU processing and ship a VP8-shaped clear prefix on an H.264 frame. h264 and h265 are the only two values where guessing wrong is harmless: it costs one logged fallback, and since the frame bytes now outrank the seed it cannot move the offset at all.
The code side is tested: What is missing is a repro of the original Safari report. Nobody has reproduced it and there is no capture of the failing frame bytes, so I cannot claim this is the fix for that symptom — only that it fixes a real mis-detection on the same code path. That gap needs a Safari repro rather than another read of the diff. |
Drop a FrameCryptor test that passed with or without the seed, since NALU auto-detection returns h264 for its payload anyway. Keep the Uint8Array construction inside the try block and pass the caller's already-computed payloadType into the fallback log, so the logged value cannot disagree with the key that suppresses it. Hoist the duplicated isVideoTrack call, and dispose the manager in the seeding tests so they stop leaking a log-level listener.
detectCodecFromNALUs checked the h264 and h265 predicates inside one loop and returned on the first hit, so a non-slice h264 NALU that reads as an h265 slice type (AUD 0x09, SEI 0x06) outranked the real h264 slice behind it. Run the h264 pass to completion first instead; h265 headers are even at nuh_layer_id 0, so they cannot mask to an h264 slice type. processNALUsForEncryption also let knownCodec override detection outright. That codec can be the one the client asked for rather than the one that was negotiated, and parsing h264 as h265 lands the offset before the slice header and encrypts it without raising anything. Prefer a conclusive read of the bytes and keep knownCodec for the inconclusive case.
Co-authored-by: Ryan Gaus <ryan.gaus@livekit.io>
51388a2 to
2a55ad7
Compare
2 participants
The problem
The E2EE sender cryptor can encrypt the first frames of a publish without knowing the video codec.
LocalSenderCreatedfires insidenegotiate(), directly aftercreateSender. That event installs the encryption transform, so the encoder starts to produce frames from that point.LocalTrackPublishedfires much later, afteraddTrackand after the SDP round trip. The Safari-only handler inE2eeManagerpostsupdateCodecon that second event. For the whole window between the two events, the sender cryptor hasvideoCodec === undefined.Chrome does not care.
getUnencryptedBytesreadsthis.getVideoCodec(frame) ?? this.videoCodec, and the per-frame metadata carries the codec. Safari has neither source. There is nometadata.mimeType, and there is no payload type to look up in thertpMap. The codec must therefore come from byte detection on the frame itself. If that detection fails, the catch block falls back to the VP8 shape: a clear prefix of 1 or 10 bytes, andrequiresNALUProcessing: false. That also skips thewriteRbspcall. A subscriber cannot decode an H.264 frame that carries a VP8-shaped clear prefix and unescaped ciphertext.What this changes
E2eeManager.setupE2EESenderpassedundefinedas the codec argument ofhandleSender. It now passes the publish codec fromtrack.publishOptions?.videoCodec, limited toh264andh265.vp8seed would skip the NALU path completely. A wrong NALU seed costs one logged fallback at most.handleSender(codec?)already puts the codec on theEncodeMessage. The worker already passes it tosetupTransform, which applies it underif (codec). The laterLocalTrackPublishedcorrection therefore still wins and stays authoritative.The second part of the change is diagnostics.
logNALUFallbackOncelogged only the error and the payload type. It now also logs the detected codec, the frame type, the byte length, and the first 16 bytes as hex.What this does not do
This is a correctness fix and a diagnostics fix. It is not a verified Safari fix.
Nobody reproduced the symptom locally, and there is no capture of the failing frame bytes. The seed cannot change the encrypt result for a frame that already parses, because
processNALUsForEncryptionresolvesknownCodec ?? detectCodecFromNALUs(...)over the same NALU indices and the same slice predicate. The value of the seed is the other half: it turns a silentunknownearly return into a thrown error that reaches the new hex log. The two parts only work as a pair, so they ship together. The next user report will show whether Safari sends length-prefixed (AVCC) frames instead of Annex-B, or something else.This supersedes #1652, which touches
findNALUIndicesonly. That PR stays open. This change does not close it.Testing
src/e2ee/E2eeManager.test.ts: the seed reacheshandleSenderfor h264 and for h265, and vp8 stays unseeded.src/e2ee/worker/FrameCryptor.test.ts: the enriched fallback log fires once.npx tsc --noEmitclean.npx vitest run: 866 passed, 1 skipped.npm run formatclean.