1064882 - RTCP SR packets were always discarded

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Oscar Patiño González]

Attachment: rtcp_sr patch for av sync in webrtc. This change was made in v2.0 but applies to 2.1 as well

RTCP SR packets were not progressing through the code and ntp values and rtp timestamp couldn't be extracted from these packets. Their values were always zero. With this patch there are sometimes they are zero, but others they are the SR synchronization values

Comment 1

[Oscar Patiño González]

Comment on attachment 8486422 [details] [diff] [review]
rtcp_sr patch for av sync in webrtc. This change was made in v2.0 but applies to 2.1 as well

Randell please, I'm not quite sure if it is necessary to add mEngineReceiving into that 'if' clause, because I removed completely rtcp_receiving in vie_receiver rtcp_packet. I removed it because if we are calling insertRTCPPacket, after some calls to successive RecieveRTCP() functions, why should we discard the rtcp at the end?

After removing that line on firefox os (I only work with mobiles) synchronization is much better. On logs I could see that sometimes NTP frac and sec and RTPtimestamp are correctly set, but not always. Before I always got 0s, and now I receive 0s but sometimes some values that can be used to synchronize.
AV synchronization is quite remarkable in loop application. Thanks.

Comment 2

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

Comment on attachment 8486422 [details] [diff] [review]
rtcp_sr patch for av sync in webrtc. This change was made in v2.0 but applies to 2.1 as well

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

f+ in that this is on the overall right track

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +547,5 @@
>      return kMediaConduitCodecInUse;
>    }
>  
>    //transmitting already ?
> +  if(mEngineTransmitting || mEngineReceiving)

instead, please see my other suggestion.

Also please try removing "if (mTransmitting)" from ReceivedRTCPPacket.

::: media/webrtc/trunk/webrtc/video_engine/vie_receiver.cc
@@ -291,5 @@
>    {
>      CriticalSectionScoped cs(receive_cs_.get());
> -    if (!receiving_rtcp_) {
> -      return -1;
> -    }

Instead, add "vie_receiver_.StartRTCPReceive(); // For SR RTCP packets" to ViEChannel::StartReceive() in vie_channel.cc (and StopRTCPReceive() in StopReceive())

Comment 3

[Eric Rescorla (:ekr)]

Can we add an automated test for this?

Comment 4

[Oscar Patiño González]

In yesterday's patch I misread my notes and applied a change in an incorrect 'if' statement. The correct 'if' statement is in function WebrtcVideoConduit::receivedRTCPPacket(). Corrected in this version. This last patch is my proposed one.

Comment 5

[Oscar Patiño González]

Attachment: rtcp_sr_not_received.patch

Comment 6

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

Comment on attachment 8487186 [details] [diff] [review]
rtcp_sr_not_received.patch

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

r+ if you apply the same change to AudioConduit.cpp.  Thanks!

Can you also detail the before/after difference you see on a phone (and in what application (Loop I presume)).

Comment 7

[Oscar Patiño González]

Attachment: rtcp_sr_not_received.patch

includes changes in both audioConduit.cpp and videoConduit.cpp

Comment 8

[Oscar Patiño González]

Thanks Randell. I have applied changes in audioConduit as well. I'm using Loop and previously av sync was awful, delayed 2 seconds at the start of videoconferencing and growing after that.
Now sync is perfect in my tests. I couldn't test for long time connections due to some instabilities and Loop sometimes crashes. I think that the problem right is now solved.
Thanks.

Comment 9

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

Comment on attachment 8487742 [details] [diff] [review]
rtcp_sr_not_received.patch

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

I'll land this.  Thanks for the writeup!

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4102ee7bdf2

Given the effect on B2G Loop operation, we'll want to uplift this.  Very simple change.

Comment 11

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

[Blocking Requested - why for this release]:
This is primarily for the one-way connections that TokBox/Loop uses; it has little impact on a=sendrecv streams that other services tend to use.  Per Oscar:
"I'm using Loop and previously av sync was awful, delayed 2 seconds at the start of videoconferencing and growing after that. Now sync is perfect in my tests."

Comment 12

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

Comment on attachment 8487742 [details] [diff] [review]
rtcp_sr_not_received.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A (upstream webrtc.org issue, plus how we handle one-way streams with the "split" send and receive conduits)

User impact if declined: degraded sync on one-way connections like TokBox uses, especially on mobile/B2G Loop (extremely degraded there at times: see oscar's description above)

Testing completed: requires manual testing in particular with Loop.  Normal Loop app (and desktop loop) user testing will test this well.  Tef and QC (and our QA) are doing this testing daily.  We'll verify this on m-c/nightly before landing any uplifts.

Risk to taking this patch (and alternatives if risky): very low risk; just tells it not to throw SR reports away.  No real new code, just uses existing mechanisms that normally are used in send/recv channels.

String or UUID changes made by this patch: none

Comment 13

[Maria Angeles Oteo (:oteo)]

(In reply to Randell Jesup [:jesup] from comment #11)
> [Blocking Requested - why for this release]:
> This is primarily for the one-way connections that TokBox/Loop uses; it has
> little impact on a=sendrecv streams that other services tend to use.  Per
> Oscar:
> "I'm using Loop and previously av sync was awful, delayed 2 seconds at the
> start of videoconferencing and growing after that. Now sync is perfect in my
> tests."

Thanks Randell!

As you say we detected 2 sec delays between audio and video without this patch when more than 100ms is not acceptable for video communications, that's the reason we need to land the fix in 2.0.

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Maria Angeles Oteo (:oteo) from comment #13)
> (In reply to Randell Jesup [:jesup] from comment #11)
> > [Blocking Requested - why for this release]:
> > This is primarily for the one-way connections that TokBox/Loop uses; it has
> > little impact on a=sendrecv streams that other services tend to use.  Per
> > Oscar:
> > "I'm using Loop and previously av sync was awful, delayed 2 seconds at the
> > start of videoconferencing and growing after that. Now sync is perfect in my
> > tests."
> 
> Thanks Randell!
> 
> As you say we detected 2 sec delays between audio and video without this
> patch when more than 100ms is not acceptable for video communications,
> that's the reason we need to land the fix in 2.0.

Maria,

Can you please help verify this on nightly before we uplift to branches?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b4102ee7bdf2

Comment 16

[Oscar Patiño González]

I have tried to test this patch in Master branch, but finally I couldn't because although I receive audio from the devices, I don't receive (or decode) any video. i haven't worked on it yet, so I don't know if video is not being sent, or at reception is unable to decode, or both.

This patch fixes lip-syncing problem anyway for 2.0, and I think it will in Master, but maybe it is better to open a bug to investigate the no-video issue in Master branch.

I have filed the following bug for the no-video issue in Master:
https://bugzilla.mozilla.org/show_bug.cgi?id=1067442

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a68bd7dca981
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/0c481b4853fa

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e99eaafdbda1

Comment 19

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/0c481b4853fa