Add H.264 STAP-A reception support

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We must support STAP-A reception for mode-1 H.264 packetization, which is easy.  We do not have to support sending STAP-A, though it would be nice for SPS/PPS.

The original patches from bug 985254 did not include STAP-A support.
Comment on attachment 8452610 [details] [diff] [review]
Support H.264 STAP-A depacketization in webrtc

Also, could you try it with some of your existing H.264 equipment?  Thanks
Attachment #8452610 - Flags: review?(ethanhugg)
2.0? because RFC 6184 mandates that for H.264 mode 1 packetization, you MUST support receiving STAP-A aggregate packets - and most mode-1 equipment will send them.
blocking-b2g: --- → 2.0?
Whiteboard: [webrtc-uplift]
Comment on attachment 8452610 [details] [diff] [review]
Support H.264 STAP-A depacketization in webrtc

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

This looks good to me but I am unable at this time to connect it to try it out.
Attachment #8452610 - Flags: review?(ethanhugg) → review+
(In reply to Randell Jesup [:jesup] from comment #3)
> 2.0? because RFC 6184 mandates that for H.264 mode 1 packetization, you MUST
> support receiving STAP-A aggregate packets - and most mode-1 equipment will
> send them.

Triage is having some difficulty understanding this. Can you explain this in terms of end-user impact if what would happen if we did not fix this?
Flags: needinfo?(rjesup)
So if we connect (via gateway) to any standard H.264 equipment, such as Cisco gear (which is a lot of why they're providing OpenH264 gratis), they will use RFC 6184 mode-1 packetization (which is all we support currently).  The problem is that they will send both FU-A (fragmented) packets, and STAP-A (aggregate packets), and without this patch we don't understand STAP-A (we currently only send FU-A).  STAP-A is mandated if you do mode 1.

In addition, if we fix our code to generate STAP-A in the future, we will then be incompatible with older revs of Firefox (i.e. a FF33 or 34 calls a B2G 2.0 phone (based on 32) - it would fail.)

I plan to land this on inbound today; I was waiting until I could double-check functionality (with a temporary patch to force sending some STAP-A packets) since we don't have a working gateway to other H.264 stuff yet.
Flags: needinfo?(rjesup)
Needed for compat, so blocking.
blocking-b2g: 2.0? → 2.0+
Now includes test code; works.  Fixed the header data, which needed to be set up like single-nals before forwarding the extracted packet onwards, and ntohs() the length words.
Attachment #8452610 - Attachment is obsolete: true
Attachment #8454228 - Flags: review?(ethanhugg)
Comment on attachment 8454228 [details] [diff] [review]
Support H.264 STAP-A depacketization in webrtc

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

This looks OK and appears to work for me on OSX with TEST_STAP_A turned on.

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc
@@ +292,5 @@
> +      h264_header->nalu_header        = payload[0];
> +      switch (*payload & RtpFormatH264::kH264NAL_TypeMask) {
> +        case RtpFormatH264::kH264NALU_SPS:
> +          // TODO(jesup): Evil hack.  see below
> +          rtp_header->header.timestamp = timestamp - 20;

The evil hack below has SPS and PPS both -10, what am I missing that would make this one -20 instead?
Attachment #8454228 - Flags: review?(ethanhugg) → review+
> > +      h264_header->nalu_header        = payload[0];
> > +      switch (*payload & RtpFormatH264::kH264NAL_TypeMask) {
> > +        case RtpFormatH264::kH264NALU_SPS:
> > +          // TODO(jesup): Evil hack.  see below
> > +          rtp_header->header.timestamp = timestamp - 20;
> 
> The evil hack below has SPS and PPS both -10, what am I missing that would
> make this one -20 instead?

Actually, the lower one gets -10 and -20 due to the fall-throughs
Depends on: 1037363
https://hg.mozilla.org/mozilla-central/rev/2bdf8a5472ce
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [webrtc-uplift]
You need to log in before you can comment on or make changes to this bug.