1478685 - Incorrect MID is sent in RTP header extension

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Jeremy Lainé]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180627012544

Steps to reproduce:

Establish an RTCPeerConnection between Firefox and another browser (either Firefox or Chrome with unified-plan SDP), with both an audio and a video track.


Actual results:

The same MID value (sdparta_0) is sent in the header of all RTP packets, regardless of whether they are for audio or video.


Expected results:

The MID announced in the SDP for the audio / video tracks should be respected, otherwise this breaks MID-based demultiplexing.

Comment 1

[Jeremy Lainé]

A consequence of this bug is that audio + video interop is broken with Chrome when using Unified Plan SDP.

Comment 2

[Philipp Hancke [:fippo]]

Attachment: mid.pcap

I can reproduce that behaviour. See for example packet #9 (audio, mid=audio) and #22 (keyframe, mid=audio)

Comment 3

[Byron Campen [:bwc]]

Interesting. Looking now.

Comment 4

[Byron Campen [:bwc]]

Attachment: Bug 1478685: Set the RTP MID to the mid of the transceiver, not the transport id string.

Review commit: https://reviewboard.mozilla.org/r/259716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259716/

Comment 5

[Michael Froman [:mjf]]

Comment on attachment 8995241 [details]
Bug 1478685: Set the RTP MID to the mid of the transceiver, not the transport id string.

https://reviewboard.mozilla.org/r/259716/#review266700

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.cpp:244
(Diff revision 1)
>                          " SetLocalSSRCs failed");
>      return NS_ERROR_FAILURE;
>    }
>  
>    mConduit->SetLocalCNAME(mJsepTransceiver->mSendTrack.GetCNAME().c_str());
> -  mConduit->SetLocalMID(mJsepTransceiver->mTransport->mTransportId);
> +  mConduit->SetLocalMID(mMid);

If |mMid| has been cleared in line 221, does this still work?

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

[Tracking Requested - why for this release]: See comment 1.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8995241 [details]
Bug 1478685: Set the RTP MID to the mid of the transceiver, not the transport id string.

https://reviewboard.mozilla.org/r/259716/#review266700

> If |mMid| has been cleared in line 221, does this still work?

It looks like it does something sensible; this function just sets a std::string in the codec config that is empty by default.

Comment 8

[Michael Froman [:mjf]]

Comment on attachment 8995241 [details]
Bug 1478685: Set the RTP MID to the mid of the transceiver, not the transport id string.

https://reviewboard.mozilla.org/r/259716/#review266716

Looks good to me.

Comment 9

[Byron Campen [:bwc]]

The try push looks good, does the binary work for you? The windows 64 debug build is here: https://queue.taskcluster.net/v1/task/Uk-VeA9YQaG_G1LBVqpRjg/runs/0/artifacts/public/build/install/sea/target.installer.exe

Comment 10

[Byron Campen [:bwc]]

Linux build should be here: https://queue.taskcluster.net/v1/task/FQqz1jgeSEOA3xQUjukhuA/runs/0/artifacts/public/build/target.tar.bz2

Comment 11

[Jeremy Lainé]

Yep it looks good to me, from the SDP:

m=audio 59564 UDP/TLS/RTP/SAVPF 109 9 0 8 101
a=mid:sdparta_0
a=ssrc:1209280607 cname:{6e544685-d92f-402b-9e3f-6fc12aefa40f}

m=video 46212 UDP/TLS/RTP/SAVPF 120 121 126 97
a=mid:sdparta_1
a=ssrc:1692656918 cname:{6e544685-d92f-402b-9e3f-6fc12aefa40f}

And what I observe on the wire:

ssrc 1209280607 mid sdparta_0
ssrc 1692656918 mid sdparta_1

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 62/63 to make sure we consider uplift to 62 if we land a fix in 63.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56590b88c041
Set the RTP MID to the mid of the transceiver, not the transport id string. r=mjf

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/56590b88c041

Comment 15

[Nils Ohlmeier [:drno]]

Since MID is not really used that much in the market we decided to simply let this ride the trains.

Comment 16

[Philipp Hancke [:fippo]]

i'm ok with this for FF63 but not uplifting this into ESR will negatively affect both the Chrome migration to unified plan as default as well as early adopters who choose to opt-in.

Comment 17

[Andreas Pehrson [:pehrsons]]

Nils/Byron, would you care to reconsider esr uplift in light of comment 16?

Comment 18

[Byron Campen [:bwc]]

I guess I could see uplifting to ESR 60, given that ESR 68 is a year away. Might be a good idea to wait until this change has made it to beta first, though.

Comment 19

[Nils Ohlmeier [:drno]]

I doubt Chrome's unified code will be in production before ESR 68 ships, but I guess it's better to be on the safe side.

Comment 20

[Nils Ohlmeier [:drno]]

[Tracking Requested - why for this release]: If we don't include this bug fix in ESR60 it will break WebRTC interop with Chrome in the near term future.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR60 approval when you get a chance.

Comment 22

[Byron Campen [:bwc]]

Comment on attachment 8995241 [details]
Bug 1478685: Set the RTP MID to the mid of the transceiver, not the transport id string.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 

   See comment 16 and comment 20

Fix Landed on Version:

   63

Risk to taking this patch (and alternatives if risky): 

   Very low.

String or UUID changes made by this patch: 

   None.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8995241 [details]
Bug 1478685: Set the RTP MID to the mid of the transceiver, not the transport id string.

Fixes a Chrome interop issue for WebRTC. Approved for ESR 60.3.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/098585dc86fc

Comment 25

[Adrian Florinescu [:aflorinescu] [PTO until Aug 26]]]

Andreas, Jeremy, would you be so kind to assist us with a more concrete STR to reproduce this issue in order to verify it?

Comment 26

[Andreas Pehrson [:pehrsons]]

Not really my area. Byron?

Comment 27

[Byron Campen [:bwc]]

This is kinda involved to verify, do we want QE to deal with it?

Comment 28

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #27)
> This is kinda involved to verify, do we want QE to deal with it?

Adrian the only way I can think of right now to verify this would be to make a video webrtc call, for example on appear.in or appr.tc. Record the network traffic with tcpdump or wireshark. And then compare bits of information from about:webrtc with all the packets in the network trace.

I tend to agree with Byron that this too much effort to be able to verify for non-expert in this area.

Comment 29

[Philipp Hancke [:fippo]]

Attachment: esr.pcapng

I grabbed ESR from https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr60&selectedJob=204863894 -- specifically https://queue.taskcluster.net/v1/task/eJkzsHvPS5mlU62wchsn3g/runs/0/artifacts/public/build/target.tar.bz2

The version said still 60.2 but the attached esr.pcapng shows the right mids, audio on audio packets and video on video packets.

Comment 30

[Adrian Florinescu [:aflorinescu] [PTO until Aug 26]]]

Thanks Phillipp for ESR verification. Marking ESR 60 as verified as per comment 29.
Phillipp, could you also assist verifying the fix for 63 Beta?

Comment 31

[Philipp Hancke [:fippo]]

Attachment: beta63.pcapng

grabbed 63.0b13 and generated the attached pcap. Contains different mid headerextensions for audio and video so that looks good to me too!

Comment 32

[Adrian Florinescu [:aflorinescu] [PTO until Aug 26]]]

Thanks Andreas for sorting the flags I messed up in comment 30.
Also, thanks Philipp for verifying Beta 63 as well. 

Based on comment 31, marking Beta63 as verified.