1337468 - RID RTP header extensions never gets send

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Bug 1337468: enabled RID RTP header extensions in simulcast test

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

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: Bug 1337468: Don't offer RID extension for audio streams

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

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: Bug 1337468: removed unused RID code and variables

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

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: Bug 1337468: pass RID values via RTP configuration

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

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

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 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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_ ...

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8835288 [details]
Bug 1337468: removed unused RID code and variables

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

Comment 9

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8835287 [details]
Bug 1337468: Don't offer RID extension for audio streams

https://reviewboard.mozilla.org/r/110992/#review112532

Comment 10

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8835286 [details]
Bug 1337468: enabled RID RTP header extensions in simulcast test

https://reviewboard.mozilla.org/r/110990/#review112534

Comment 11

[Nils Ohlmeier [:drno]]

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 12

[Nils Ohlmeier [:drno]]

Comment on attachment 8835289 [details]
Bug 1337468: pass RID values via RTP configuration

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

Comment 13

[Nils Ohlmeier [:drno]]

Comment on attachment 8835289 [details]
Bug 1337468: pass RID values via RTP configuration

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

Comment 14

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8835289 [details]
Bug 1337468: pass RID values via RTP configuration

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

Comment 15

[Pulsebot]

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

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/d67568f13010
https://hg.mozilla.org/mozilla-central/rev/99c3ec3f4456
https://hg.mozilla.org/mozilla-central/rev/441d27d95426
https://hg.mozilla.org/mozilla-central/rev/254d3dec5563