Closed Bug 1107307 Opened 7 years ago Closed 7 years ago

JsepSessionImpl does not handle '*' as payload types in rtcp-fb

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(1 file, 1 obsolete file)

The SDP wrapper handles this fine, we just need to teach the logic in signaling/jsep to treat '*' specially.
This might just do the trick.
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Fix SDP wrapper code, and add test-cases.
Attachment #8531761 - Attachment is obsolete: true
Attachment #8532063 - Flags: review?(drno)
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+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/08707baf514f
Status: ASSIGNED → RESOLVED
Closed: 7 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.