Clean up MediaPipeline::IsRtp()

NEW
Assigned to

Status

()

P5
trivial
Rank:
55
6 years ago
a year ago

People

(Reporter: jesup, Assigned: jesup, NeedInfo)

Tracking

22 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 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

6 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 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

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

Comment 4

3 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

3 years ago
Attachment #8715528 - Flags: review?(pkerr) → review+
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 hidden (Intermittent Failures Robot)
You need to log in before you can comment on or make changes to this bug.