From 78bb4169b248ffacecfbb15a77d9f283eff7f1ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Thu, 23 Jan 2025 05:07:50 -0800 Subject: [PATCH] Re-enable and improve h264-unidirectional-codec.offer.https.html. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update WPTs to reflect WebRTC roll of [1] and add a new test. The H265 tests (both offer to send and offer to receive) now pass (virtual because H265 is behind flag). The H264 offer to send test no longer throws in setCodecPreferences() and it successfully negotiates H264, however the test still fails for reasons unrelated to the WebRTC fix, which is: - H264 always offers profile-level-id=64001f (3.1) regardless of which level we ask it to offer, in my case 640034 (5.2). This is https://crbug.com/391750825. The new test being added tests that even if we setCodecPreferences() with a recvonly H264 codec on a sendrecv transceiver, we should still offer H264 (but with a different level-id) because of codec filtering being based on ignoring levels. - This reveals a real bug in webrtc::IsSameRtpCodecIgnoringLevel that we should fix in a later CL. The problem is that it only ignores level for H265. Other codec comparison algorithms in WebRTC exists that do ignore levels for all codecs. [1] https://webrtc-review.googlesource.com/c/src/+/374520 Bug: chromium:381407888 Change-Id: I2586abc01a955168cb9da1a7f0834a3e0aee049c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6190353 Commit-Queue: Henrik Boström Auto-Submit: Henrik Boström Commit-Queue: Harald Alvestrand Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#1410251} --- ...h264-unidirectional-codec-offer.https.html | 122 +++++++++++------- 1 file changed, 78 insertions(+), 44 deletions(-) diff --git a/webrtc/protocol/h264-unidirectional-codec-offer.https.html b/webrtc/protocol/h264-unidirectional-codec-offer.https.html index 08640e1f62f9b2..3c4663b3bd2fe2 100644 --- a/webrtc/protocol/h264-unidirectional-codec-offer.https.html +++ b/webrtc/protocol/h264-unidirectional-codec-offer.https.html @@ -52,36 +52,31 @@ return codecs; } -// Returns an array of { payloadType, sdpFmtpLine } entries in the order that -// they appeared in the SDP. +// Returns an array of { mimeType, payloadType, sdpFmtpLine } entries in the +// order that they appeared in the SDP. function parseCodecsFromSdp(sdp) { const codecs = []; // For each a=rtpmap:... - const kRtpMapLineStart = 'a=rtpmap:'; - const kFmtpLineStart = 'a=fmtp:'; - for (let rtpmapIndex = 0;; ) { - rtpmapIndex = sdp.indexOf(kRtpMapLineStart, rtpmapIndex); - if (rtpmapIndex == -1) { - break; - } - rtpmapIndex += kRtpMapLineStart.length; - const payloadType = - Number(sdp.slice(rtpmapIndex, sdp.indexOf(' ', rtpmapIndex))); - const fmtpLineWithPT = kFmtpLineStart + payloadType; + const kRtpMapLineRegex = /\r\na=rtpmap:/g; + for (const match of sdp.matchAll(kRtpMapLineRegex)) { + const rtpmapIndex = match.index + 11; + const rtpmapSpaceIndex = sdp.indexOf(' ', rtpmapIndex) + const payloadType = Number(sdp.slice(rtpmapIndex, rtpmapSpaceIndex)); + const codecName = sdp.slice(rtpmapSpaceIndex + 1, + sdp.indexOf('/', rtpmapSpaceIndex)); + let sdpFmtpLine = undefined; // Can be undefined e.g. VP8. + const fmtpLineWithPT = `\r\na=fmtp:${payloadType} `; let fmtpIndex = sdp.indexOf(fmtpLineWithPT, rtpmapIndex); - if (fmtpIndex == -1) { - throw `Parse error: Missing expected ${fmtpLineWithPT} line`; - } - fmtpIndex += fmtpLineWithPT.length + 1; // +1 to skip the space separator. - const fmtpLineEnd = sdp.indexOf('\r\n', fmtpIndex); - if (fmtpLineEnd == -1) { - throw 'Parse error: Missing expected end of FMTP line'; + if (fmtpIndex != -1) { + fmtpIndex += fmtpLineWithPT.length; + const fmtpLineEnd = sdp.indexOf('\r\n', fmtpIndex); + if (fmtpLineEnd == -1) { + throw 'Parse error: Missing expected end of FMTP line'; + } + sdpFmtpLine = sdp.slice(fmtpIndex, fmtpLineEnd); } - const sdpFmtpLine = sdp.slice(fmtpIndex, fmtpLineEnd); - const codec = { payloadType, sdpFmtpLine }; + const codec = { mimeType: `video/${codecName}`, payloadType, sdpFmtpLine }; codecs.push(codec); - // Continue the loop after end of current fmtp line. - rtpmapIndex = fmtpLineEnd; } return codecs; } @@ -129,8 +124,8 @@ // makes modifying the SDP answer easier, but because we cannot send the // recvonly codec we have to fake it with remote SDP munging. const sendRecvCodecs = getCodecsWithDirection('video/H264', 'sendrecv'); - // If the folling optional asserts fail the test ends with - // [PRECONDITION_FAILED] as opposed to test failures. + // If any of the following optional asserts fail the test ends with + // [PRECONDITION_FAILED] as opposed to [FAIL]. assert_implements_optional( recvOnlyCodecs.length > 0, `There are no recvonly H264 codecs available in getCapabilities.`); @@ -147,10 +142,12 @@ await pc1.setLocalDescription(); const offeredCodecs = parseCodecsFromSdp(pc1.localDescription.sdp); assert_equals(offeredCodecs.length, 2, 'Two codecs should be offered'); - assert_equals(offeredCodecs[0].sdpFmtpLine, recvOnlyCodec.sdpFmtpLine, - 'The first offered codec should be the recvonly codec.'); - assert_equals(offeredCodecs[1].sdpFmtpLine, sendRecvCodec.sdpFmtpLine, - 'The second offered codec should be the sendrecv codec.'); + assert_equals(offeredCodecs[0].mimeType, 'video/H264'); + assert_true(offeredCodecs[0].sdpFmtpLine == recvOnlyCodec.sdpFmtpLine, + `The first offered codec's sdpFmtpLine is the recvonly one.`); + assert_equals(offeredCodecs[1].mimeType, 'video/H264'); + assert_true(offeredCodecs[1].sdpFmtpLine == sendRecvCodec.sdpFmtpLine, + `The second offered codec's sdpFmtpLine is the sendrecv one.`); await pc2.setRemoteDescription(pc1.localDescription); // Answer to send. @@ -161,15 +158,49 @@ // been removed from the SDP answer. const answeredCodecs = parseCodecsFromSdp(pc2.localDescription.sdp); assert_equals(answeredCodecs.length, 1, 'One codec should be answered'); - assert_equals(answeredCodecs[0].sdpFmtpLine, sendRecvCodec.sdpFmtpLine, - 'The first answered codec should be the sendrecv codec.'); + assert_equals(answeredCodecs[0].mimeType, 'video/H264'); + assert_true(answeredCodecs[0].sdpFmtpLine == sendRecvCodec.sdpFmtpLine, + `The answered codec's sdpFmtpLine is the sendrecv one.`); // Trick `pc1` into thinking `pc2` can send the codec by modifying the SDP. // Receiving media is not testable but this ensures that the SDP is accepted. const modifiedSdp = replaceCodecInSdp( pc2.localDescription.sdp, answeredCodecs[0], offeredCodecs[0]); await pc1.setRemoteDescription({type: 'answer', sdp: modifiedSdp}); -}, `Offer to receive a recvonly H264 codec`); +}, `Offer to receive a recvonly H264 codec on a recvonly transceiver`); + +promise_test(async t => { + const pc = new RTCPeerConnection(); + t.add_cleanup(() => pc.close()); + + const h264RecvOnlyCodecs = getCodecsWithDirection('video/H264', 'recvonly'); + const vp8SendRecvCodecs = getCodecsWithDirection('video/VP8', 'sendrecv'); + // If any of the following optional asserts fail the test ends with + // [PRECONDITION_FAILED] as opposed to [FAIL]. + assert_implements_optional( + h264RecvOnlyCodecs.length > 0, + `There are no recvonly H264 codecs available in getCapabilities.`); + assert_implements_optional( + vp8SendRecvCodecs.length > 0, + `There are no sendrecv VP8 codecs available in getCapabilities.`); + const h264RecvOnlyCodec = h264RecvOnlyCodecs[0]; + const vp8SendRecvCodec = vp8SendRecvCodecs[0]; + + const transceiver = pc.addTransceiver('video', {direction: 'sendrecv'}); + transceiver.setCodecPreferences([h264RecvOnlyCodec, vp8SendRecvCodec]); + await pc.setLocalDescription(); + const offeredCodecs = parseCodecsFromSdp(pc.localDescription.sdp); + // Even though this H264 codec with its level ID is recvonly, we should still + // offer to sendrecv H264 but with a different profile-level-id for sendrecv. + assert_equals(offeredCodecs.length, 2, 'Two codecs are offered (H264, VP8).'); + assert_equals(offeredCodecs[0].mimeType, 'video/H264', + 'The first offered codec is H264.'); + assert_true(offeredCodecs[0].sdpFmtpLine != h264RecvOnlyCodec.sdpFmtpLine, + 'The offered H264 profile-level-id should be different from ' + + 'the recvonly one.'); + assert_equals(offeredCodecs[1].mimeType, 'video/VP8', + 'The second offered codec is VP8.'); +}, `Offering a recvonly codec on a sendrecv transceiver`); promise_test(async t => { const pc1 = new RTCPeerConnection(); @@ -183,8 +214,8 @@ // makes modifying the SDP answer easier, but because we cannot receive the // sendonly codec we have to fake it with remote SDP munging. const sendRecvCodecs = getCodecsWithDirection('video/H264', 'sendrecv'); - // If the folling optional asserts fail the test ends with - // [PRECONDITION_FAILED] as opposed to test failures. + // If any of the following optional asserts fail the test ends with + // [PRECONDITION_FAILED] as opposed to [FAIL]. assert_implements_optional( sendOnlyCodecs.length > 0, `There are no sendonly H264 codecs available in getCapabilities.`); @@ -201,10 +232,12 @@ await pc1.setLocalDescription(); const offeredCodecs = parseCodecsFromSdp(pc1.localDescription.sdp); assert_equals(offeredCodecs.length, 2, 'Two codecs should be offered'); - assert_equals(offeredCodecs[0].sdpFmtpLine, sendOnlyCodec.sdpFmtpLine, - 'The first offered codec should be the sendonly codec.'); - assert_equals(offeredCodecs[1].sdpFmtpLine, sendRecvCodec.sdpFmtpLine, - 'The second offered codec should be the sendrecv codec.'); + assert_equals(offeredCodecs[0].mimeType, 'video/H264'); + assert_true(offeredCodecs[0].sdpFmtpLine == sendOnlyCodec.sdpFmtpLine, + `The first offered codec's sdpFmtpLine is the sendonly one.`); + assert_equals(offeredCodecs[1].mimeType, 'video/H264'); + assert_true(offeredCodecs[1].sdpFmtpLine == sendRecvCodec.sdpFmtpLine, + `The second offered codec's sdpFmtpLine is the sendrecv one.`); await pc2.setRemoteDescription(pc1.localDescription); // Answer to receive. @@ -213,8 +246,9 @@ // been removed from the SDP answer. const answeredCodecs = parseCodecsFromSdp(pc2.localDescription.sdp); assert_equals(answeredCodecs.length, 1, 'One codec should be answered'); - assert_equals(answeredCodecs[0].sdpFmtpLine, sendRecvCodec.sdpFmtpLine, - 'The first answered codec should be the sendrecv codec.'); + assert_equals(answeredCodecs[0].mimeType, 'video/H264'); + assert_true(answeredCodecs[0].sdpFmtpLine == sendRecvCodec.sdpFmtpLine, + `The answered codec's sdpFmtpLine is the sendrecv one.`); // Trick `pc1` into thinking `pc2` can receive the codec by modifying the SDP. const modifiedSdp = replaceCodecInSdp( pc2.localDescription.sdp, answeredCodecs[0], offeredCodecs[0]); @@ -226,7 +260,7 @@ `Only one codec should have been negotiated`); assert_equals(params.codecs[0].payloadType, offeredCodecs[0].payloadType, `The sendonly codec's payloadType shows up in getParameters()`); - assert_equals(params.codecs[0].sdpFmtpLine, offeredCodecs[0].sdpFmtpLine, - `The sendonly codec's sdpFmtpLine shows up in getParameters()`); -}, `Offer to send a sendonly H264 codec`); + assert_true(params.codecs[0].sdpFmtpLine == offeredCodecs[0].sdpFmtpLine, + `The sendonly codec's sdpFmtpLine shows up in getParameters()`); +}, `Offer to send a sendonly H264 codec on a sendonly transceiver`);