Implement FrameMarking header extension support

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

[sergio.garcia.murillo, 2017-06-22 15:17:50]

Description was changed from

==========
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
==========

to

==========
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
==========

[sergio.garcia.murillo, 2017-06-22 15:17:50]

sergio.garcia.murillo@gmail.com changed reviewers:
+ deadbeef@webrtc.org, pthatcher@google.com

[pthatcher, 2017-06-23 23:11:12]

pthatcher@google.com changed reviewers:
+ mflodman@webrtc.org
- deadbeef@webrtc.org, pthatcher@google.com

[stefan-webrtc, 2017-06-26 11:11:31]

stefan@webrtc.org changed reviewers:
+ danilchap@webrtc.org, nisse@webrtc.org, stefan@webrtc.org

[stefan-webrtc, 2017-06-26 11:11:31]

Adding nisse and danil as reviewers.

[mflodman1, 2017-06-26 11:16:45]

mflodman@google.com changed reviewers:
- mflodman@webrtc.org

[danilchap, 2017-06-28 16:07:07]

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):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/common_types.h#new...
webrtc/common_types.h:732: bool startOfFrame;
use lowercase_with_underscores style for new code
https://google.github.io/styleguide/cppguide.html#Variable_Names

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/config.h#newcode103
webrtc/config.h:103: // Frame Marking RTP Header Extension
remove two spaces

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:355: if (length==1) {
please use git cl format (to add spaces around '==')

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:364:
frame_marks->temporalLayerId = (data[0] & 0x07) != 0;
remove "!= 0"

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:382: &&
frame_marks.tl0PicIdx != (uint8_t)kNoTl0PicIdx)
use static_cast instead of c-cast,

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:397: ||
(frame_marks.temporalLayerId
is temporal_layer_id == 0 a value that shouldn't be put into extension?

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:164: static bool
Parse(const uint8_t* data,
Parse expect rtc::ArrayView<const uint8_t> as 1st parameter to be used with
rtp::Packet

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:305: // Set Frame Marks
Prefer to end comments with a '.'

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:319:
frame_marks.temporalLayerId = 0;
may be kNoTemporalIdx/kNoSpaitalIdx/kNoTl0PicIdx?

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:327:
frame_marks.spatialLayerId = 0;
kNoSpatialIdx?

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:335: // TODO: This will need
to be changed to support VP9 SVC, but videoheader
add bugnumber or username to TODO()
https://google.github.io/styleguide/cppguide.html#TODO_Comments

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:336: // is set per-frame, not
per packet, so we can't have access to this
this is same for VP8, why is it a problem? SendVideo called per layer frame, not
per super frame for VP9.
same as with VP8 all fields except start_of_frame/end_of_frame can be set in
advance, those two fields need modification per-packet anyway.

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:440: // Update start and end
marks
may be put this block inside if (frame_marking_enabled)

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:484: // For Frame Marking RTP
Header Extension:
may be reuse parsing code:
header->extension.has_frame_marks = FrameMarking::Parse(rtc::MakeArrayView(ptr,
len), &header->extension.frame_marks);

[sergio.garcia.murillo, 2017-06-29 12:58:58]

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 webrtc/common_types.h (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/common_types.h#new...
webrtc/common_types.h:732: bool startOfFrame;
On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> use lowercase_with_underscores style for new code
> https://google.github.io/styleguide/cppguide.html#Variable_Names

Done

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/config.h#newcode103
webrtc/config.h:103: // Frame Marking RTP Header Extension
On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> remove two spaces

Done.

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:355: if (length==1) {
On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> please use git cl format (to add spaces around '==')

Done.

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:364:
frame_marks->temporalLayerId = (data[0] & 0x07) != 0;
On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> remove "!= 0"

msvc gives C4800 warning: forcing value to bool 'true' or 'false' (performance
warning) if not

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:382: &&
frame_marks.tl0PicIdx != (uint8_t)kNoTl0PicIdx)
On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> use static_cast instead of c-cast, 

Done.

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:397: ||
(frame_marks.temporalLayerId
On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> is temporal_layer_id == 0 a value that shouldn't be put into extension?

No, but if temporal and spatial layer are 0 and tl0_pic_idx, then header
extension is shorter and the last 2 bytes are omitted.:

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:164: static bool
Parse(const uint8_t* data,
On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> Parse expect rtc::ArrayView<const uint8_t> as 1st parameter to be used with
> rtp::Packet

Done.

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:305: // Set Frame Marks
On 2017/06/28 16:07:07, danilchap OOO until July17 wrote:
> Prefer to end comments with a '.'

Acknowledged.

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:319:
frame_marks.temporalLayerId = 0;
On 2017/06/28 16:07:07, danilchap OOO until July17 wrote:
> may be kNoTemporalIdx/kNoSpaitalIdx/kNoTl0PicIdx?

Done.

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:327:
frame_marks.spatialLayerId = 0;
On 2017/06/28 16:07:07, danilchap OOO until July17 wrote:
> kNoSpatialIdx?

Done.

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:336: // is set per-frame, not
per packet, so we can't have access to this
On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> this is same for VP8, why is it a problem? SendVideo called per layer frame,
not
> per super frame for VP9.
> same as with VP8 all fields except start_of_frame/end_of_frame can be set in
> advance, those two fields need modification per-packet anyway.

No, because in VP8 there is no support for spatial scalability, so all packets
in the frame have same spatial, temporal and tl0picid. 

On VP9 the spatial layer id, changes between packets on the same frame.

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:440: // Update start and end
marks
On 2017/06/28 16:07:07, danilchap OOO until July17 wrote:
> may be put this block inside if (frame_marking_enabled) 

Done.

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:484: // For Frame Marking RTP
Header Extension:
On 2017/06/28 16:07:07, danilchap OOO until July17 wrote:
> may be reuse parsing code:
> header->extension.has_frame_marks =
FrameMarking::Parse(rtc::MakeArrayView(ptr,
> len), &header->extension.frame_marks);

Acknowledged.

[danilchap, 2017-06-29 13:47:27]

There is one more place that use rtp header extensions enum and thus will break
a bot with new extension:
webrtc/test/fuzzers/rtp_packet_fuzzer.cc

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:364:
frame_marks->temporalLayerId = (data[0] & 0x07) != 0;
On 2017/06/29 12:58:57, sergio.garcia.murillo wrote:
> On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> > remove "!= 0"
> 
> msvc gives C4800 warning: forcing value to bool 'true' or 'false' (performance
> warning) if not

is temporal_layer_id a boolean field?

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:382: &&
frame_marks.tl0PicIdx != (uint8_t)kNoTl0PicIdx)
On 2017/06/29 12:58:58, sergio.garcia.murillo wrote:
> On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> > use static_cast instead of c-cast, 
> 
> Done.

still c-cast in this line

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:336: // is set per-frame, not
per packet, so we can't have access to this
On 2017/06/29 12:58:58, sergio.garcia.murillo wrote:
> On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> > this is same for VP8, why is it a problem? SendVideo called per layer frame,
> not
> > per super frame for VP9.
> > same as with VP8 all fields except start_of_frame/end_of_frame can be set in
> > advance, those two fields need modification per-packet anyway.
> 
> No, because in VP8 there is no support for spatial scalability, so all packets
> in the frame have same spatial, temporal and tl0picid. 
> 
> On VP9 the spatial layer id, changes between packets on the same frame.

SendVideo takes layer frame, not super frame for VP9, i.e. all packets here use
same spatial layer id.

https://codereview.webrtc.org/2954503002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2954503002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:358:
frame_marks->base_layer_sync = 0;
may be = false

[sergio.garcia.murillo, 2017-06-30 10:09:23]

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:364:
frame_marks->temporalLayerId = (data[0] & 0x07) != 0;
On 2017/06/29 13:47:26, danilchap OOO until July17 wrote:
> On 2017/06/29 12:58:57, sergio.garcia.murillo wrote:
> > On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> > > remove "!= 0"
> > 
> > msvc gives C4800 warning: forcing value to bool 'true' or 'false'
(performance
> > warning) if not
> 
> is temporal_layer_id a boolean field?

uuuupsss!

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:382: &&
frame_marks.tl0PicIdx != (uint8_t)kNoTl0PicIdx)
On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> use static_cast instead of c-cast, 

Done. This time for sure.

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:336: // is set per-frame, not
per packet, so we can't have access to this
On 2017/06/29 13:47:26, danilchap OOO until July17 wrote:
> On 2017/06/29 12:58:58, sergio.garcia.murillo wrote:
> > On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> > > this is same for VP8, why is it a problem? SendVideo called per layer
frame,
> > not
> > > per super frame for VP9.
> > > same as with VP8 all fields except start_of_frame/end_of_frame can be set
in
> > > advance, those two fields need modification per-packet anyway.
> > 
> > No, because in VP8 there is no support for spatial scalability, so all
packets
> > in the frame have same spatial, temporal and tl0picid. 
> > 
> > On VP9 the spatial layer id, changes between packets on the same frame.
> 
> SendVideo takes layer frame, not super frame for VP9, i.e. all packets here
use
> same spatial layer id.

Perfect then.

Side note, wouldn't it still be a problem with:
     codecHeader.VP9.beginning_of_frame
and 
     codecHeader.VP9.end_of_frame values

They are per packet and not per layer frame (although not relevant for this CL)

https://codereview.webrtc.org/2954503002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2954503002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:358:
frame_marks->base_layer_sync = 0;
On 2017/06/29 13:47:26, danilchap OOO until July17 wrote:
> may be = false

Done.

[danilchap, 2017-06-30 12:53:25]

code looks good to me with few more nits

Can you please sign Google CLA: https://cla.developers.google.com/clas

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2954503002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:336: // is set per-frame, not
per packet, so we can't have access to this
On 2017/06/30 10:09:23, sergio.garcia.murillo wrote:
> On 2017/06/29 13:47:26, danilchap OOO until July17 wrote:
> > On 2017/06/29 12:58:58, sergio.garcia.murillo wrote:
> > > On 2017/06/28 16:07:06, danilchap OOO until July17 wrote:
> > > > this is same for VP8, why is it a problem? SendVideo called per layer
> frame,
> > > not
> > > > per super frame for VP9.
> > > > same as with VP8 all fields except start_of_frame/end_of_frame can be
set
> in
> > > > advance, those two fields need modification per-packet anyway.
> > > 
> > > No, because in VP8 there is no support for spatial scalability, so all
> packets
> > > in the frame have same spatial, temporal and tl0picid. 
> > > 
> > > On VP9 the spatial layer id, changes between packets on the same frame.
> > 
> > SendVideo takes layer frame, not super frame for VP9, i.e. all packets here
> use
> > same spatial layer id.
> 
> Perfect then.
> 
> Side note, wouldn't it still be a problem with:
>      codecHeader.VP9.beginning_of_frame
> and 
>      codecHeader.VP9.end_of_frame values
> 
> They are per packet and not per layer frame (although not relevant for this
CL)
> 

same struct is used for per-packet vp9 header (when parsed).
These two fields expected to be unused in this function.

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:382:
frame_marks.tl0_pic_idx != static_cast<uint8_t>(kNoTl0PicIdx)))
does it make 0xff picture id invalid?
may be instead convert tl0_pic_idx to int

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:400:
frame_marks.tl0_pic_idx != static_cast<uint8_t>(kNoTl0PicIdx))) {
same here (since this check is large and non-trivial may be reuse previous
function:
if (ValueSize(frame_marks) == 3) {
  data[0] |= ...
}

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:417: //   |  ID=2 |  L=2
 |S|E|I|D|B| TID |0|0|0|P|U| SID |    TL0PICIDX  |
linked spec has different format for VP9 LID Mapping (looking at 3.2.1.2)
Is there more recent spec?

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:168: static uint8_t
CreateLayerId(RTPVideoHeaderVP9 vp9);
probably
const RTPVideoHeaderVP9&
(that struct is large and thus not cheaply copyable or cheaply movable)

[sergio.garcia.murillo, 2017-06-30 13:42:00]

Google CLA signed!

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right):

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
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, danilchap OOO until July17 wrote:
> does it make 0xff picture id invalid?
> may be instead convert tl0_pic_idx to int

Agree, changing it to int so it matches the other code using tl0_pic_idx

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:400:
frame_marks.tl0_pic_idx != static_cast<uint8_t>(kNoTl0PicIdx))) {
On 2017/06/30 12:53:25, danilchap OOO until July17 wrote:
> same here (since this check is large and non-trivial may be reuse previous
> function:
> if (ValueSize(frame_marks) == 3) {
>   data[0] |= ...
> }

Done.

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:417: //   |  ID=2 |  L=2
 |S|E|I|D|B| TID |0|0|0|P|U| SID |    TL0PICIDX  |
On 2017/06/30 12:53:25, danilchap OOO until July17 wrote:
> linked spec has different format for VP9 LID Mapping (looking at 3.2.1.2)
> Is there more recent spec?

Current draft has invalid format for LID. Spatial id has only two bits and it
includes a quality layer id, which is not used in vp9, it also misses important
information required by an SFU in order to do the layer selection (the P and U
bits).

I have submitted a patch to the IETF group and it will be included in the next
draft (I think it will be pre-IETF 99)
https://www.ietf.org/mail-archive/web/avtext/current/msg01359.html

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right):

https://codereview.webrtc.org/2954503002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:168: static uint8_t
CreateLayerId(RTPVideoHeaderVP9 vp9);
On 2017/06/30 12:53:25, danilchap OOO until July17 wrote:
> probably
> const RTPVideoHeaderVP9&
> (that struct is large and thus not cheaply copyable or cheaply movable)

Indeed. Done.

[danilchap, 2017-06-30 13:53:02]

lgtm

[danilchap, 2017-06-30 23:51:41]

The CQ bit was checked by danilchap@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-30 23:51:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-30 23:54:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-30 23:54:22]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18809)

[danilchap, 2017-07-17 11:17:27]

The CQ bit was checked by danilchap@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-17 11:17:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-07-17 11:40:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-17 11:40:26]

Dry run: Try jobs failed on following builders:
  linux_msan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/22136)

[danilchap, 2017-07-17 15:14:39]

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

[nisse-webrtc, 2017-08-22 07:44:46]

Before adding one more codec type switch down in the low-level rtp code, I'd
like to understand if and how the codec-independent information could be
represented in a codec-independent fashion, e.g., in EncodedImage or one of its
subclasses.

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) {
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.

[sergio.garcia.murillo, 2017-08-25 10:49:39]

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.