From 6e8adf065ae1db227166e7e7bc8c771c9bfacdea Mon Sep 17 00:00:00 2001 From: aler9 <46489434+aler9@users.noreply.github.com> Date: Mon, 27 May 2024 22:43:33 +0200 Subject: [PATCH] Fix AV1 and VP9 codec matching Currently, AV1 or VP9 formats are matched regardless of the profile parameter. This was not noticeable until browsers started advertising multiple VP9 formats at the same time, each with a different profile ID, in order to allow the counterpart to send different streams on the basis of supported profiles. This causes two issues: first, the library includes in the SDP all formats passed by the browser, regardless of the fact that the profile ID is registered in the API or not. Then, the library is unable to choose the correct format for streaming, causing an intermittent failure. This patch fixes the matching algorithm and also covers the case in which the profile ID is missing, by using values dictated by specifications. Tests were refactored since previous ones covered the same lines multiple times. --- internal/fmtp/av1.go | 40 +++ internal/fmtp/fmtp.go | 45 +++- internal/fmtp/fmtp_test.go | 538 +++++++++++++++++++++++++++++++------ internal/fmtp/h264_test.go | 145 ---------- internal/fmtp/vp9.go | 40 +++ 5 files changed, 573 insertions(+), 235 deletions(-) create mode 100644 internal/fmtp/av1.go delete mode 100644 internal/fmtp/h264_test.go create mode 100644 internal/fmtp/vp9.go diff --git a/internal/fmtp/av1.go b/internal/fmtp/av1.go new file mode 100644 index 00000000000..59d8b31a8d1 --- /dev/null +++ b/internal/fmtp/av1.go @@ -0,0 +1,40 @@ +// SPDX-FileCopyrightText: 2023 The Pion community +// SPDX-License-Identifier: MIT + +package fmtp + +type av1FMTP struct { + parameters map[string]string +} + +func (h *av1FMTP) MimeType() string { + return "video/av1" +} + +func (h *av1FMTP) Match(b FMTP) bool { + c, ok := b.(*av1FMTP) + if !ok { + return false + } + + // AV1 RTP specification: + // If the profile parameter is not present, it MUST be inferred to be 0 (“Main” profile). + hProfile, ok := h.parameters["profile"] + if !ok { + hProfile = "0" + } + cProfile, ok := c.parameters["profile"] + if !ok { + cProfile = "0" + } + if hProfile != cProfile { + return false + } + + return true +} + +func (h *av1FMTP) Parameter(key string) (string, bool) { + v, ok := h.parameters[key] + return v, ok +} diff --git a/internal/fmtp/fmtp.go b/internal/fmtp/fmtp.go index 5461c019bd2..f515a648d01 100644 --- a/internal/fmtp/fmtp.go +++ b/internal/fmtp/fmtp.go @@ -8,6 +8,22 @@ import ( "strings" ) +func parseParameters(line string) map[string]string { + parameters := make(map[string]string) + + for _, p := range strings.Split(line, ";") { + pp := strings.SplitN(strings.TrimSpace(p), "=", 2) + key := strings.ToLower(pp[0]) + var value string + if len(pp) > 1 { + value = pp[1] + } + parameters[key] = value + } + + return parameters +} + // FMTP interface for implementing custom // FMTP parsers based on MimeType type FMTP interface { @@ -23,29 +39,30 @@ type FMTP interface { } // Parse parses an fmtp string based on the MimeType -func Parse(mimetype, line string) FMTP { +func Parse(mimeType, line string) FMTP { var f FMTP - parameters := make(map[string]string) - - for _, p := range strings.Split(line, ";") { - pp := strings.SplitN(strings.TrimSpace(p), "=", 2) - key := strings.ToLower(pp[0]) - var value string - if len(pp) > 1 { - value = pp[1] - } - parameters[key] = value - } + parameters := parseParameters(line) switch { - case strings.EqualFold(mimetype, "video/h264"): + case strings.EqualFold(mimeType, "video/h264"): f = &h264FMTP{ parameters: parameters, } + + case strings.EqualFold(mimeType, "video/vp9"): + f = &vp9FMTP{ + parameters: parameters, + } + + case strings.EqualFold(mimeType, "video/av1"): + f = &av1FMTP{ + parameters: parameters, + } + default: f = &genericFMTP{ - mimeType: mimetype, + mimeType: mimeType, parameters: parameters, } } diff --git a/internal/fmtp/fmtp_test.go b/internal/fmtp/fmtp_test.go index 2bc2bc873e7..96c1a8e7434 100644 --- a/internal/fmtp/fmtp_test.go +++ b/internal/fmtp/fmtp_test.go @@ -8,127 +8,513 @@ import ( "testing" ) -func TestGenericParseFmtp(t *testing.T) { - testCases := map[string]struct { - input string +func TestParseParameters(t *testing.T) { + for _, ca := range []struct { + name string + line string + parameters map[string]string + }{ + { + "one param", + "key-name=value", + map[string]string{ + "key-name": "value", + }, + }, + { + "one param with white spaces", + "\tkey-name=value ", + map[string]string{ + "key-name": "value", + }, + }, + { + "two params", + "key-name=value;key2=value2", + map[string]string{ + "key-name": "value", + "key2": "value2", + }, + }, + { + "two params with white spaces", + "key-name=value; \n\tkey2=value2 ", + map[string]string{ + "key-name": "value", + "key2": "value2", + }, + }, + } { + t.Run(ca.name, func(t *testing.T) { + parameters := parseParameters(ca.line) + if !reflect.DeepEqual(parameters, ca.parameters) { + t.Errorf("expected '%v', got '%v'", ca.parameters, parameters) + } + }) + } +} + +func TestParse(t *testing.T) { + for _, ca := range []struct { + name string + mimeType string + line string expected FMTP }{ - "OneParam": { - input: "key-name=value", - expected: &genericFMTP{ + { + "generic", + "generic", + "key-name=value", + &genericFMTP{ mimeType: "generic", parameters: map[string]string{ "key-name": "value", }, }, }, - "OneParamWithWhiteSpeces": { - input: "\tkey-name=value ", - expected: &genericFMTP{ + { + "generic case normalization", + "generic", + "Key=value", + &genericFMTP{ mimeType: "generic", + parameters: map[string]string{ + "key": "value", + }, + }, + }, + { + "h264", + "video/h264", + "key-name=value", + &h264FMTP{ parameters: map[string]string{ "key-name": "value", }, }, }, - "TwoParams": { - input: "key-name=value;key2=value2", - expected: &genericFMTP{ - mimeType: "generic", + { + "vp9", + "video/vp9", + "key-name=value", + &vp9FMTP{ parameters: map[string]string{ "key-name": "value", - "key2": "value2", }, }, }, - "TwoParamsWithWhiteSpeces": { - input: "key-name=value; \n\tkey2=value2 ", - expected: &genericFMTP{ - mimeType: "generic", + { + "av1", + "video/av1", + "key-name=value", + &av1FMTP{ parameters: map[string]string{ "key-name": "value", - "key2": "value2", }, }, }, - } - for name, testCase := range testCases { - testCase := testCase - t.Run(name, func(t *testing.T) { - f := Parse("generic", testCase.input) - if !reflect.DeepEqual(testCase.expected, f) { - t.Errorf("Expected Fmtp params: %v, got: %v", testCase.expected, f) + } { + t.Run(ca.name, func(t *testing.T) { + f := Parse(ca.mimeType, ca.line) + if !reflect.DeepEqual(ca.expected, f) { + t.Errorf("expected '%v', got '%v'", ca.expected, f) } - if f.MimeType() != "generic" { - t.Errorf("Expected MimeType of generic, got: %s", f.MimeType()) + if f.MimeType() != ca.mimeType { + t.Errorf("Expected '%v', got '%s'", ca.mimeType, f.MimeType()) } }) } } -func TestGenericFmtpCompare(t *testing.T) { +func TestMatch(t *testing.T) { consistString := map[bool]string{true: "consist", false: "inconsist"} - testCases := map[string]struct { - a, b string + for _, ca := range []struct { + name string + a FMTP + b FMTP consist bool }{ - "Equal": { - a: "key1=value1;key2=value2;key3=value3", - b: "key1=value1;key2=value2;key3=value3", - consist: true, - }, - "EqualWithWhitespaceVariants": { - a: "key1=value1;key2=value2;key3=value3", - b: " key1=value1; \nkey2=value2;\t\nkey3=value3", - consist: true, - }, - "EqualWithCase": { - a: "key1=value1;key2=value2;key3=value3", - b: "key1=value1;key2=Value2;Key3=value3", - consist: true, - }, - "OneHasExtraParam": { - a: "key1=value1;key2=value2;key3=value3", - b: "key1=value1;key2=value2;key3=value3;key4=value4", - consist: true, - }, - "Inconsistent": { - a: "key1=value1;key2=value2;key3=value3", - b: "key1=value1;key2=different_value;key3=value3", - consist: false, - }, - "Inconsistent_OneHasExtraParam": { - a: "key1=value1;key2=value2;key3=value3;key4=value4", - b: "key1=value1;key2=different_value;key3=value3", - consist: false, + { + "generic equal", + &genericFMTP{ + mimeType: "generic", + parameters: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }, + }, + &genericFMTP{ + mimeType: "generic", + parameters: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }, + }, + true, }, - } - for name, testCase := range testCases { - testCase := testCase - check := func(t *testing.T, a, b string) { - aa := Parse("", a) - bb := Parse("", b) - c := aa.Match(bb) - if c != testCase.consist { + { + "generic one extra param", + &genericFMTP{ + mimeType: "generic", + parameters: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }, + }, + &genericFMTP{ + mimeType: "generic", + parameters: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + "key4": "value4", + }, + }, + true, + }, + { + "generic inconsistent different kind", + &genericFMTP{ + mimeType: "generic", + parameters: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }, + }, + &h264FMTP{}, + false, + }, + { + "generic inconsistent different mime type", + &genericFMTP{ + mimeType: "generic1", + parameters: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }, + }, + &genericFMTP{ + mimeType: "generic2", + parameters: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }, + }, + false, + }, + { + "generic inconsistent different parameters", + &genericFMTP{ + mimeType: "generic", + parameters: map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }, + }, + &genericFMTP{ + mimeType: "generic", + parameters: map[string]string{ + "key1": "value1", + "key2": "different_value", + "key3": "value3", + }, + }, + false, + }, + { + "h264 equal", + &h264FMTP{ + parameters: map[string]string{ + "level-asymmetry-allowed": "1", + "packetization-mode": "1", + "profile-level-id": "42e01f", + }, + }, + &h264FMTP{ + parameters: map[string]string{ + "level-asymmetry-allowed": "1", + "packetization-mode": "1", + "profile-level-id": "42e01f", + }, + }, + true, + }, + { + "h264 one extra param", + &h264FMTP{ + parameters: map[string]string{ + "level-asymmetry-allowed": "1", + "packetization-mode": "1", + "profile-level-id": "42e01f", + }, + }, + &h264FMTP{ + parameters: map[string]string{ + "packetization-mode": "1", + "profile-level-id": "42e01f", + }, + }, + true, + }, + { + "h264 different profile level ids version", + &h264FMTP{ + parameters: map[string]string{ + "packetization-mode": "1", + "profile-level-id": "42e01f", + }, + }, + &h264FMTP{ + parameters: map[string]string{ + "packetization-mode": "1", + "profile-level-id": "42e029", + }, + }, + true, + }, + { + "h264 inconsistent different kind", + &h264FMTP{ + parameters: map[string]string{ + "packetization-mode": "0", + "profile-level-id": "42e01f", + }, + }, + &genericFMTP{}, + false, + }, + { + "h264 inconsistent different parameters", + &h264FMTP{ + parameters: map[string]string{ + "packetization-mode": "0", + "profile-level-id": "42e01f", + }, + }, + &h264FMTP{ + parameters: map[string]string{ + "packetization-mode": "1", + "profile-level-id": "42e01f", + }, + }, + false, + }, + { + "h264 inconsistent missing packetization mode", + &h264FMTP{ + parameters: map[string]string{ + "packetization-mode": "0", + "profile-level-id": "42e01f", + }, + }, + &h264FMTP{ + parameters: map[string]string{ + "profile-level-id": "42e01f", + }, + }, + false, + }, + { + "h264 inconsistent missing profile level id", + &h264FMTP{ + parameters: map[string]string{ + "packetization-mode": "1", + "profile-level-id": "42e01f", + }, + }, + &h264FMTP{ + parameters: map[string]string{ + "packetization-mode": "1", + }, + }, + false, + }, + { + "h264 inconsistent invalid profile level id", + &h264FMTP{ + parameters: map[string]string{ + "packetization-mode": "1", + "profile-level-id": "42e029", + }, + }, + &h264FMTP{ + parameters: map[string]string{ + "packetization-mode": "1", + "profile-level-id": "41e029", + }, + }, + false, + }, + { + "vp9 equal", + &vp9FMTP{ + parameters: map[string]string{ + "profile-id": "1", + }, + }, + &vp9FMTP{ + parameters: map[string]string{ + "profile-id": "1", + }, + }, + true, + }, + { + "vp9 missing profile", + &vp9FMTP{ + parameters: map[string]string{}, + }, + &vp9FMTP{ + parameters: map[string]string{}, + }, + true, + }, + { + "vp9 inferred profile", + &vp9FMTP{ + parameters: map[string]string{ + "profile-id": "0", + }, + }, + &vp9FMTP{ + parameters: map[string]string{}, + }, + true, + }, + { + "vp9 inconsistent different kind", + &vp9FMTP{ + parameters: map[string]string{ + "profile-id": "0", + }, + }, + &genericFMTP{}, + false, + }, + { + "vp9 inconsistent different profile", + &vp9FMTP{ + parameters: map[string]string{ + "profile-id": "0", + }, + }, + &vp9FMTP{ + parameters: map[string]string{ + "profile-id": "1", + }, + }, + false, + }, + { + "vp9 inconsistent different inferred profile", + &vp9FMTP{ + parameters: map[string]string{}, + }, + &vp9FMTP{ + parameters: map[string]string{ + "profile-id": "1", + }, + }, + false, + }, + { + "av1 equal", + &av1FMTP{ + parameters: map[string]string{ + "profile": "1", + }, + }, + &av1FMTP{ + parameters: map[string]string{ + "profile": "1", + }, + }, + true, + }, + { + "av1 missing profile", + &av1FMTP{ + parameters: map[string]string{}, + }, + &av1FMTP{ + parameters: map[string]string{}, + }, + true, + }, + { + "av1 inferred profile", + &av1FMTP{ + parameters: map[string]string{ + "profile": "0", + }, + }, + &av1FMTP{ + parameters: map[string]string{}, + }, + true, + }, + { + "av1 inconsistent different kind", + &av1FMTP{ + parameters: map[string]string{ + "profile": "0", + }, + }, + &genericFMTP{}, + false, + }, + { + "av1 inconsistent different profile", + &av1FMTP{ + parameters: map[string]string{ + "profile": "0", + }, + }, + &av1FMTP{ + parameters: map[string]string{ + "profile": "1", + }, + }, + false, + }, + { + "av1 inconsistent different inferred profile", + &av1FMTP{ + parameters: map[string]string{}, + }, + &av1FMTP{ + parameters: map[string]string{ + "profile": "1", + }, + }, + false, + }, + } { + t.Run(ca.name, func(t *testing.T) { + c := ca.a.Match(ca.b) + if c != ca.consist { t.Errorf( "'%s' and '%s' are expected to be %s, but treated as %s", - a, b, consistString[testCase.consist], consistString[c], + ca.a, ca.b, consistString[ca.consist], consistString[c], ) } - // test reverse case here - c = bb.Match(aa) - if c != testCase.consist { + c = ca.b.Match(ca.a) + if c != ca.consist { t.Errorf( "'%s' and '%s' are expected to be %s, but treated as %s", - a, b, consistString[testCase.consist], consistString[c], + ca.a, ca.b, consistString[ca.consist], consistString[c], ) } - } - t.Run(name, func(t *testing.T) { - check(t, testCase.a, testCase.b) }) } } diff --git a/internal/fmtp/h264_test.go b/internal/fmtp/h264_test.go deleted file mode 100644 index 93da80d5e71..00000000000 --- a/internal/fmtp/h264_test.go +++ /dev/null @@ -1,145 +0,0 @@ -// SPDX-FileCopyrightText: 2023 The Pion community -// SPDX-License-Identifier: MIT - -package fmtp - -import ( - "reflect" - "testing" -) - -func TestH264FMTPParse(t *testing.T) { - testCases := map[string]struct { - input string - expected FMTP - }{ - "OneParam": { - input: "key-name=value", - expected: &h264FMTP{ - parameters: map[string]string{ - "key-name": "value", - }, - }, - }, - "OneParamWithWhiteSpeces": { - input: "\tkey-name=value ", - expected: &h264FMTP{ - parameters: map[string]string{ - "key-name": "value", - }, - }, - }, - "TwoParams": { - input: "key-name=value;key2=value2", - expected: &h264FMTP{ - parameters: map[string]string{ - "key-name": "value", - "key2": "value2", - }, - }, - }, - "TwoParamsWithWhiteSpeces": { - input: "key-name=value; \n\tkey2=value2 ", - expected: &h264FMTP{ - parameters: map[string]string{ - "key-name": "value", - "key2": "value2", - }, - }, - }, - } - for name, testCase := range testCases { - testCase := testCase - t.Run(name, func(t *testing.T) { - f := Parse("video/h264", testCase.input) - if !reflect.DeepEqual(testCase.expected, f) { - t.Errorf("Expected Fmtp params: %v, got: %v", testCase.expected, f) - } - - if f.MimeType() != "video/h264" { - t.Errorf("Expected MimeType of video/h264, got: %s", f.MimeType()) - } - }) - } -} - -func TestH264FMTPCompare(t *testing.T) { - consistString := map[bool]string{true: "consist", false: "inconsist"} - - testCases := map[string]struct { - a, b string - consist bool - }{ - "Equal": { - a: "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f", - b: "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f", - consist: true, - }, - "EqualWithWhitespaceVariants": { - a: "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f", - b: " level-asymmetry-allowed=1; \npacketization-mode=1;\t\nprofile-level-id=42e01f", - consist: true, - }, - "EqualWithCase": { - a: "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f", - b: "level-asymmetry-allowed=1;packetization-mode=1;PROFILE-LEVEL-ID=42e01f", - consist: true, - }, - "OneHasExtraParam": { - a: "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f", - b: "packetization-mode=1;profile-level-id=42e01f", - consist: true, - }, - "DifferentProfileLevelIDVersions": { - a: "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f", - b: "packetization-mode=1;profile-level-id=42e029", - consist: true, - }, - "Inconsistent": { - a: "packetization-mode=1;profile-level-id=42e029", - b: "packetization-mode=0;profile-level-id=42e029", - consist: false, - }, - "Inconsistent_MissingPacketizationMode": { - a: "packetization-mode=1;profile-level-id=42e029", - b: "profile-level-id=42e029", - consist: false, - }, - "Inconsistent_MissingProfileLevelID": { - a: "packetization-mode=1;profile-level-id=42e029", - b: "packetization-mode=1", - consist: false, - }, - "Inconsistent_InvalidProfileLevelID": { - a: "packetization-mode=1;profile-level-id=42e029", - b: "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=41e029", - consist: false, - }, - } - for name, testCase := range testCases { - testCase := testCase - check := func(t *testing.T, a, b string) { - aa := Parse("video/h264", a) - bb := Parse("video/h264", b) - c := aa.Match(bb) - if c != testCase.consist { - t.Errorf( - "'%s' and '%s' are expected to be %s, but treated as %s", - a, b, consistString[testCase.consist], consistString[c], - ) - } - - // test reverse case here - c = bb.Match(aa) - if c != testCase.consist { - t.Errorf( - "'%s' and '%s' are expected to be %s, but treated as %s", - a, b, consistString[testCase.consist], consistString[c], - ) - } - } - t.Run(name, func(t *testing.T) { - check(t, testCase.a, testCase.b) - }) - } -} diff --git a/internal/fmtp/vp9.go b/internal/fmtp/vp9.go new file mode 100644 index 00000000000..9400fe02b6f --- /dev/null +++ b/internal/fmtp/vp9.go @@ -0,0 +1,40 @@ +// SPDX-FileCopyrightText: 2023 The Pion community +// SPDX-License-Identifier: MIT + +package fmtp + +type vp9FMTP struct { + parameters map[string]string +} + +func (h *vp9FMTP) MimeType() string { + return "video/vp9" +} + +func (h *vp9FMTP) Match(b FMTP) bool { + c, ok := b.(*vp9FMTP) + if !ok { + return false + } + + // draft-ietf-payload-vp9-16: + // If no profile-id is present, Profile 0 MUST be inferred + hProfileID, ok := h.parameters["profile-id"] + if !ok { + hProfileID = "0" + } + cProfileID, ok := c.parameters["profile-id"] + if !ok { + cProfileID = "0" + } + if hProfileID != cProfileID { + return false + } + + return true +} + +func (h *vp9FMTP) Parameter(key string) (string, bool) { + v, ok := h.parameters[key] + return v, ok +}