Closed Bug 1358224 Opened 3 years ago Closed 3 years ago

simulcast mochitests (offer and answer) need work to filter by rid to avoid issues with switching ssrcs

Categories

(Core :: WebRTC, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: mjf, Assigned: mjf)

References

Details

Attachments

(3 files)

test_peerConnection_simulcastOffer.html and test_peerConnection_simulcastAnswer.html both suffer from switching ssrcs and timing out at step PC_LOCAL_WAIT_FOR_MEDIA_FLOW.  We believe this can be dealt with by filtering by RID instead of ssrc since we can set up the RID filter earlier.  There is also an additional issue with the answer tests because the extension map isn't setup properly due to the way the RID info is added to the offer.  This causes the parser to drop the RID extension making it difficult to filter on RID.
To re-iterate the options how to fix this:

#1: check in SetLocalDescription if the RID header extension got added to the SDP offer and add the RID extension ID to the list of negotiated extensions accordingly
#2: just check in SetRemoteDescription if the RID header extension got added by the answerer and again add the RID extension ID to list of negotiated extensions accordingly
#3: add a special testing only function call (similar to select_ssrc() only executable with super powers) which adds a given ID number as the RID to the list of negotiated RTP header extensions
#4: add a special testing only function call (see above) which allows to query a PC which SSRC it's going to use for a given RID value

All of these should only allow the RTP parser to correctly parse the RID header extension so that in another step we can then extend our RTP filters with RID filtering.

