859971 - RTCP Should be sent/received regardless of MediaStream direction attribute

Status: NEW

(Core :: WebRTC: Networking, defect, P4)

Description

[Suhas]

RFC4566 requires control data should be processed as normal regardless of media direction attribute.

Comment 1

[Suhas]

While working on the patch to Bug 783295, i realized that RTCP is not exchanged for inactive, sendonly/recvonly streams appropriately.

The patch to Bug 783295 should set the conduits to enable this to happen. We can use this bug to verify RTCP exchange with right test-cases. Hence making this bug dependent on 783295 for now.

Comment 2

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

We should fix this separate from bug 783295, and also add it to the unit tests (since I don't think the mochitests can see this deep)

Comment 3

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

Removing dependency

Comment 4

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

RTCP must work for sendonly/recvonly

Comment 5

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

Ethan - can you coordinate with Suhas to get a fix for this ASAP so we can land it and uplift it to Aurora?

Comment 6

[Ethan Hugg [:ehugg]]

Suhas is working on a patch for this today, hope to have it ready tomorrow.

Comment 7

[Jason Smith [:jsmith]]

Targeted to ship for Fx22, tracking to get on rel mgmt radar.

Comment 8

[Suhas]

Attachment: Part2 - Add Apis to report RTCP packet count

Comment 9

[Suhas]

Attachment: Part1 - Signaling support for enaling RTCP flows

Comment 10

[Eric Rescorla (:ekr)]

Comment on attachment 737229 [details]
Part1 - Signaling support for enaling RTCP flows

Review of attachment 737229 [details]:
-----------------------------------------------------------------

This patch doesn't look correct to me.

If I am reading it correctly, the only time when we
instantiate a pipeline with isRTCP is when we ware
in a=inactive mode. Note that because pipelines
are unidirectional, sendrecv is just a send + a
recv, so I dont see why you need special
support for sendonly or recvonly.

Assuming that the problem is solely inactive then
this doesn't seem like the right solution, since
it's going to be a mess if/when the stream goes
active due to renegotiation.

In the outgoing direction, what you want is to
have a single flag indicating that you should
be discarding media from the stream. The incoming
direction is more complicated since I believe you
can actually start receiving RTP before you get
the offer telling you that the stream is active,
and you don't want to reject it. I suspect that
the answer here is that you actually don't
do anything special in that case, but we should
ask abr.

Also, a style note: please try not to use boolean
arguments to functions to control this kind of
thing. It makes it harder to read the code.

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +1082,5 @@
>  
>  #define LSM_TMP_VAD_LEN 64
>  
>  static void
> +lsm_tx_start (lsm_lcb_t *lcb, const char *fname, fsmdef_media_t *media, boolean isRTCP)

Please do not use booleans here. The relevant information should be in the media structure.

@@ +3793,5 @@
>          if (rx_refresh ||
>              (media->is_multicast &&
>               media->direction_set &&
>               media->direction == SDP_DIRECTION_RECVONLY)) {
> +            lsm_rx_start(lcb, caller_fname, media, rx_rtcp);

This can never happen, right? Because we're not going to set media->is_multicast

@@ +3798,3 @@
>          }
>          if (tx_refresh) {
> +            lsm_tx_start(lcb, caller_fname, media, tx_rtcp);

As far as I can tell, this is the only time that we call lsm_tx_start() with isRTCP = true

Comment 11

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

Comment on attachment 737228 [details]
Part2 - Add Apis to report RTCP packet count

Review of attachment 737228 [details]:
-----------------------------------------------------------------

r- for the two main issues in PeerConnectionMedia.cpp; the rest are nits

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +246,5 @@
>    // Called when OnLocal/RemoteDescriptionSuccess/Error
>    // is called to start the list over.
>    void ClearSdpParseErrorMessages();
>  
> +  // Utlility delegaes for retrieving RTCP Tx/Rx counts

Utility delegates

Though maybe "Functions" would be be better.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +401,5 @@
> +int
> +PeerConnectionMedia::GetRTCPPacketsSent(int aStreamId, bool aIsVideo)
> +{
> +  RemoteSourceStreamInfo* info = GetRemoteStream(aStreamId);
> +  if(!info) {

space after "if" - repeat throughout patch

@@ +426,5 @@
> +
> +  if(!aIsVideo) {
> +    for(uint32_t i=0; i < info->AudioTrackCount(); ++i) {
> +      mozilla::RefPtr<mozilla::MediaPipeline> pipeline =
> +          info->RetreivePipeline(info->GetAudioTrackAt(i));

This reads as 1/2 a final implementation for multiple tracks of a type.  It supports multiple tracks, but returns only the first one.  Either:

a) add a track number or other selector to the function API, so this is (at least in theory) a multiple-tracks-supporting implementation, or
b) take track 0 and assert (or NS_WARNING, etc) if there is more than 1 track of a type, so that we know it needs rewrite as soon as we add multiple track support.

I'm more tempted by option b, since that ensures attention to the calling sites as well (since the API would change), but I'm ok with option a.

@@ +430,5 @@
> +          info->RetreivePipeline(info->GetAudioTrackAt(i));
> +      if(!pipeline) {
> +        continue;
> +      }
> +      return pipeline->rtcp_packets_received();

if (pipeline) {
  return .....;
}
much simpler to read

@@ +500,5 @@
>  
> +// XXX: Revisit this function once we support more tracks of same type
> +//      and when more streams of same type are supported
> +mozilla::RefPtr<mozilla::MediaPipeline>
> +RemoteSourceStreamInfo::RetreivePipeline(bool aIsVideo)  {

Retrieve

@@ +506,5 @@
> +    if(it->second != aIsVideo) {
> +      return mPipelines[it->first];
> +    } else {
> +      return mPipelines[it->first];
> +    }

This is just totally wrong.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +182,5 @@
>      return mMediaStream;
>    }
>    void StorePipeline(int aTrack, mozilla::RefPtr<mozilla::MediaPipeline> aPipeline);
>  
> +  mozilla::RefPtr<mozilla::MediaPipeline> RetreivePipeline(int aTrack) {

Retrieve

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +2048,5 @@
> +
> +  PR_Sleep(kDefaultTimeout * 2); // Wait for some data to get written
> +
> +  //verify rtcp packets count for audio
> +  ASSERT_GE(a1_.GetRTCPTxCount(0,false), 1);

space after comma

@@ +2050,5 @@
> +
> +  //verify rtcp packets count for audio
> +  ASSERT_GE(a1_.GetRTCPTxCount(0,false), 1);
> +  ASSERT_GE(a1_.GetRTCPRxCount(0,false), 1);
> +  ASSERT_GE(a2_.GetRTCPTxCount(0,false), 1);

How many RTCP "should" we get and send in this time period?  Please add a comment detailing that

@@ +2073,5 @@
> +
> +  PR_Sleep(kDefaultTimeout * 2); // Wait for some data to get written
> +
> +  //verify rtcp packets count for audio
> +  ASSERT_GE(a1_.GetRTCPTxCount(0,true), 5);

space after comma

@@ +2075,5 @@
> +
> +  //verify rtcp packets count for audio
> +  ASSERT_GE(a1_.GetRTCPTxCount(0,true), 5);
> +  ASSERT_GE(a1_.GetRTCPRxCount(0,true), 5);
> +  ASSERT_GE(a2_.GetRTCPTxCount(0,true), 5);

How many RTCP "should" we get and send in this time period? Please add a comment detailing that

Comment 12

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

Comment on attachment 737229 [details]
Part1 - Signaling support for enaling RTCP flows

Review of attachment 737229 [details]:
-----------------------------------------------------------------

Cancelling review for now, since it appears significant work is needed.

Comment 13

[Suhas]

Comment on attachment 737228 [details]
Part2 - Add Apis to report RTCP packet count

Review of attachment 737228 [details]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +426,5 @@
> +
> +  if(!aIsVideo) {
> +    for(uint32_t i=0; i < info->AudioTrackCount(); ++i) {
> +      mozilla::RefPtr<mozilla::MediaPipeline> pipeline =
> +          info->RetreivePipeline(info->GetAudioTrackAt(i));

For a AV session, local track ids take value 0 and 1 , remotesourcestream takes tracks 1 and 2 for audio and video respectively.  So we need to extract the correct trackId to index into Pipelines map.

I will leave the API as it is and will Warn if there are more than one track with same value

@@ +430,5 @@
> +          info->RetreivePipeline(info->GetAudioTrackAt(i));
> +      if(!pipeline) {
> +        continue;
> +      }
> +      return pipeline->rtcp_packets_received();

Will Do

@@ +500,5 @@
>  
> +// XXX: Revisit this function once we support more tracks of same type
> +//      and when more streams of same type are supported
> +mozilla::RefPtr<mozilla::MediaPipeline>
> +RemoteSourceStreamInfo::RetreivePipeline(bool aIsVideo)  {

Ah !! Will fix

@@ +506,5 @@
> +    if(it->second != aIsVideo) {
> +      return mPipelines[it->first];
> +    } else {
> +      return mPipelines[it->first];
> +    }

Here, I am indexing based on mTypes<TrackId, Boolean> map. This map stores True for Video Track and False for Audio Track. Hence based on the aIsVideo, i am extracting the trackId to index into mPipelines.

Comment 14

[Suhas]

Attachment: Patch to enable and verify RTCP send/recv

Comment 15

[Eric Rescorla (:ekr)]

Comment on attachment 737685 [details] [diff] [review]
Patch to enable and verify RTCP send/recv

Review of attachment 737685 [details] [diff] [review]:
-----------------------------------------------------------------

assigning to myself for review. there's some stuff I want to check on.

Comment 16

[Suhas]

Attachment: RTCP Tx/Rx Logs

Comment 17

[Suhas]

Attachment: RTCP Tx/Rx Logs

Comment 18

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

Comment on attachment 737685 [details] [diff] [review]
Patch to enable and verify RTCP send/recv

Review of attachment 737685 [details] [diff] [review]:
-----------------------------------------------------------------

Without this patch, normal timed RTCP SR's aren't getting sent even in 2-way video cases (other RTCP's do get sent (REMB, NACK, etc), and they include RR/SR data when they are).  In one-way video, no RTCP would be sent without this patch.  It appears that the transmit pipeline, even though it's wasn't intended for sending RTCP (per the comments) allows it to be done.  And I verified that with this patch, the video RTCPs are all being sent and appear to all be received and processed by the GIPS code (using webrtc_trace:65535).

The videoconduit code should be refactored to merge the sending and receiving engines like Audio, but that's a different, larger bug that I'm working on.  We need this patch or something like it for Aurora Uplift.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +664,5 @@
>      if (mOtherDirection)
>      {
>        return mOtherDirection->SendRTCPPacket(channel, data, len);
>      }
> +  } 

trailing WS

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +102,5 @@
> +  } else {
> +    // extract video pipeline.
> +    return mPipelines[GetVideoTrackAt(0)];
> +  }
> +  return nullptr;

return nullptr is unreachable.

Proper style would be:

if (foo) {
   return ..
}
return ...

@@ +451,5 @@
> +  mozilla::RefPtr<mozilla::MediaPipeline> pipeline; 
> +  nsRefPtr<LocalSourceStreamInfo>  localInfo = mLocalSourceStreams[aStreamId];
> +  if (localInfo->HasPipelineAttached()) {
> +    pipeline = localInfo->RetrievePipeline(aIsVideo);
> +    return pipeline->rtcp_packets_sent();

Get rid of HasPipelineAttached() and use:
pipeline = ....
if (pipeline) {
   return pipeline->xxxx();
}

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +183,5 @@
>    }
> +
> +  bool HasPipelineAttached() const {
> +    return mHasPipeline;
> +  }

remove

@@ +189,3 @@
>    void StorePipeline(int aTrack, mozilla::RefPtr<mozilla::MediaPipeline> aPipeline);
>  
> +  mozilla::RefPtr<mozilla::MediaPipeline> RetrievePipeline(bool aIsVideo); 

trailing WS

@@ +211,5 @@
>    nsRefPtr<DOMMediaStream> mMediaStream;
>    nsTArray<mozilla::TrackID> mAudioTracks;
>    nsTArray<mozilla::TrackID> mVideoTracks;
>    PeerConnectionMedia *mParent;
> +  // for unidirectional stream,pipeline will not be attached 

WS, repeat

Comment 19

[Eric Rescorla (:ekr)]

Comment on attachment 737685 [details] [diff] [review]
Patch to enable and verify RTCP send/recv

Review of attachment 737685 [details] [diff] [review]:
-----------------------------------------------------------------

I have a number of concerns about this patch.

1. As far as I can tell, it only allows *sending* of RTCP, not reception.
Since we use distinct conduits for sending and receiving, this means
that the RTCP goes to the wrong conduit and hence channel.

2. The tests should be at the pipeline level.

3. I would like to see some tests that actually measure the user-visible
features that are impacted by this defect. For instance, demonstrate that
A/V sync doesn't work without this fix but that it does with it.

At a higher level, I think we need to work out the proper relationship
between channels, conduits, media pipelines, and potentially m-lines
rather than just doing point fixes.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +425,1 @@
>    if (direction_ == RECEIVE) {

Unless I am missing something, we still ignore any incoming RTCP
when we are in receive mode, which means that SRs are not
sent to the right channel/engine.

Comment 20

[Eric Rescorla (:ekr)]

Comment on attachment 737685 [details] [diff] [review]
Patch to enable and verify RTCP send/recv

Review of attachment 737685 [details] [diff] [review]:
-----------------------------------------------------------------

Gah. That should be an r-.

Comment 21

[Suhas]

Comment on attachment 737685 [details] [diff] [review]
Patch to enable and verify RTCP send/recv

Review of attachment 737685 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +425,1 @@
>    if (direction_ == RECEIVE) {

That's correct. We are not feeding received RTCP packet as it is today. Since there is a separate bug raised to fix this once we have SSRC and related stuff implemented.

Comment 22

[Eric Rescorla (:ekr)]

I would rather see both issues fixed at the same time

Comment 23

[Suhas]

I think we have following ways we can deal with this bug

1. Go with the point fix to enable Send of RTCP for both sendrecv/sendonly directions for Ffox 22

2. Make this bug non-blocking and take a step back to look at refactoring conduit  and fixing Rx RTCP with SSRC and any related things implemented. Also come up with a way for verifying user visible impacts.

3. If there is a interim between 1 and 2 that would still be good for Aurora, do that

Any thoughts on how should we proceed.

Comment 24

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

Not blocking, but if we get get a safe fix for uplift we'd take it.  The trick is to get it to deliver RTCP without double-delivering it in the bidirectional case - RTCP incoming gets sent to BOTH pipelines, and the "Receive" pipeline blocks it.

Comment 25

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

Removing from tracking as we've decided it's not blocking release

Comment 26

[Adam Roach [:abr]]

(In reply to Eric Rescorla (:ekr) from comment #10)

> The incoming
> direction is more complicated since I believe you
> can actually start receiving RTP before you get
> the offer telling you that the stream is active,
> and you don't want to reject it. I suspect that
> the answer here is that you actually don't
> do anything special in that case, but we should
> ask abr.

I agree. In fact, it's not that you *can* start getting media before the signaling; it's that you *usually* *will* start getting media before the signaling, since the signaling is normally going to traverse the web server while the media frequently goes directly between peers (although there's nothing preventing WebRTC applications from using a data channel for session manipulation once the session is setup).

For a sendonly (or inactive) stream, we really want to render incoming media to avoid clipping off the first part of the media. In the general (non-WebRTC) case, you might have to worry about third-party attacks that involve spoofing RTP; but for WebRTC, anything that we can decrypt can be trusted to come from the other side, so playout should be safe.

Comment 28

[todorst]

I think that this bug should have a higher priority, because it affects not only the RTCP SR/RR, but also the RTCP Feedback packets, i.e. FIR, PLI. It appears that PeerConnection with sendonly video does not react on FIR packets sent to it.

My test scenario is tree PeerConnections (A,B,C) connected via Video MCU (switching unit). A and B are sendonly and C is recvonly. The MCU is using FIRs to switch the RTP stream send to C between A and B. Since A and B are sendonly they don't react on the FIR packets and does not generate a key-frame.
The same scenario works if A and B have sendrecv video. I guess the received RTCP packets not processed in case of sendonly video, or the RTCP receiver is inactive.

Even in p2p scenario a sendonly video won't react on RTCP PLI packets, which could lead to bad/frozen video on the remote peer.

Comment 29

[Martin Thomson [:mt:]]

:bwc, is this something that the recent changes to our signaling and pipeline setup could have fixed?  Is this something we could test with a mochitest?

Comment 30

[Byron Campen [:bwc]]

This was at least partially fixed in the bundle work. Someone would need to verify; might be possible to write a test in signaling_unittests.

Comment 31

[Nils Ohlmeier [:drno]]

Attachment: bug_859971_test.patch

I think the last patch in here has bit rotten so much, it is very hard to repair.
But as it turns out our singaling unit test verifies RTCP sending and receiving in direction already. This small patch adds verification in the opposite direction, and suddenly a big chunk of the tests start to fail.

Comment 32

[Nils Ohlmeier [:drno]]

Comment on attachment 8594123 [details] [diff] [review]
bug_859971_test.patch

Never mind that was just me not understanding how the media pipelines work...

Comment 33

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.