Closed Bug 985254 Opened 6 years ago Closed 5 years ago

RTP packetization for H.264

Categories

(Core :: WebRTC, defect, P1)

x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla32
feature-b2g 2.0

People

(Reporter: mreavy, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Whiteboard: [p=3, s=Fx32])

Attachments

(7 files, 2 obsolete files)

Add RTP packetization for H.264 as part of adding OpenH264 support to WebRTC
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.
Assignee: nobody → ethanhugg
Blocks: 996379
Duplicate of this bug: 998220
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
Assignee: ethanhugg → rjesup
Whiteboard: [p=3, s=Fx32]
Priority: -- → P1
Target Milestone: --- → mozilla32
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.
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'
Attachment #8410454 - Attachment is obsolete: true
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 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.
Attachment #8425153 - Flags: review?(pkerr)
Attachment #8425154 - Flags: review?(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
Attachment #8425153 - Flags: review?(pkerr) → review+
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.
Attachment #8425154 - Flags: review?(pkerr) → review+
feature-b2g: --- → 2.0
Attachment #8424761 - Attachment is obsolete: true
Attachment #8427516 - Flags: review?(jolin)
Attachment #8427522 - Flags: review?(pkerr)
Attachment #8427516 - Flags: review?(jolin) → review+
Attachment #8427522 - Flags: review?(pkerr) → review+
Duplicate of this bug: 1005418
Duplicate of this bug: 996379
Duplicate of this bug: 996379
Blocks: 1036049
You need to log in before you can comment on or make changes to this bug.