1402495 - Add MID rtp header extension to rtp packets

Status: RESOLVED FIXED

(Core :: WebRTC, enhancement, P2)

Comment 1

[Michael Froman [:mjf]]

Attachment: Bug 1402495 - Use constants for the RtpExtension URIs.

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

Comment 2

[Michael Froman [:mjf]]

Attachment: Bug 1402495 - changes to support MID in video packets.

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

Comment 3

[Michael Froman [:mjf]]

Attachment: Bug 1402495 - changes to support MID in audio packets.

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

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8911897 [details]
Bug 1402495 - Use constants for the RtpExtension URIs.

https://reviewboard.mozilla.org/r/183272/#review189574

Comment 5

[Nils Ohlmeier [:drno]]

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 6

[Nils Ohlmeier [:drno]]

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 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8911899 [details]
Bug 1402495 - changes to support MID in audio packets.

https://reviewboard.mozilla.org/r/183276/#review189584

Comment 8

[Michael Froman [:mjf]]

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 9

[Michael Froman [:mjf]]

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.

Comment 10

[Michael Froman [:mjf]]

Comment on attachment 8911897 [details]
Bug 1402495 - Use constants for the RtpExtension URIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183272/diff/1-2/

Comment 11

[Michael Froman [:mjf]]

Comment on attachment 8911898 [details]
Bug 1402495 - changes to support MID in video packets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183274/diff/1-2/

Comment 12

[Michael Froman [:mjf]]

Comment on attachment 8911899 [details]
Bug 1402495 - changes to support MID in audio packets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183276/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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