Clean up MediaPipeline::IsRtp()

NEW
Assigned to

Status

()

Core
WebRTC: Networking
P5
trivial
Rank:
55
5 years ago
7 months ago

People

(Reporter: jesup, Assigned: jesup, NeedInfo)

Tracking

22 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Created attachment 806619 [details] [diff] [review]
rtcp_cleanup

Code in MediaPipeline::IsRtp() is a bit messy and could be clearer/smaller
Trivial, but tripped over it while working something else
Attachment #806619 - Flags: review?(adam)

Comment 1

5 years ago
Comment on attachment 806619 [details] [diff] [review]
rtcp_cleanup

Lateralling this to EKR; I think he understands the context better than I do.
Attachment #806619 - Flags: review?(adam) → review?(ekr)

Comment 2

5 years ago
Comment on attachment 806619 [details] [diff] [review]
rtcp_cleanup

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

I'm not sure this is an improvement.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +468,5 @@
>  
>    // Check if this is a RTCP packet. Logic based on the types listed in
> +  // media/webrtc/trunk/src/modules/rtp_rtcp/source/rtp_utility.cc,
> +  // and needs to conform with RFC 5761 (RTP/RTCP Mux)
> +  // We don't support RFC 2032 FIR or NACK (193/194)

Why is removing 193 and 194 a good idea just
because we don't support them currently. This seems like a
pointy object waiting for us to step on if we ever add
them, especially because it's a way to get out of sync
with WebRTC.org. Has Google changed their code?

@@ +480,4 @@
>    if ((data[1] >= 200) && (data[1] <= 207))  // SR, RR, SDES, BYE,
>      return false;                            // APP, RTPFB, PSFB, XR
>  
> +  // Anything outside this range is RTP.

Having an assert here is intended to verify that we explicitly address each case.
Attachment #806619 - Flags: review?(ekr) → review-
(Assignee)

Comment 3

3 years ago
Ping myself to unbitrot
backlog: --- → webRTC+
Rank: 55
Flags: needinfo?(rjesup)
Priority: -- → P5
(Assignee)

Comment 4

2 years ago
Created attachment 8715528 [details]
MozReview Request: Bug 917794: make packet filter for RTP vs RTCP match webrtc.org impl r?pkerr

Review commit: https://reviewboard.mozilla.org/r/33497/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33497/
Attachment #8715528 - Flags: review?(pkerr)

Updated

2 years ago
Attachment #8715528 - Flags: review?(pkerr) → review+

Comment 5

2 years ago
Comment on attachment 8715528 [details]
MozReview Request: Bug 917794: make packet filter for RTP vs RTCP match webrtc.org impl r?pkerr

https://reviewboard.mozilla.org/r/33497/#review30207

Comment 6

7 months ago
3 failures in 864 pushes (0.003 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-inbound: 3

Platform breakdown:
* windows10-64-stylo-disabled: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=917794&startday=2017-10-16&endday=2017-10-22&tree=all
You need to log in before you can comment on or make changes to this bug.