RTCP SR packets were always discarded

RESOLVED FIXED in Firefox 33, Firefox OS v2.0

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Oscar Patiño González, Assigned: Oscar Patiño González)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox32 wontfix, firefox33 fixed, firefox34 fixed, firefox35 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created 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

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
(Assignee)

Comment 1

4 years ago
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.
Attachment #8486422 - Flags: feedback?(rjesup)

Updated

4 years ago
Attachment #8486422 - Attachment is patch: true
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())
Attachment #8486422 - Flags: feedback?(rjesup) → feedback+

Comment 3

4 years ago
Can we add an automated test for this?
(Assignee)

Updated

4 years ago
Assignee: nobody → opatinobugzilla
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
Created attachment 8487186 [details] [diff] [review]
rtcp_sr_not_received.patch
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Attachment #8486422 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8487186 - Flags: review?(rjesup)
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)).
Attachment #8487186 - Flags: review?(rjesup) → review+
(Assignee)

Updated

4 years ago
Attachment #8487186 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
Created attachment 8487742 [details] [diff] [review]
rtcp_sr_not_received.patch

includes changes in both audioConduit.cpp and videoConduit.cpp
Attachment #8487742 - Flags: review?(rjesup)
(Assignee)

Comment 8

4 years ago
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 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!
Attachment #8487742 - Flags: review?(rjesup) → review+
Blocks: 1062883
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.
Whiteboard: [webrtc-uplift]
Target Milestone: --- → mozilla35
[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."
blocking-b2g: --- → 2.0?
status-b2g-v2.0: --- → affected
status-firefox32: --- → wontfix
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → affected
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
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
Attachment #8487742 - Flags: approval-mozilla-beta?
Attachment #8487742 - Flags: approval-mozilla-b2g32?
Attachment #8487742 - Flags: approval-mozilla-aurora?
(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.
Blocks: 1036490

Updated

4 years ago
blocking-b2g: 2.0? → 2.0+
(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?
https://hg.mozilla.org/mozilla-central/rev/b4102ee7bdf2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
status-firefox35: affected → fixed
(Assignee)

Comment 16

4 years ago
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

Updated

4 years ago
Attachment #8487742 - Flags: approval-mozilla-b2g32?
Attachment #8487742 - Flags: approval-mozilla-b2g32+
Attachment #8487742 - Flags: approval-mozilla-aurora?
Attachment #8487742 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/a68bd7dca981
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/0c481b4853fa
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: --- → fixed
status-b2g-v2.2: --- → fixed
status-firefox34: affected → fixed
Attachment #8487742 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

4 years ago
Whiteboard: [webrtc-uplift]
You need to log in before you can comment on or make changes to this bug.