985253 - SDP negotiation for H264

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Maire Reavy [:mreavy]]

Add SDP negotiation (signaling) for H.264 support in WebRTC.

Comment 1

[Maire Reavy [:mreavy]]

Ethan has agreed to own this bug, and Mozilla will help him as needed (since he's busy on multiple pieces of this project).

Comment 2

[Ethan Hugg [:ehugg]]

I believe this entails adding lines like this on a CreateOffer

a=rtpmap:97 H264/90000
a=fmtp:97 profile-level-id=42E00C

And similarly negotiating those a= lines on CreateAnswer.

OpenH264 will be using GMP, but there is nothing in that API about these signaling details.  Do we want to complicate GMP with SDP details or do we want to have to edit the SDP handling code for every new plugin that using GMP?  Similarly if the profile-level-id changed for the plugin we'd have to change the signaling code at the same time.

EKR do you have thoughts on this?

Comment 3

[Eric Rescorla (:ekr)]

I think initially we should bake the assumption that the only codecs are H.264 and VP8 into the system for now. In the future, we should expand the interface to have the extension points you suggest.

Comment 4

[Ethan Hugg [:ehugg]]

A couple more design details.  I assume that we will only want to add H264 to the Offer/Answer if 

1. The pref for H264 is on - assuming there will be an about:config pref for this.
2. The GMP OpenH264 plugin has successfully been loaded.

Comment 5

[Eric Rescorla (:ekr)]

I think only #2 is relevant in the long term.

Comment 6

[Ethan Hugg [:ehugg]]

I was under the impression that we would always have a way in the UI for folks to turn off this feature.

Comment 7

[Eric Rescorla (:ekr)]

We'll have a way for people to disable the plugin, so that would fall under #2

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

The patches that landed as part of bug 911046 (OMX H.264 support) invoke a fair amount of existing H.264 SDP support in SIPCC, though it doesn't trigger support for Mode 1 (which is triggered by  "5.1.0 (AngelFire)" and vcmGetVideoMaxSupportedPacketizationMode() >= 1).  Some small changes can likely bring this up to a workable level of SDP support for H.264

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

We should probably detail out here which parts of H.265/RFC 6184 are needed for initial use, and which we want eventually (in particular for interop-with-non-webrtc cases), and have that be a second, follow-on bug.

Required for initial use:

send/parse a:rtpmap
send a:fmtp with profile-level-id and packetization-mode
parse a:fmtp with profile-level-id and packetization-mode, max-fs, max-mbps, sprop-parameter-sets, others?
support for mode 0 and probably mode 1
profile-level negotiation
<list may be incomplete>

Very useful, perhaps required:
send a:fmtp max-fs, max-mbps

Very useful:
send a:fmtp sprop-parameter-sets

Comment 10

[Maire Reavy [:mreavy]]

Ethan, We need basic SDP negotiation for FxOS.  Randell and I will coordinate with you on what's needed and who should do what.  We'll likely want to use this for the "basic" (or minimal) SDP work and a follow on bug for additional SDP work.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch to fix rtcp-fb

This adds a=rtcp-fb's for H264 (important), but they don't appear in the answer (though they should); they do show up in the offer.  Note that it appears the answer doesn't find any in the offer....  Added some debugs; got too late to debug so uploading.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Send rtcp-fb for all video codecs, and fix answer generation for H.264 for rtcp-fb

Found the issue; default packetization mode wasn't being applied correctly and that was causing some of the code to think we were in mode 1, causing rtcp-fb not to be inserted in an answer.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

We'll want to leave this open for more SDP improvements for H.264.  The current patch is minimal support.

Comment 14

[Ethan Hugg [:ehugg]]

Comment on attachment 8415334 [details] [diff] [review]
Send rtcp-fb for all video codecs, and fix answer generation for H.264 for rtcp-fb


These changes look fine to me.  Do you think tests should be added to the signaling unittests for this?

Also, do you have a way of trying out this negotiation on desktop or just on B2G?

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

I'll file a followup for tests (I suspect we don't have good (enough) multiple-video-codec tests for example).

Right now, it's triggered by the external OMX H264 codec being enabled; that said you just need to make it think H264 is in play (OMX cheats for now and hijacks I420, which we don't use anyways).  Note that support is spotty and things like mode one are blocked by a test for a specific cisco product... so there's more to do here.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4d621eeada

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4c4d621eeada

Comment 18

[Maire Reavy [:mreavy]]

Randell's going to take on what's needed for OpenH264 and H.264 on FxOS to ship (version 1).  Any work beyond the minimum to ship will be done in a follow-up bug.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP patch for H.264 RTP mode 1 support in webrtc signaling

Assumes h264 packetization patches have been applied first; some hacks around the external-codec interface to remove.  Works.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8424674 [details] [diff] [review]
WIP patch for H.264 RTP mode 1 support in webrtc signaling

We still need to figure out what the "right" way to handle the "use an external-registered codec" should be.  This is hacked right now; otherwise this should be mostly done other than hooking up SDP stuff to the H264 codec-specific params.

Comment 21

[Ethan Hugg [:ehugg]]

Comment on attachment 8424674 [details] [diff] [review]
WIP patch for H.264 RTP mode 1 support in webrtc signaling

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