I don't like options #1 and #2 as they incentivize SDP munging. I like option #3 the best, because we can setup the filters on either side once the (modified) offer has been generated. Where option #4 requires to do PC interrogation in the answerer scenario, right after creating the answer, but before passing the answer to setLocalDescription() as the PC as that point can start transmitting RTP at any point.
Assignee: nobody → mfroman
Depends on: 1351531
Depends on: 1351590
Could someone summarize the end-user implications, if any?
Rank: 18
Priority: -- → P1
(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> Could someone summarize the end-user implications, if any?

None. This affects only scenarios in which Firefox is receiving simulcast. Which should be only the case in our tests and not out in the wild.
Rank: 18 → 23
Priority: P1 → P2
Comment on attachment 8862086 [details]
Bug 1358224 - pt 2 - change to RTP stream id filtering on simulcast mochitests.

https://reviewboard.mozilla.org/r/134058/#review137048

::: dom/media/tests/mochitest/test_peerConnection_simulcastAnswer.html:93
(Diff revision 1)
> -        function PC_REMOTE_SET_RTP_FIRST_RID(test) {
> -          // Cause pcLocal to filter out everything but the first SSRC. This
> -          // lets only one of the simulcast streams through.
> -          selectRecvSsrc(test.pcLocal, 0);
> +      // has been created.
> +      test.chain.insertAfter('PC_LOCAL_SET_REMOTE_DESCRIPTION',[
> +        function PC_LOCAL_SET_RTP_FIRST_RID(test) {
> +          // Cause pcLocal to filter out everything but RID "bar", only
> +          // allowing one of the simulcast streams through.
> +          addRIDExtension(test.pcLocal, 3);

I think explaining where the number 3 is coming from might help to fix this if it breaks in the future.

::: dom/media/tests/mochitest/test_peerConnection_simulcastOffer.html:86
(Diff revision 1)
> +      test.chain.insertAfter('PC_REMOTE_SET_LOCAL_DESCRIPTION',[
>          function PC_REMOTE_SET_RTP_FIRST_RID(test) {
> -          // Cause pcRemote to filter out everything but the first SSRC. This
> -          // lets only one of the simulcast streams through.
> -          selectRecvSsrc(test.pcRemote, 0);
> +          // Cause pcRemote to filter out everything but RID "bar", only
> +          // allowing one of the simulcast streams through.
> +          addRIDExtension(test.pcRemote, 3);
> +          selectRecvRID(test.pcRemote, "bar");

Would be good if the offerer test checks for "foo" then "bar" and the answerer test check in the opposite order (first "bar" then "foo"). But that is optional. Leave it out if causes too much trouble.
Attachment #8862086 - Flags: review?(drno) → review+
Comment on attachment 8862085 [details]
Bug 1358224 - pt 1 - addRIDExtension and addRIDFilter chrome-only API for RID (RTP Stream Id) filtering of receive tracks.

https://reviewboard.mozilla.org/r/134056/#review137056
Attachment #8862085 - Flags: review?(kyle) → review+
Comment on attachment 8862086 [details]
Bug 1358224 - pt 2 - change to RTP stream id filtering on simulcast mochitests.

https://reviewboard.mozilla.org/r/134058/#review137048

> Would be good if the offerer test checks for "foo" then "bar" and the answerer test check in the opposite order (first "bar" then "foo"). But that is optional. Leave it out if causes too much trouble.

I went ahead and made that change too.
Comment on attachment 8862584 [details]
Bug 1358224 - pt 3 - fix leak in RTPHeaderExtension's rid char buffer.

https://reviewboard.mozilla.org/r/134444/#review137450
Attachment #8862584 - Flags: review?(drno) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/55f0af5ad661
pt 1 - addRIDExtension and addRIDFilter chrome-only API for RID (RTP Stream Id) filtering of receive tracks. r=qdot
https://hg.mozilla.org/integration/autoland/rev/cbecfb20b227
pt 2 - change to RTP stream id filtering on simulcast mochitests. r=drno
https://hg.mozilla.org/integration/autoland/rev/95fc658ecda5
pt 3 - fix leak in RTPHeaderExtension's rid char buffer. r=drno
Keywords: checkin-needed
Blocks: 1361139
Comment on attachment 8862086 [details]
Bug 1358224 - pt 2 - change to RTP stream id filtering on simulcast mochitests.

https://reviewboard.mozilla.org/r/134058/#review140740

Hi! A drive-by review here, as I reviewed this by mistake in bug 1359775 comment 17 due to mozreview's bug where it includes too much code after a rebase. Didn't want the input to go to waste.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:738
(Diff revision 2)
>  void
> +MediaPipeline::AddRIDExtension_m(size_t extension_id)
> +{
> +  RUN_ON_THREAD(sts_thread_,
> +                WrapRunnable(this,

Don't we need

    RefPtr<MediaPipeline>(this)

here to keep this alive?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:756
(Diff revision 2)
> +MediaPipeline::AddRIDFilter_m(const std::string& rid)
> +{
> +  RUN_ON_THREAD(sts_thread_,
> +                WrapRunnable(this,

ditto.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:668
(Diff revision 2)
> +  RefPtr<MediaPipeline> GetMediaPipelineForTrack(
> +      dom::MediaStreamTrack& aRecvTrack);

This breakdown seems to add an extra refcount compared to before. Is it needed?
Comment on attachment 8862086 [details]
Bug 1358224 - pt 2 - change to RTP stream id filtering on simulcast mochitests.

https://reviewboard.mozilla.org/r/134058/#review140740

> Don't we need
> 
>     RefPtr<MediaPipeline>(this)
> 
> here to keep this alive?

Both AddRIDExtension_m and AddRIDFilter_m are test-only calls used for the simulcast mochitests that wait for media flow which also means there is virtually no chance that 'this' can go away while dispatching to the sts thread.  If you still feel super strongly about the issue, let me know and I'll open a new bug to make the change.

> This breakdown seems to add an extra refcount compared to before. Is it needed?

Not sure I follow.  PeerConnectionImpl.cpp:2357 is returning it->second which is the value from std::map<std::string, RefPtr<MediaPipeline>>.  Are you suggesting I return the raw ptr ( it->second.get() ) instead?
Comment on attachment 8862086 [details]
Bug 1358224 - pt 2 - change to RTP stream id filtering on simulcast mochitests.

https://reviewboard.mozilla.org/r/134058/#review140740

> Both AddRIDExtension_m and AddRIDFilter_m are test-only calls used for the simulcast mochitests that wait for media flow which also means there is virtually no chance that 'this' can go away while dispatching to the sts thread.  If you still feel super strongly about the issue, let me know and I'll open a new bug to make the change.

Good to know it is not web exposed. Still, I wouldn't leave known Use-After-Free code lying around like this for anyone to cut'n'paste or get ideas from. This would not have passed static analysis had lambdas been used fwiw.

> Not sure I follow.  PeerConnectionImpl.cpp:2357 is returning it->second which is the value from std::map<std::string, RefPtr<MediaPipeline>>.  Are you suggesting I return the raw ptr ( it->second.get() ) instead?

The RefPtr copy constructor addrefs. We could return RefPtr<MediaPipeline>& here instead I think.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #17)
> Good to know it is not web exposed. Still, I wouldn't leave known
> Use-After-Free code lying around like this for anyone to cut'n'paste or get
> ideas from. This would not have passed static analysis had lambdas been used
> fwiw.
"lying around like this for anyone to cut'n'paste" is exactly how I ended up with that code. ;-)  I'll create a bug to fix this.

> The RefPtr copy constructor addrefs. We could return RefPtr<MediaPipeline>&
> here instead I think.
I tried that originally, but the compiler complained about "return nullptr;". Jan-Ivar and I talked and "return Move(RefPtr<MediaPipeline>(nullptr));" takes care of the warning, so I'll clean this up as well in the new bug.
Depends on: 1365291
Comment on attachment 8862085 [details]
Bug 1358224 - pt 1 - addRIDExtension and addRIDFilter chrome-only API for RID (RTP Stream Id) filtering of receive tracks.

Approval Request Comment
[Feature/Bug causing the regression]: 1338521 landed and exercises 1351590 and 1351531 (intermittent oranges for simulcast offer/answer mochitests).
[User impact if declined]: No end-user impact, but lots of oranges on beta
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Not risky because it is a test-only fix.
[String changes made/needed]: n/a
Attachment #8862085 - Flags: approval-mozilla-beta?
Attachment #8862086 - Flags: approval-mozilla-beta?
Attachment #8862584 - Flags: approval-mozilla-beta?
Comment on attachment 8862085 [details]
Bug 1358224 - pt 1 - addRIDExtension and addRIDFilter chrome-only API for RID (RTP Stream Id) filtering of receive tracks.

Looks like this may help fix a bunch of test failures on beta that went wild when we uplifted the fix from bug 13385521 (for webrtc freezes). Let's uplift this fix too.
Attachment #8862085 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Part 3 has conflicts around bug 1325173. I tried to rebase around them but the Try run ended up orange. We'll need a rebased patch or an approval request on that bug too.
Flags: needinfo?(mfroman)
drno: This is now outside of what I'm comfortable with uplifting.  Do you want to add this, or go back to disabling the simulcast mochitest fixes until they ride the trains?
Flags: needinfo?(mfroman) → needinfo?(drno)
As the uplift is not that far out it is probably safer to not uplift bug 1325173, and hope nobody uplifts anything in the remaining days which destroys the simulcast feature in Beta or Release (after the uplift).
Flags: needinfo?(drno)
Comment on attachment 8862085 [details]
Bug 1358224 - pt 1 - addRIDExtension and addRIDFilter chrome-only API for RID (RTP Stream Id) filtering of receive tracks.

this isn't going to beta54 after all
Attachment #8862085 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
Attachment #8862086 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8862584 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.