Repository: pion/webrtc
Pull request #1682 (2021-02-17 07:05:05)

Fix updateHeaderExtension for cloned MediaEngine
#### Description
It addresses the issue that unable to updateHeaderExtension for cloned
MediaEngine, which is resulted from previous commit:

c8b7aa386a92100a0377fd32660c810d6e4e7e28

#### Reference issue
Fixes a possible(?) side-effect from the PR for #1662 


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/1682?src=pr&el=h1) Report
> Merging [#1682](https://codecov.io/gh/pion/webrtc/pull/1682?src=pr&el=desc) (7c1f7aa) into [master](https://codecov.io/gh/pion/webrtc/commit/19537f2807068fcfb7423bbaa063995bbaddf503?el=desc) (19537f2) will **decrease** coverage by `5.12%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pion/webrtc/pull/1682/graphs/tree.svg?width=650&height=150&src=pr&token=XOOuQo07I2)](https://codecov.io/gh/pion/webrtc/pull/1682?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1682      +/-   ##
==========================================
- Coverage   75.02%   69.90%   -5.13%     
==========================================
  Files          79       57      -22     
  Lines        5718     2143    -3575     
==========================================
- Hits         4290     1498    -2792     
+ Misses       1055      542     -513     
+ Partials      373      103     -270     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `69.90% <ø> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/1682?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [icetcp.go](https://codecov.io/gh/pion/webrtc/pull/1682/diff?src=pr&el=tree#diff-aWNldGNwLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/1682/diff?src=pr&el=tree#diff-cnRwY29kZWMuZ28=) | `0.00% <0.00%> (-89.48%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/1682/diff?src=pr&el=tree#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/1682/diff?src=pr&el=tree#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/1682/diff?src=pr&el=tree#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-67.86%)` | :arrow_down: |
| [internal/mux/muxfunc.go](https://codecov.io/gh/pion/webrtc/pull/1682/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbXV4L211eGZ1bmMuZ28=) | `0.00% <0.00%> (-47.83%)` | :arrow_down: |
| [gathering\_complete\_promise.go](https://codecov.io/gh/pion/webrtc/pull/1682/diff?src=pr&el=tree#diff-Z2F0aGVyaW5nX2NvbXBsZXRlX3Byb21pc2UuZ28=) | `60.00% <0.00%> (-40.00%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/1682/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `22.22% <0.00%> (-38.89%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/1682/diff?src=pr&el=tree#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-37.50%)` | :arrow_down: |
| [dtlsrole.go](https://codecov.io/gh/pion/webrtc/pull/1682/diff?src=pr&el=tree#diff-ZHRsc3JvbGUuZ28=) | `68.96% <0.00%> (-31.04%)` | :arrow_down: |
| ... and [30 more](https://codecov.io/gh/pion/webrtc/pull/1682/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/webrtc/pull/1682?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/webrtc/pull/1682?src=pr&el=footer). Last update [19537f2...7c1f7aa](https://codecov.io/gh/pion/webrtc/pull/1682?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

mission-liao: OK, would provide another commit to make it always initialize later. It would be less complicated.
mission-liao: I've looked around codes for `negotiatedHeaderExtensions`, and think it's still better to keep the last commit.

The behavior: `updateHeaderExtension would do nothing if RegisterHeaderExtension is not called` seems reasonable.

Being of two minds.

Patch:
diff --git a/mediaengine.go b/mediaengine.go
index 95527eec..3418086c 100644
--- a/mediaengine.go
+++ b/mediaengine.go
@@ -284,11 +284,15 @@ func (m *MediaEngine) getHeaderExtensionID(extension RTPHeaderExtensionCapabilit
 // copy copies any user modifiable state of the MediaEngine
 // all internal state is reset
 func (m *MediaEngine) copy() *MediaEngine {
-	return &MediaEngine{
+	cloned := &MediaEngine{
 		videoCodecs:      append([]RTPCodecParameters{}, m.videoCodecs...),
 		audioCodecs:      append([]RTPCodecParameters{}, m.audioCodecs...),
 		headerExtensions: append([]mediaEngineHeaderExtension{}, m.headerExtensions...),
 	}
+	if len(m.headerExtensions) > 0 {
+		cloned.negotiatedHeaderExtensions = map[int]mediaEngineHeaderExtension{}
+	}
+	return cloned
 }
 
 func (m *MediaEngine) getCodecByPayload(payloadType PayloadType) (RTPCodecParameters, RTPCodecType, error) {


Test patch:
diff --git a/mediaengine_test.go b/mediaengine_test.go
index 6abb044a..5485490e 100644
--- a/mediaengine_test.go
+++ b/mediaengine_test.go
@@ -261,3 +261,28 @@ func TestMediaEngineDoubleRegister(t *testing.T) {
 
 	assert.Equal(t, len(m.audioCodecs), 1)
 }
+
+// The cloned MediaEngine instance should be able to update negotiated header extensions.
+func TestUpdateHeaderExtenstionToClonedMediaEngine(t *testing.T) {
+	src := MediaEngine{}
+
+	assert.NoError(t, src.RegisterCodec(
+		RTPCodecParameters{
+			RTPCodecCapability: RTPCodecCapability{MimeTypeOpus, 48000, 0, "", nil},
+			PayloadType:        111,
+		}, RTPCodecTypeAudio))
+
+	assert.NoError(t, src.RegisterHeaderExtension(RTPHeaderExtensionCapability{"test-extension"}, RTPCodecTypeAudio))
+
+	validate := func(m *MediaEngine) {
+		assert.NoError(t, m.updateHeaderExtension(2, "test-extension", RTPCodecTypeAudio))
+
+		id, audioNegotiated, videoNegotiated := m.getHeaderExtensionID(RTPHeaderExtensionCapability{URI: "test-extension"})
+		assert.Equal(t, 2, id)
+		assert.True(t, audioNegotiated)
+		assert.False(t, videoNegotiated)
+	}
+
+	validate(&src)
+	validate(src.copy())
+}