Closed Bug 1402495 Opened 3 years ago Closed 3 years ago

Add MID rtp header extension to rtp packets

Categories

(Core :: WebRTC, enhancement, P2)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: mjf, Assigned: mjf)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

No description provided.
Rank: 19
Comment on attachment 8911897 [details]
Bug 1402495 - Use constants for the RtpExtension URIs.

https://reviewboard.mozilla.org/r/183272/#review189574
Attachment #8911897 - Flags: review?(drno) → review+
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
Attachment #8911898 - Flags: review?(drno) → review+
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
Attachment #8911899 - Flags: review?(drno) → review+
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.
See Also: → 1405494
Blocks: 1405495
See Also: → 1105005
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
You need to log in before you can comment on or make changes to this bug.