Comment on attachment 8911897 [details] Bug 1402495 - Use constants for the RtpExtension URIs. https://reviewboard.mozilla.org/r/183272/#review189574

Comment on attachment 8911898 [details] Bug 1402495 - changes to support MID in video packets. https://reviewboard.mozilla.org/r/183274/#review189580 One thing came to my mind when reviewing this: I think we also need to put the MID into RTCP, or? ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:181 (Diff revision 1) > // Returns SSRC. > virtual uint32_t SSRC() const = 0; > > // Set RID value for the RID header extension or RTCP SDES > virtual int32_t SetRID(const char *rid) = 0; > + virtual int32_t SetMID(const char *mid) = 0; I guess the changes in webrtc/modules/... would also need to go into a patch for upstream

Comment on attachment 8911899 [details] Bug 1402495 - changes to support MID in audio packets. https://reviewboard.mozilla.org/r/183276/#review189582 ::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:230 (Diff revision 1) > + if (mId && !mId->empty()) { > + packet->SetExtension<MId>(*mId); > + } > + Wrong indentation. ::: media/webrtc/trunk/webrtc/voice_engine/include/voe_rtp_rtcp.h:113 (Diff revision 1) > // reference counter. Returns the new reference count. This value should > // be zero for all sub-API:s before the VoiceEngine object can be safely > // deleted. > virtual int Release() = 0; > > + virtual int SetLocalMID(int channel, const char* mid) = 0; A comment here like for the other functions might be helpful. ::: media/webrtc/trunk/webrtc/voice_engine/include/voe_rtp_rtcp.h:128 (Diff revision 1) > // Sets the status of rtp-audio-level-indication on a specific |channel|. > virtual int SetSendAudioLevelIndicationStatus(int channel, > bool enable, > unsigned char id = 1) = 0; > > + virtual int SetSendMIDStatus(int channel, bool enable, unsigned char id = 1) = 0; And a comment here as well.

Comment on attachment 8911899 [details] Bug 1402495 - changes to support MID in audio packets. https://reviewboard.mozilla.org/r/183276/#review189584

Comment on attachment 8911898 [details] Bug 1402495 - changes to support MID in video packets. https://reviewboard.mozilla.org/r/183274/#review189580 > I guess the changes in webrtc/modules/... would also need to go into a patch for upstream Would it be better to put all of those changes into a separate commit here to ease that process?

Comment on attachment 8911898 [details] Bug 1402495 - changes to support MID in video packets. https://reviewboard.mozilla.org/r/183274/#review189580 I will open a separate bug for that.

Pushed by mfroman@nostrum.com: https://hg.mozilla.org/integration/autoland/rev/3b55ea977fb4 Use constants for the RtpExtension URIs. r=drno https://hg.mozilla.org/integration/autoland/rev/0049df4c862d changes to support MID in video packets. r=drno https://hg.mozilla.org/integration/autoland/rev/a1a75ca5a4d8 changes to support MID in audio packets. r=drno

https://hg.mozilla.org/mozilla-central/rev/3b55ea977fb4 https://hg.mozilla.org/mozilla-central/rev/0049df4c862d https://hg.mozilla.org/mozilla-central/rev/a1a75ca5a4d8