Closed Bug 1043515 Opened 5 years ago Closed 5 years ago

H264 SDP Issues

Categories

(Core :: WebRTC: Signaling, defect)

34 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
blocking-b2g 2.0+
Tracking Status
firefox32 --- unaffected
firefox33 --- fixed
firefox34 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: alan.ford, Assigned: jesup)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
Attachment #8463143 - Flags: review?(ethanhugg)
Attachment #8463144 - Flags: review?(ethanhugg)
Assignee: nobody → rjesup
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [openh264-uplift]
Component: WebRTC → WebRTC: Signaling
Blocks: OpenH264
Attachment #8463152 - Flags: review?(ethanhugg)
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.
Attachment #8463152 - Flags: review?(ethanhugg) → review+
Attachment #8463144 - Flags: review?(ethanhugg) → review+
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
Attachment #8463143 - Flags: review?(ethanhugg) → review+
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
Attachment #8463143 - Flags: approval-mozilla-aurora?
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.
Attachment #8463144 - Flags: approval-mozilla-aurora?
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
Attachment #8463152 - Flags: approval-mozilla-beta?
Attachment #8463152 - Flags: approval-mozilla-aurora?
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).
Attachment #8463144 - Flags: approval-mozilla-beta?
Attachment #8463143 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8463144 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8463152 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8463144 - Flags: approval-mozilla-beta?
Attachment #8463152 - Flags: approval-mozilla-beta?
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.0?
QA Contact: drno
This is a new feature for 2.0 hence blocking for broken new feature
blocking-b2g: 2.0? → 2.0+
Whiteboard: [openh264-uplift]
You need to log in before you can comment on or make changes to this bug.