From 2d529be5717a7c8a454abbafad4c6851d30ca793 Mon Sep 17 00:00:00 2001 From: Ryan Shumate Date: Thu, 29 Jul 2021 16:10:12 -0400 Subject: [PATCH] Improved h264 fmtp line parsing Implements h264 fmtp parsing based on RFC 6184 Section 8.2.2 --- fmtp.go | 37 ------ internal/fmtp/fmtp.go | 92 +++++++++++++ fmtp_test.go => internal/fmtp/fmtp_test.go | 62 ++++++--- internal/fmtp/h264.go | 80 ++++++++++++ internal/fmtp/h264_test.go | 142 +++++++++++++++++++++ mediaengine.go | 5 +- rtpcodec.go | 8 +- 7 files changed, 365 insertions(+), 61 deletions(-) delete mode 100644 fmtp.go create mode 100644 internal/fmtp/fmtp.go rename fmtp_test.go => internal/fmtp/fmtp_test.go (65%) create mode 100644 internal/fmtp/h264.go create mode 100644 internal/fmtp/h264_test.go diff --git a/fmtp.go b/fmtp.go deleted file mode 100644 index 9d4be5da645..00000000000 --- a/fmtp.go +++ /dev/null @@ -1,37 +0,0 @@ -package webrtc - -import ( - "strings" -) - -type fmtp map[string]string - -// parseFmtp parses fmtp string. -func parseFmtp(line string) fmtp { - f := fmtp{} - 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] - } - f[key] = value - } - return f -} - -// fmtpConsist checks that two FMTP parameters are not inconsistent. -func fmtpConsist(a, b fmtp) bool { - for k, v := range a { - if vb, ok := b[k]; ok && !strings.EqualFold(vb, v) { - return false - } - } - for k, v := range b { - if va, ok := a[k]; ok && !strings.EqualFold(va, v) { - return false - } - } - return true -} diff --git a/internal/fmtp/fmtp.go b/internal/fmtp/fmtp.go new file mode 100644 index 00000000000..86057594acf --- /dev/null +++ b/internal/fmtp/fmtp.go @@ -0,0 +1,92 @@ +// Package fmtp implements per codec parsing of fmtp lines +package fmtp + +import ( + "strings" +) + +// FMTP interface for implementing custom +// FMTP parsers based on MimeType +type FMTP interface { + // MimeType returns the MimeType associated with + // the fmtp + MimeType() string + // Match compares two fmtp descriptions for + // compatibility based on the MimeType + Match(f FMTP) bool + // Parameter returns a value for the associated key + // if contained in the parsed fmtp string + Parameter(key string) (string, bool) +} + +// Parse parses an fmtp string based on the MimeType +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 + } + + switch { + case strings.EqualFold(mimetype, "video/h264"): + f = &h264FMTP{ + parameters: parameters, + } + default: + f = &genericFMTP{ + mimeType: mimetype, + parameters: parameters, + } + } + + return f +} + +type genericFMTP struct { + mimeType string + parameters map[string]string +} + +func (g *genericFMTP) MimeType() string { + return g.mimeType +} + +// Match returns true if g and b are compatible fmtp descriptions +// The generic implementation is used for MimeTypes that are not defined +func (g *genericFMTP) Match(b FMTP) bool { + c, ok := b.(*genericFMTP) + if !ok { + return false + } + + if g.mimeType != c.MimeType() { + return false + } + + for k, v := range g.parameters { + if vb, ok := c.parameters[k]; ok && !strings.EqualFold(vb, v) { + return false + } + } + + for k, v := range c.parameters { + if va, ok := g.parameters[k]; ok && !strings.EqualFold(va, v) { + return false + } + } + + return true +} + +func (g *genericFMTP) Parameter(key string) (string, bool) { + v, ok := g.parameters[key] + return v, ok +} diff --git a/fmtp_test.go b/internal/fmtp/fmtp_test.go similarity index 65% rename from fmtp_test.go rename to internal/fmtp/fmtp_test.go index 3f0a498e78b..0ec8b567346 100644 --- a/fmtp_test.go +++ b/internal/fmtp/fmtp_test.go @@ -1,54 +1,70 @@ -package webrtc +package fmtp import ( "reflect" "testing" ) -func TestParseFmtp(t *testing.T) { +func TestGenericParseFmtp(t *testing.T) { testCases := map[string]struct { input string - expected fmtp + expected FMTP }{ "OneParam": { input: "key-name=value", - expected: fmtp{ - "key-name": "value", + expected: &genericFMTP{ + mimeType: "generic", + parameters: map[string]string{ + "key-name": "value", + }, }, }, "OneParamWithWhiteSpeces": { input: "\tkey-name=value ", - expected: fmtp{ - "key-name": "value", + expected: &genericFMTP{ + mimeType: "generic", + parameters: map[string]string{ + "key-name": "value", + }, }, }, "TwoParams": { input: "key-name=value;key2=value2", - expected: fmtp{ - "key-name": "value", - "key2": "value2", + expected: &genericFMTP{ + mimeType: "generic", + parameters: map[string]string{ + "key-name": "value", + "key2": "value2", + }, }, }, "TwoParamsWithWhiteSpeces": { input: "key-name=value; \n\tkey2=value2 ", - expected: fmtp{ - "key-name": "value", - "key2": "value2", + expected: &genericFMTP{ + mimeType: "generic", + parameters: map[string]string{ + "key-name": "value", + "key2": "value2", + }, }, }, } for name, testCase := range testCases { testCase := testCase t.Run(name, func(t *testing.T) { - f := parseFmtp(testCase.input) + f := Parse("generic", testCase.input) if !reflect.DeepEqual(testCase.expected, f) { t.Errorf("Expected Fmtp params: %v, got: %v", testCase.expected, f) } + + if f.MimeType() != "generic" { + t.Errorf("Expected MimeType of generic, got: %s", f.MimeType()) + } }) } } -func TestFmtpConsist(t *testing.T) { +func TestGenericFmtpCompare(t *testing.T) { consistString := map[bool]string{true: "consist", false: "inconsist"} testCases := map[string]struct { @@ -89,7 +105,18 @@ func TestFmtpConsist(t *testing.T) { for name, testCase := range testCases { testCase := testCase check := func(t *testing.T, a, b string) { - c := fmtpConsist(parseFmtp(a), parseFmtp(b)) + aa := Parse("", a) + bb := Parse("", 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", @@ -100,8 +127,5 @@ func TestFmtpConsist(t *testing.T) { t.Run(name, func(t *testing.T) { check(t, testCase.a, testCase.b) }) - t.Run(name+"_Reversed", func(t *testing.T) { - check(t, testCase.b, testCase.a) - }) } } diff --git a/internal/fmtp/h264.go b/internal/fmtp/h264.go new file mode 100644 index 00000000000..5a79b9e64a4 --- /dev/null +++ b/internal/fmtp/h264.go @@ -0,0 +1,80 @@ +package fmtp + +import ( + "encoding/hex" +) + +func profileLevelIDMatches(a, b string) bool { + aa, err := hex.DecodeString(a) + if err != nil || len(aa) < 2 { + return false + } + bb, err := hex.DecodeString(b) + if err != nil || len(bb) < 2 { + return false + } + return aa[0] == bb[0] && aa[1] == bb[1] +} + +type h264FMTP struct { + parameters map[string]string +} + +func (h *h264FMTP) MimeType() string { + return "video/h264" +} + +// Match returns true if h and b are compatible fmtp descriptions +// Based on RFC6184 Section 8.2.2: +// The parameters identifying a media format configuration for H.264 +// are profile-level-id and packetization-mode. These media format +// configuration parameters (except for the level part of profile- +// level-id) MUST be used symmetrically; that is, the answerer MUST +// either maintain all configuration parameters or remove the media +// format (payload type) completely if one or more of the parameter +// values are not supported. +// Informative note: The requirement for symmetric use does not +// apply for the level part of profile-level-id and does not apply +// for the other stream properties and capability parameters. +func (h *h264FMTP) Match(b FMTP) bool { + c, ok := b.(*h264FMTP) + if !ok { + return false + } + + // test packetization-mode + hpmode, hok := h.parameters["packetization-mode"] + if !hok { + return false + } + cpmode, cok := c.parameters["packetization-mode"] + if !cok { + return false + } + + if hpmode != cpmode { + return false + } + + // test profile-level-id + hplid, hok := h.parameters["profile-level-id"] + if !hok { + return false + } + + cplid, cok := c.parameters["profile-level-id"] + if !cok { + return false + } + + if !profileLevelIDMatches(hplid, cplid) { + return false + } + + return true +} + +func (h *h264FMTP) Parameter(key string) (string, bool) { + v, ok := h.parameters[key] + return v, ok +} diff --git a/internal/fmtp/h264_test.go b/internal/fmtp/h264_test.go new file mode 100644 index 00000000000..1e8fc97d441 --- /dev/null +++ b/internal/fmtp/h264_test.go @@ -0,0 +1,142 @@ +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/mediaengine.go b/mediaengine.go index 196780417cc..b26351cf610 100644 --- a/mediaengine.go +++ b/mediaengine.go @@ -12,6 +12,7 @@ import ( "github.com/pion/rtp/v2" "github.com/pion/rtp/v2/codecs" "github.com/pion/sdp/v3" + "github.com/pion/webrtc/v3/internal/fmtp" ) const ( @@ -372,8 +373,8 @@ func (m *MediaEngine) matchRemoteCodec(remoteCodec RTPCodecParameters, typ RTPCo codecs = m.audioCodecs } - remoteFmtp := parseFmtp(remoteCodec.RTPCodecCapability.SDPFmtpLine) - if apt, hasApt := remoteFmtp["apt"]; hasApt { + remoteFmtp := fmtp.Parse(remoteCodec.RTPCodecCapability.MimeType, remoteCodec.RTPCodecCapability.SDPFmtpLine) + if apt, hasApt := remoteFmtp.Parameter("apt"); hasApt { payloadType, err := strconv.Atoi(apt) if err != nil { return codecMatchNone, err diff --git a/rtpcodec.go b/rtpcodec.go index 30a0cd908f4..cde2b8e7183 100644 --- a/rtpcodec.go +++ b/rtpcodec.go @@ -2,6 +2,8 @@ package webrtc import ( "strings" + + "github.com/pion/webrtc/v3/internal/fmtp" ) // RTPCodecType determines the type of a codec @@ -97,12 +99,12 @@ const ( // Used for lookup up a codec in an existing list to find a match // Returns codecMatchExact, codecMatchPartial, or codecMatchNone func codecParametersFuzzySearch(needle RTPCodecParameters, haystack []RTPCodecParameters) (RTPCodecParameters, codecMatchType) { - needleFmtp := parseFmtp(needle.RTPCodecCapability.SDPFmtpLine) + needleFmtp := fmtp.Parse(needle.RTPCodecCapability.MimeType, needle.RTPCodecCapability.SDPFmtpLine) // First attempt to match on MimeType + SDPFmtpLine for _, c := range haystack { - if strings.EqualFold(c.RTPCodecCapability.MimeType, needle.RTPCodecCapability.MimeType) && - fmtpConsist(needleFmtp, parseFmtp(c.RTPCodecCapability.SDPFmtpLine)) { + cfmtp := fmtp.Parse(c.RTPCodecCapability.MimeType, c.RTPCodecCapability.SDPFmtpLine) + if needleFmtp.Match(cfmtp) { return c, codecMatchExact } }