1043515 - H264 SDP Issues

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Alan Ford]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3) AppleWebKit/537.76.4 (KHTML, like Gecko) Version/7.0.4 Safari/537.76.4

Steps to reproduce:

We have been playing with trying to get Firefox H264 to speak to a standard SIP endpoint, and have discovered some further issues with Firefox's H264 SDP. There are two problems:

1. limitations in Firefox H264 profile
2. limitations in parsing incoming fmtp lines


Actual results:

The a=fmtp line presented by Firefox is as follows:
a=fmtp:126 profile-level-id=42e00c;packetization-mode=1

Profile-level-id 42e00c maps to H264 profile level 1.2, which maps to the following parameters:

'max-mbps': 6000 (maximum macroblocks per second)
'max-fs': 396 (maximum frame size)
'max-br': 384 (maximum bit rate)

This means the maximum resolutions it's saying it can support is CIF @ 15fps or QCIF @ 30fps. Neither of which give a particularly good experience. Overriding the max-mbps parameter on the a=fmtp line would allow Firefox to get up to CIF @ 30fps.

draft-burman-rtcweb-h264-proposal recommends profile level 1.3 in order to support 720p @ 30fps (which sets 'max-mbps': 11880, 'max-fs': 396, 'max-br': 768). Since VP8 is used at up to 2Mbps, however, it would seem desirable to offer an even higher quality profile level.


The second issue is that the SDP being sent back to Firefox from the remote end includes some non-standard parameters (specifically, max-smbps and max-fps) on the fmtp line. Firefox appears not to like the presence of any parameters it does not recognise (we had to remove these to get to the above working scenario at QCIF). RFC6184 is pretty clear that a receiver MUST ignore any parameters it does not recognise.

Comment 1

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

Attachment: Add max-br and max-mbps H.264 SDP fmtp parameters; update supported h264 level

Comment 2

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

Attachment: Ignore unknown fmtp values; partially fix unittests to handle H264_P0 disabled

Comment 3

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

Attachment: Add support for a preferred codec to be chosen/offered first always

Comment 4

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

https://tbpl.mozilla.org/?tree=Try&rev=ba6fd459db11

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 8463152 [details] [diff] [review]
Add support for a preferred codec to be chosen/offered first always

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

::: media/webrtc/signaling/src/sipcc/core/common/prot_configmgr.c
@@ +466,5 @@
> +
> +  if(vcmGetVideoPreferredCodec((int32_t *) &codec) == 0) {
> +    return (rtp_ptype) codec;
> +  }
> +#else

Don't you want to remove this code instead of leaving it ifdef'd out?

@@ +702,5 @@
> +          aSupportedCodecs[0] = pref_codec;
> +          return count;
> +        }
> +      }
> +      // preferred not found, oh well

We need a log message here I think.

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 8463143 [details] [diff] [review]
Add max-br and max-mbps H.264 SDP fmtp parameters; update supported h264 level

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2878,5 @@
>    return 0x42E00C;
> +#else
> +  // XXX See bug 1043515 - we may want to support a higher profile than
> +  // 1.3, depending on hardware(?)
> +  return 0x42E00D;

I believe this test from Nils looks for exactly 42e00c.  Not sure which will land first.

https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1042791&attachment=8462333

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb83a67e4bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/d81eb5a189a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4477aa19a72

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fdb83a67e4bb
https://hg.mozilla.org/mozilla-central/rev/d81eb5a189a5
https://hg.mozilla.org/mozilla-central/rev/f4477aa19a72

Comment 9

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

Comment on attachment 8463143 [details] [diff] [review]
Add max-br and max-mbps H.264 SDP fmtp parameters; update supported h264 level

This is for all 3 patches

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Interop problems with other equipment using H.264

[Describe test coverage new/current, TBPL]: Baked on central for a week; tested in interop situations by Cisco

[Risks and why]: Virtually none - very simple hooking up of existing support for the fmtp parameters in the code to vars, and updating the default H.264 level to be correct.

[String/UUID change made/needed]: None

Comment 10

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

Comment on attachment 8463144 [details] [diff] [review]
Ignore unknown fmtp values; partially fix unittests to handle H264_P0 disabled

Approval Request Comment
Note: includes CPP unit tests

This is especially critical for interop as we must ignore parameters we don't understand.

Comment 11

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

Comment on attachment 8463152 [details] [diff] [review]
Add support for a preferred codec to be chosen/offered first always

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Important for certain users, and for 2.0 8x10 devices in interop situations.

[Describe test coverage new/current, TBPL]: Baked on central, tried here and at Cisco.

[Risks and why]: virtually none (none if pref isn't used)

[String/UUID change made/needed]: none

Comment 12

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

Comment on attachment 8463144 [details] [diff] [review]
Ignore unknown fmtp values; partially fix unittests to handle H264_P0 disabled

Requesting Beta on this as well for use of 8x10 HW H.264 devices in interop situations (ability to ignore unknown values would block interop use).

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8463144 [details] [diff] [review]
Ignore unknown fmtp values; partially fix unittests to handle H264_P0 disabled

Randell confirmed that this fix is only required for b2g. As such, noming for 2.0. Justification is in comment 11.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

[Blocking Requested - why for this release]:

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fafb35ae7892
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7a26b0d0de5
https://hg.mozilla.org/releases/mozilla-aurora/rev/1f0ef1b4edd7

Comment 16

[Preeti Raghunath(:Preeti)]

This is a new feature for 2.0 hence blocking for broken new feature

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f33de1612654
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/a35314c9c656
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/464c236b1193