Closed
Bug 1107307
Opened 11 years ago
Closed 11 years ago
JsepSessionImpl does not handle '*' as payload types in rtcp-fb
Categories
(Core :: WebRTC: Signaling, defect)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: bwc, Assigned: bwc)
References
Details
Attachments
(1 file, 1 obsolete file)
|
6.77 KB,
patch
|
drno
:
review+
|
Details | Diff | Splinter Review |
The SDP wrapper handles this fine, we just need to teach the logic in signaling/jsep to treat '*' specially.
|
Assignee | |
Comment 1•11 years ago
|
||
This might just do the trick.
|
|
Assignee | |
Updated•11 years ago
|
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 2•11 years ago
|
||
Fix SDP wrapper code, and add test-cases.
|
|
Assignee | |
Updated•11 years ago
|
Attachment #8531761 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•11 years ago
|
Attachment #8532063 -
Flags: review?(drno)
|
||
Comment 3•11 years ago
|
||
Comment on attachment 8532063 [details] [diff] [review]
Teach JsepSessionImpl to handle rtcp-fb:*
Review of attachment 8532063 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM, just one small NIT.
::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
@@ +135,1 @@
> if (!negotiated->LoadRtcpFbs(*i)) {
NIT: shouldn't we merge these two if's into one?
Attachment #8532063 -
Flags: review?(drno) → review+
|
|
Assignee | |
Comment 4•11 years ago
|
||
(In reply to Nils Ohlmeier [:drno] from comment #3)
> Comment on attachment 8532063 [details] [diff] [review]
> Teach JsepSessionImpl to handle rtcp-fb:*
>
> Review of attachment 8532063 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> LGTM, just one small NIT.
>
> ::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
> @@ +135,1 @@
> > if (!negotiated->LoadRtcpFbs(*i)) {
>
> NIT: shouldn't we merge these two if's into one?
I almost always find that mixing || and && in the same if statement is less readable than breaking the if statement up.
|
|
||
Comment 5•11 years ago
|
||
(In reply to Byron Campen [:bwc] from comment #4)
> I almost always find that mixing || and && in the same if statement is less
> readable than breaking the if statement up.
Fair enough.
|
|
Assignee | |
Comment 6•11 years ago
|
||
Flags: needinfo?(docfaraday)
|
|
Assignee | |
Comment 7•11 years ago
|
||
Flags: needinfo?(docfaraday)
|
||
Comment 8•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•