So, according to bug 985253 comment 7 it seems that the plan of record is to have any relevant config settings affect the loading on startup of GMP plugins.  This means we need to to check some flag that tells us whether GMP loaded anything.  I'm guessing we could hold a global in the GMP code or do a SetBoolPref in GMP to the pref service.  Then you could check it directly instead of using mExternalRecvCodec and perhaps hardcode the mType for now.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +692,5 @@
> +              if(CopyCodecToDB(codecConfigList[i]))
> +              {
> +                success = true;
> +              } else {
> +                CSFLogError(logTag,"%s Unable to updated Codec Database", __FUNCTION__);

The cut/paste here replicated this misspelling.

@@ +1161,5 @@
>    cinst.plType  = codecInfo->mType;
> +  static_assert(sizeof(cinst.plName) >= sizeof("H264"), "codec name too long"); // longest string
> +  if (codecInfo->mName == "H264_P0" || codecInfo->mName == "H264_P1") {
> +    cinst.codecType = webrtc::kVideoCodecH264;
> +    strcpy(cinst.plName,"H264");

I got rid of all of these in bug 776682.  Would be helpful to replace with sstrncpy just to make it easier to find other dangerous uses.

@@ +1175,1 @@
>    // leave width/height alone; they'll be overridden on the first frame

I assume this comment is no longer useful.

Comment 22

[Ethan Hugg [:ehugg]]

Comment on attachment 8415334 [details] [diff] [review]
Send rtcp-fb for all video codecs, and fix answer generation for H.264 for rtcp-fb


Resetting the r+ that was accidently removed.

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Ethan Hugg [:ehugg] from comment #21)
> So, according to bug 985253 comment 7 it seems that the plan of record is to
> have any relevant config settings affect the loading on startup of GMP
> plugins.  This means we need to to check some flag that tells us whether GMP
> loaded anything.  I'm guessing we could hold a global in the GMP code or do
> a SetBoolPref in GMP to the pref service.  Then you could check it directly
> instead of using mExternalRecvCodec and perhaps hardcode the mType for now.

So, with OpenH264: we check a setting of some sort to tell us if H264 is supported - and if so, with what limits, since hardware H264 and OpenH264 will not necessarily use the same levels, constraints, max-fs, max-mbps, etc.  This speaks to a registry the codecs add themselves to "at some point"; I'd prefer to defer this until first-use (i.e. on first SDP generation load any GMP plugins and check for any hardware encoders/decoders).  Also, support decoder-only HW, etc.  Things get fun if you have Main profile OpenH264 and HW baseline profile............ :-(

We can use temporary hard-coded workarounds for now.


> @@ +1161,5 @@
> >    cinst.plType  = codecInfo->mType;
> > +  static_assert(sizeof(cinst.plName) >= sizeof("H264"), "codec name too long"); // longest string
> > +  if (codecInfo->mName == "H264_P0" || codecInfo->mName == "H264_P1") {
> > +    cinst.codecType = webrtc::kVideoCodecH264;
> > +    strcpy(cinst.plName,"H264");
> 
> I got rid of all of these in bug 776682.  Would be helpful to replace with
> sstrncpy just to make it easier to find other dangerous uses.

Sure.  I did include a static_assert, though.  :-)

> 
> @@ +1175,1 @@
> >    // leave width/height alone; they'll be overridden on the first frame
> 
> I assume this comment is no longer useful.

They will be overridden; I'll revise.  Webrtc.org demands the initial size be sane.

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Attachment: H.264 RTP mode 1 support in webrtc signaling

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8425829 [details] [diff] [review]
H.264 RTP mode 1 support in webrtc signaling

A relatively clean way to handle "external" configed codecs for now.

Comment 26

[Ethan Hugg [:ehugg]]

Comment on attachment 8425829 [details] [diff] [review]
H.264 RTP mode 1 support in webrtc signaling

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

I think this will be fine for now.  Just nits.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +685,4 @@
>          {
> +          success = true;
> +        } else {
> +          CSFLogError(logTag,"%s Unable to updated Codec Database", __FUNCTION__);

Nit: I know this a cut-paste but it would be good to change 'updated' to 'update' on this one and the copy below.

@@ +1191,5 @@
> +  } else if (codecInfo->mName == "I420") {
> +    cinst.codecType = webrtc::kVideoCodecI420;
> +    PL_strncpyz(cinst.plName, "I420", sizeof(cinst.plName));
> +  }
> +  // XXX more...

Should this comment reference this bug or be more informative in some way.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2732,5 @@
>   * must be 0 or 1. Value 2 is not supported yet.
>   */
>  int vcmGetVideoMaxSupportedPacketizationMode()
>  {
> +  // XXX bump when we add mode 1 packetization

Should this comment start with something besides XXX, perhaps this bug number or something else more informative.

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Update to patch for H.264 RTP mode 1

Still has a hack in there to force SPS/PPS to separate timestamps, since keeping them on the same timestamp doesn't work 100%

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Green try on the mass of patches -
https://tbpl.mozilla.org/?tree=Try&rev=36fed02b8193

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11b8d0665232

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11b8d0665232

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

leave-open was for the first patch we landed for this bug