985254 - RTP packetization for H.264

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Maire Reavy [:mreavy]]

Add RTP packetization for H.264 as part of adding OpenH264 support to WebRTC

Comment 1

[Maire Reavy [:mreavy]]

Someone from Cisco will write the patches for this bug.  Ethan is helping to identify who and will then help get them set up to contribute the code.

Comment 2

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

Attachment: Port of Mo Zanaty's patch for webrtc.org to our branch (3.43)

From https://webrtc-codereview.appspot.com/11239004/

Comment 4

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

I'm doing some more work on this.  Since upstream will not be taking it as-is, I'm making it an internal codec (and adding h264 codecSpecific data from negotiation); we can back it out and replace it with a patch from upstream when it's ready.

Taking at least temporarily

Comment 5

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

Attachment: H264 RTP packetization imported from upstream patchset #10

This is patchset #10 from upstream https://webrtc-codereview.appspot.com/13399004/  That patchset still needs work, and I'll upload patches that make it function in mozilla.

Comment 6

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

Attachment: Add H264 codec-specific structure to carry negotiated data

Alternatively we could put all the codec-specific stuff in the generic codec config collection.  Right now H264 is 90% built-in and 10% 'external'

Comment 7

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

Attachment: modify upstream h264 packetization patch to make it work

Comment 8

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

Attachment: Modify H264 OMX code to deal with upstream code inserting start codes

Fiddled the patch queue order this depends on.  with the mode 1 changes, the timestamp stuff for SPS becomes irrelevant, but not until then

Comment 9

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

Attachment: cleanup upstream h264 packetization patch per review comments upstream

Comment 10

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

Attachment: modify upstream h264 packetization patch to make it work

Comment 11

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

Comment on attachment 8425153 [details] [diff] [review]
cleanup upstream h264 packetization patch per review comments upstream

These are partial cleanups of issues I flagged during upstream review at
https://webrtc-codereview.appspot.com/13399004/

I'm also submitting these patches there for review at webrtc.org, but I don't know how long that will take.

Comment 12

[Paul Kerr [:pkerr]]

Comment on attachment 8425153 [details] [diff] [review]
cleanup upstream h264 packetization patch per review comments upstream

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

R+ with some nits/questions.

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc
@@ +242,2 @@
>  
> +    uint8_t original_nal_header = fnri | original_nal_type;

Why reconstruct payload_data[0] instead of just assigning it to original_payload?

::: media/webrtc/trunk/webrtc/modules/video_coding/main/source/session_info.cc
@@ +444,5 @@
>        last_packet_seq_num_ = static_cast<int>(packet.seqNum);
>      } else if (last_packet_seq_num_ != -1 &&
>        IsNewerSequenceNumber(packet.seqNum, last_packet_seq_num_)) {
> +      //LOG(LS_WARNING) << "Received packet with a sequence number which is out "
> +      //                 " of frame boundaries";

Wouldn't it be better to remove this or conditionally compile instead of commenting out?

@@ +472,5 @@
>        first_packet_seq_num_ = static_cast<int>(packet.seqNum);
>      } else if (first_packet_seq_num_ != -1 &&
>        !IsNewerSequenceNumber(packet.seqNum, first_packet_seq_num_)) {
> +      //LOG(LS_WARNING) << "Received packet with a sequence number which is out "
> +      //                 "of frame boundaries";

see above

@@ +486,5 @@
>        last_packet_seq_num_ = static_cast<int>(packet.seqNum);
>      } else if (last_packet_seq_num_ != -1 &&
>          IsNewerSequenceNumber(packet.seqNum, last_packet_seq_num_)) {
> +      //LOG(LS_WARNING) << "Received packet with a sequence number which is out "
> +      //                 "of frame boundaries";

see above

Comment 13

[Paul Kerr [:pkerr]]

Comment on attachment 8425154 [details] [diff] [review]
modify upstream h264 packetization patch to make it work

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

r+ with some nits/questions.

::: media/webrtc/trunk/webrtc/modules/interface/module_common_types.h
@@ +1066,5 @@
>  
> +inline bool IsNewerOrSameTimestamp(uint32_t timestamp, uint32_t prev_timestamp) {
> +  return timestamp == prev_timestamp ||
> +      static_cast<uint32_t>(timestamp - prev_timestamp) < 0x80000000;
> +}

Perhaps 0x80000000 should be replaced with a declared constant to be used in both comparison functions.

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc
@@ +278,5 @@
>  
>      // WebRtcRTPHeader
> +    if (nal_type == RtpFormatH264::kH264NALU_SPS ||
> +        nal_type == RtpFormatH264::kH264NALU_PPS ||
> +        nal_type == RtpFormatH264::kH264NALU_IDR) {

Expand on your comment as to why you need to set the frameType to kVideoFrameKey for SPS and PPS payloads in addition to an IDR.

Comment 14

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

Attachment: Modify H264 OMX code to deal with upstream code inserting start codes

Comment 15

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

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

Comment 16

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

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

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e88a722da07e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5246ba3954b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c1d93b3698
https://hg.mozilla.org/integration/mozilla-inbound/rev/d00df59cb452
https://hg.mozilla.org/integration/mozilla-inbound/rev/53c1c87969b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a470fb782be

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e88a722da07e
https://hg.mozilla.org/mozilla-central/rev/5246ba3954b7
https://hg.mozilla.org/mozilla-central/rev/d2c1d93b3698
https://hg.mozilla.org/mozilla-central/rev/d00df59cb452
https://hg.mozilla.org/mozilla-central/rev/53c1c87969b5
https://hg.mozilla.org/mozilla-central/rev/3a470fb782be