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

Issue 2954503002: Implement FrameMarking header extension support

Created:
5 months ago by sergio.garcia.murillo
Modified:
2 months, 3 weeks 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, the sun, mflodman
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: 4

Patch Set 9 : Rebase to latest master and fix asan memory check #

Patch Set 10 : fix fuzzer #

Unified diffs Side-by-side diffs Delta from patch set Stats (+350 lines, -2 lines) Patch
M webrtc/common_types.h View 1 2 3 4 5 6 7 8 2 chunks +56 lines, -0 lines 0 comments Download
M webrtc/common_types.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
M webrtc/config.h View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
M webrtc/config.cc View 1 2 3 4 5 6 7 8 2 chunks +9 lines, -1 line 0 comments Download
M webrtc/media/engine/webrtcvideoengine.cc View 1 2 3 4 5 6 7 8 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 7 8 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 7 8 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 7 8 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 7 8 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 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc View 1 2 3 4 5 6 7 8 3 chunks +80 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc View 1 2 3 4 5 6 7 8 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 8 3 chunks +55 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_utility.cc View 1 2 3 4 5 6 7 8 2 chunks +12 lines, -0 lines 0 comments Download
M webrtc/test/fuzzers/rtp_packet_fuzzer.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
Trybot results:

Messages

Total messages: 24 (13 generated)
stefan-webrtc
Adding nisse and danil as reviewers.
4 months, 3 weeks 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): ...
4 months, 3 weeks 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 ...
4 months, 3 weeks 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 ...
4 months, 3 weeks 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 ...
4 months, 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: ...
4 months, 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, ...
4 months, 3 weeks ago (2017-06-30 13:42:00 UTC) #12
danilchap
lgtm
4 months, 3 weeks ago (2017-06-30 13:53:02 UTC) #13
danilchap
https://codereview.webrtc.org/2954503002/diff/140001/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2954503002/diff/140001/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc#newcode346 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 ...
4 months ago (2017-07-17 15:14:39 UTC) #22
nisse-webrtc
Before adding one more codec type switch down in the low-level rtp code, I'd like ...
2 months, 4 weeks ago (2017-08-22 07:44:46 UTC) #23
sergio.garcia.murillo
2 months, 3 weeks ago (2017-08-25 10:49:39 UTC) #24
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:315: switch (video_type) {
On 2017/08/22 07:44:46, nisse-webrtc wrote:
> Should we really add more codec-specific logic in this layer?
> 
> Since the information represented by frame marks are supposed to be
> codec-independent, they ought to be made available by the encoder classes in
> some way so that the information can be used without caring about which
encoder
> was used.

The information is codec specific and varies within layers of the same frame, so
it can't be added to the image information (it is only available after the rtp
packetization). This logic could be moved to a framemarking helper function
instead.

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);
On 2017/07/17 15:14:39, danilchap wrote:
> [ 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

Done. Created default constructor to initialize members.

Powered by Google App Engine
This is Rietveld efc10ee0f