Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2954503002: Implement FrameMarking header extension support

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 weeks, 1 day ago by sergio.garcia.murillo
Modified:
4 days, 13 hours ago
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, danilchap, yujie_mao (webrtc), zhuangzesen_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc(OOOtoAug12), the sun (OOO until July 24th), mflodman_OOO
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Implement FrameMarking header extension support Frame Marking RTP header extension is used to convey information about video frames that is critical for error recovery and packet forwarding in RTP middleboxes or network nodes. It is most useful when media is encrypted, and essential when the middlebox or node has no access to the media decryption keys. It is also useful for codec-agnostic processing of encrypted or unencrypted media, while it also supports extensions for codec-specific information. BUG=webrtc:7765 https://tools.ietf.org/html/draft-ietf-avtext-framemarking-04

Patch Set 1 #

Patch Set 2 : Implement Frame Marking header extension #

Total comments: 34

Patch Set 3 : Implement FrameMarking header extension and tests #

Total comments: 2

Patch Set 4 : Add VP9 SVC support and tests #

Patch Set 5 : Missing fix on temporal_layer_id #

Total comments: 8

Patch Set 6 : Changed tl0_pic_idx to int and added helper function to FrameMarking extension to know if it is SVC… #

Patch Set 7 : merge master #

Patch Set 8 : remove unneeded change in comment #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+328 lines, -2 lines) Patch
M webrtc/common_types.h View 1 2 3 4 5 6 2 chunks +46 lines, -0 lines 0 comments Download
M webrtc/common_types.cc View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
M webrtc/config.h View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
M webrtc/config.cc View 1 2 3 4 5 6 2 chunks +9 lines, -1 line 0 comments Download
M webrtc/media/engine/webrtcvideoengine.cc View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_header_extension_map.cc View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h View 1 2 3 4 5 6 1 chunk +15 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc View 1 2 3 4 5 6 1 chunk +102 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_packet.cc View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc View 1 2 3 3 chunks +69 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc View 1 2 3 4 5 6 7 3 chunks +55 lines, -0 lines 1 comment Download
M webrtc/modules/rtp_rtcp/source/rtp_utility.cc View 1 2 3 4 5 6 2 chunks +11 lines, -0 lines 0 comments Download
M webrtc/test/fuzzers/rtp_packet_fuzzer.cc View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 22 (13 generated)
stefan-webrtc
Adding nisse and danil as reviewers.
3 weeks, 4 days ago (2017-06-26 11:11:31 UTC) #5
danilchap
Can you also extend rtp_packet.cc Packet::GetHeader function and add unit tests https://codereview.webrtc.org/2954503002/diff/20001/webrtc/common_types.h File webrtc/common_types.h (right): ...
3 weeks, 2 days ago (2017-06-28 16:07:07 UTC) #7
sergio.garcia.murillo
Appart of resolving the comments, I have added the required tests to RTPPacket https://codereview.webrtc.org/2954503002/diff/20001/webrtc/common_types.h File ...
3 weeks, 1 day ago (2017-06-29 12:58:58 UTC) #8
danilchap
There is one more place that use rtp header extensions enum and thus will break ...
3 weeks, 1 day ago (2017-06-29 13:47:27 UTC) #9
sergio.garcia.murillo
https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc#newcode364 webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:364: frame_marks->temporalLayerId = (data[0] & 0x07) != 0; On 2017/06/29 ...
3 weeks ago (2017-06-30 10:09:23 UTC) #10
danilchap
code looks good to me with few more nits Can you please sign Google CLA: ...
3 weeks ago (2017-06-30 12:53:25 UTC) #11
sergio.garcia.murillo
Google CLA signed! https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc#newcode382 webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:382: frame_marks.tl0_pic_idx != static_cast<uint8_t>(kNoTl0PicIdx))) On 2017/06/30 12:53:25, ...
3 weeks ago (2017-06-30 13:42:00 UTC) #12
danilchap
lgtm
3 weeks ago (2017-06-30 13:53:02 UTC) #13
danilchap
4 days, 13 hours ago (2017-07-17 15:14:39 UTC) #22
https://codereview.webrtc.org/2954503002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2954503002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:346:
rtp_header->SetExtension<FrameMarking>(frame_marks);
[ RUN      ] VideoSendStreamTest.Vp9FlexModeRefCount
==10472==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x1ffddb0 in webrtc::FrameMarking::ValueSize(webrtc::FrameMarks const&)
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:451:10
    #1 0x20411d8 in SetExtension<webrtc::FrameMarking, webrtc::FrameMarks>
webrtc/modules/rtp_rtcp/source/rtp_packet.h:186:29
    #2 0x20411d8 in
webrtc::RTPSenderVideo::SendVideo(webrtc::RtpVideoCodecTypes, webrtc::FrameType,
signed char, unsigned int, long, unsigned char const*, unsigned long,
webrtc::RTPFragmentationHeader const*, webrtc::RTPVideoHeader const*)
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:346:0
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973