779205 - Allow VP8 negotiation using SIPCC

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[enda mannion (:emannion)]

Attachment: VP8 Negotiation

This patch causes VP8 to be the default video codec and will enable negotiation of the said video codec.

Comment 1

[Ethan Hugg [:ehugg]]

Comment on attachment 647592 [details] [diff] [review]
VP8 Negotiation

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +107,2 @@
>    codecMask |= VCM_CODEC_RESOURCE_VP8;
>    codecMask |= VCM_CODEC_RESOURCE_I420;

How about I420, should we be removing this one as well?

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_private.h
@@ +439,5 @@
>      union {
>          tinybool              boolean_val;
>          u32                   u32_val;
>          char                  string_val[SDP_MAX_STRING_LEN+1];
> +        char                  ice_attr[120];

Can we use a def for this instead of just 120?  I'm all for not re-using SDP_MAX_STRING, but I think you should make a new one.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +51,5 @@
>    "a=candidate:1 1 UDP 2130706431 192.168.2.1 50005 typ host\r\n"
>    "a=candidate:2 2 UDP 2130706431 192.168.2.2 50006 typ host\r\n"
>    "m=video 1024 RTP/AVP 97\r\n"
>    "c=IN IP4 10.86.255.143\r\n"
> +  "a=rtpmap:120 VP8/90000\r\n"

Do we know that the right number here is 90000 also for VP8?

Comment 2

[enda mannion (:emannion)]

(In reply to Ethan Hugg [:ehugg] from comment #1)
> Comment on attachment 647592 [details] [diff] [review]
> VP8 Negotiation
> 
> Review of attachment 647592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
> @@ +107,2 @@
> >    codecMask |= VCM_CODEC_RESOURCE_VP8;
> >    codecMask |= VCM_CODEC_RESOURCE_I420;
> 
> How about I420, should we be removing this one as well?

I looked at a sample of Chromium SDP and they don't have I420 so I removed it.

> 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_private.h
> @@ +439,5 @@
> >      union {
> >          tinybool              boolean_val;
> >          u32                   u32_val;
> >          char                  string_val[SDP_MAX_STRING_LEN+1];
> > +        char                  ice_attr[120];
> 
> Can we use a def for this instead of just 120?  I'm all for not re-using
> SDP_MAX_STRING, but I think you should make a new one.

I added a new define, I can't re-use SDP_MAX_STRING as it is not wide enough as I discovered on my network.

> 
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +51,5 @@
> >    "a=candidate:1 1 UDP 2130706431 192.168.2.1 50005 typ host\r\n"
> >    "a=candidate:2 2 UDP 2130706431 192.168.2.2 50006 typ host\r\n"
> >    "m=video 1024 RTP/AVP 97\r\n"
> >    "c=IN IP4 10.86.255.143\r\n"
> > +  "a=rtpmap:120 VP8/90000\r\n"
> 
> Do we know that the right number here is 90000 also for VP8?

I looked as chromium SDP and they also use 9000 so I think we keep that for now.

Comment 3

[enda mannion (:emannion)]

Attachment: VP8 Negotiation

Comment 4

[enda mannion (:emannion)]

Attachment: Revised: VP8 Negotiation

Comment 5

[enda mannion (:emannion)]

Although this patch does not look like it was pushed to alder, this code has made it into the codebase, maybe through another patch. Thus this bug can be closed or made a duplicate. Right now I can not find the duplicate bug.

Comment 6

[Ethan Hugg [:ehugg]]

Pushed to M-C in the Alder merge rollup.  Bug 792188.  We are definitely negotiating VP8 in the SDP.