Closed Bug 1337468 Opened 3 years ago Closed 3 years ago

RID RTP header extensions never gets send

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed
Blocking Flags:

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(4 files)

No description provided.
backlog: --- → webrtc/webaudio+
Rank: 15
Summary: RTP header extensions never get send even when negotiated → RID RTP header extensions never gets send
Attachment #8835286 - Flags: review?(na-g)
Attachment #8835287 - Flags: review?(na-g)
Attachment #8835288 - Flags: review?(na-g)
Attachment #8835289 - Flags: review?(na-g)
Comment on attachment 8835289 [details]
Bug 1337468: pass RID values via RTP configuration

https://reviewboard.mozilla.org/r/110996/#review112292

::: media/webrtc/trunk/webrtc/video/vie_channel.h:116
(Diff revision 1)
> -  int SetSendRtpStreamId(bool enable, int id); // RtpStreamId (RID)
> +  int SetSendRtpStreamId(bool enable, int id, std::vector<std::string> rids); // RtpStreamId (RID)
> -        int SetReceiveRtpStreamId(bool enable, int id); // RtpStreamId (RID)
> +  int SetReceiveRtpStreamId(bool enable, int id); // RtpStreamId (RID)

Probably remove the comments (redundant)

::: media/webrtc/trunk/webrtc/video_send_stream.h:134
(Diff revision 1)
>        } rtx;
>  
>        // RTCP CNAME, see RFC 3550.
>        std::string c_name;
> +
> +      std::vector<std::string> rids; //MOZ addition for RtpSenderId (RID)

get rid of the comment
Comment on attachment 8835289 [details]
Bug 1337468: pass RID values via RTP configuration

https://reviewboard.mozilla.org/r/110996/#review112528

Looks good. r+ with one question.

::: media/webrtc/trunk/webrtc/video/vie_channel.cc:672
(Diff revision 1)
> +  // Enable the extension
> -    // NOTE: simulcast streams must be set via the SetSendCodec() API
> +  // NOTE: simulcast streams must be set via the SetSendCodec() API
> -  } else {
> -    // Disable the extension.
> -    for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
> +  for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
> -    	rtp_rtcp->DeregisterSendRtpHeaderExtension(
> +    error |= rtp_rtcp->RegisterSendRtpHeaderExtension(

Is error a mask? If not, maybe error = error || rtp_ ...
Attachment #8835289 - Flags: review?(na-g) → review+
Comment on attachment 8835288 [details]
Bug 1337468: removed unused RID code and variables

https://reviewboard.mozilla.org/r/110994/#review112526

::: media/webrtc/trunk/webrtc/video/vie_channel.cc:673
(Diff revision 1)
>      // NOTE: simulcast streams must be set via the SetSendCodec() API
>    } else {
>      // Disable the extension.
> -    rid_extension_id_ = kInvalidRtpExtensionId;
>      for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
>      	rtp_rtcp->DeregisterSendRtpHeaderExtension(

If it is not much trouble, zap this tab.
Comment on attachment 8835288 [details]
Bug 1337468: removed unused RID code and variables

https://reviewboard.mozilla.org/r/110994/#review112530
Attachment #8835288 - Flags: review?(na-g) → review+
Comment on attachment 8835287 [details]
Bug 1337468: Don't offer RID extension for audio streams

https://reviewboard.mozilla.org/r/110992/#review112532
Attachment #8835287 - Flags: review?(na-g) → review+
Comment on attachment 8835286 [details]
Bug 1337468: enabled RID RTP header extensions in simulcast test

https://reviewboard.mozilla.org/r/110990/#review112534
Attachment #8835286 - Flags: review?(na-g) → review+
Comment on attachment 8835289 [details]
Bug 1337468: pass RID values via RTP configuration

https://reviewboard.mozilla.org/r/110996/#review112528

> Is error a mask? If not, maybe error = error || rtp_ ...

No it's not a error mask.
But what I don't like about the || alternative is that it results in no calls to RegisterSendRtpHeaderExtension() after encountering the first error. I guess one can argue if it makes to continue after encountering the first error. All the other functions from webrtc.org use the |=, that's why I copied it.
Comment on attachment 8835289 [details]
Bug 1337468: pass RID values via RTP configuration

https://reviewboard.mozilla.org/r/110996/#review113350
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/d67568f13010
enabled RID RTP header extensions in simulcast test r=ng
https://hg.mozilla.org/integration/autoland/rev/99c3ec3f4456
Don't offer RID extension for audio streams r=ng
https://hg.mozilla.org/integration/autoland/rev/441d27d95426
removed unused RID code and variables r=ng
https://hg.mozilla.org/integration/autoland/rev/254d3dec5563
pass RID values via RTP configuration r=ng
Depends on: 1365090
You need to log in before you can comment on or make changes to this bug.