Add sender controlled playout delay limits

Add sender controlled playout delay limits

This CL adds support for an extension on RTP frames to allow the sender
to specify the minimum and maximum playout delay limits.

The receiver makes a best-effort attempt to keep the capture-to-render delay
within this range. This allows different types of application to specify
different end-to-end delay goals. For example gaming can support rendering
of frames as soon as received on receiver to minimize delay. A movie playback
application can specify a minimum playout delay to allow fixed buffering
in presence of network jitter.

There are no tests at this time and most of testing is done with chromium
webrtc prototype.

On chromoting performance tests, this extension helps bring down end-to-end
delay by about 150 ms on small frames.

BUG=webrtc:5895
Committed: https://crrev.com/6b4b5f37704cb414420d813eb39b90b25f5b2d6c
Cr-Commit-Position: refs/heads/master@{#13059}

[Irfan, 2016-05-24 07:49:31]

Description was changed from

==========
Add sender controlled playout delay limits

This CL adds support for an extension on RTP frames to allow the sender
to specify the minimum and maximum playout delay limits.

The receiver makes a best-effort attempt to keep the capture-to-render delay
within this range. This allows different types of application to specify
different end-to-end delay goals. For example gaming can support rendering
of frames as soon as received on receiver to minimize delay. A movie playback
application can specify a minimum playout delay to allow fixed buffering
in presence of network jitter.

There are no tests at this time and most of testing is done with chromium
webrtc prototype.

On chromoting performance tests, this extension helps bring down end-to-end
delay by about 150 ms on small frames.

BUG=webrtc:5895
==========

to

==========
Add sender controlled playout delay limits

This CL adds support for an extension on RTP frames to allow the sender
to specify the minimum and maximum playout delay limits.

The receiver makes a best-effort attempt to keep the capture-to-render delay
within this range. This allows different types of application to specify
different end-to-end delay goals. For example gaming can support rendering
of frames as soon as received on receiver to minimize delay. A movie playback
application can specify a minimum playout delay to allow fixed buffering
in presence of network jitter.

There are no tests at this time and most of testing is done with chromium
webrtc prototype.

On chromoting performance tests, this extension helps bring down end-to-end
delay by about 150 ms on small frames.

BUG=webrtc:5895
==========

[Irfan, 2016-05-24 07:49:31]

isheriff@chromium.org changed reviewers:
+ danilchap@webrtc.org, sprang@google.com, stefan@webrtc.org

[Irfan, 2016-05-24 07:50:36]

isheriff@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Irfan, 2016-05-24 07:51:52]

Please refer to this doc for the CL:

https://docs.google.com/document/d/1i8zTPob7ygaOPjiOujGVq9ws-YvjdhGmsSQU7C7it...

I have largely done testing on chromium webrtc prototype (please refer to commit
for results). Note that there are no tests yet. I plan to add tests once I get
first round of feedback from folks.

[Irfan, 2016-05-24 07:53:09]

Danil, I have not updated rtp_packet in this CL to keep it focussed. I can add
necessary changes in a follow up CL.

[danilchap, 2016-05-24 13:27:43]

On 2016/05/24 07:53:09, Irfan wrote:
> Danil, I have not updated rtp_packet in this CL to keep it focussed. I can add
> necessary changes in a follow up CL.

Sure, I'll update it in CL that would start use RtpPacket, to keep each CL
focused on own topic.
(though I do not expect RtpPacket would require any changes, but instead in
rtp_header_extensions or in a new file there will be one more class)

[danilchap, 2016-05-24 13:28:25]

https://codereview.webrtc.org/2007743003/diff/1/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.webrtc.org/2007743003/diff/1/webrtc/config.cc#newcode58
webrtc/config.cc:58: const int RtpExtension::kPlayoutDelayId = 6;
DefaultId

https://codereview.webrtc.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h (right):

https://codereview.webrtc.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:14: #include <stdint.h>
#include "webrtc/base/basictypes.h" instead of <stdint.h> directly

https://codereview.webrtc.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:40: bool
ShouldIncludePlayoutDelayExtension(int ssrc) const;
We use uint32_t for ssrcs (it is always 32bits)

https://codereview.webrtc.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:56:
std::unordered_map<uint32_t, uint32_t> ssrc_to_high_seq_num_;
PlayoutDelayOracle lives inside RtpSender that is responsible for single remote
point and has two ssrcs: one for normal packets, another - for rtx channel.
It feels like you do not need maps or ssrc in this class. rtcp receiver should
filter out unrelated ssrc when assembling receiver report list.

https://codereview.webrtc.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:529: sequence_number_);
sequence_number_ here is 16 bit, i.e. wrapped, but playout_delay_oracle expects
it to be unwrapped 32bit.
Probably need to update Oracle to support 16bit sequence number and compare them
there with IsNewerSequenceNumber function from
modules/include/module_common_types.h

https://codereview.webrtc.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1719: RTC_DCHECK(length ==
kPlayoutDelayLength);
RTC_DCHECK_EQ

https://codereview.webrtc.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:427:
header->extension.min_playout_delay_ms = min_playout_delay * 10;
kPlayoutDelayGranularityMs instead of 10 here and next line

[sprang_webrtc, 2016-05-24 14:46:07]

sprang@webrtc.org changed reviewers:
+ sprang@webrtc.org

[sprang_webrtc, 2016-05-24 14:46:08]

https://codereview.chromium.org/2007743003/diff/1/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/common_types.h#newco...
webrtc/common_types.h:779: int16_t max_playout_delay_ms;
Maybe you can have a small separate struct PlayoutDelay, with a min and max
member. They seem to be always treated as an entity so it should simplify things
in some places.

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc:26:
send_playout_delay_ssrc_.at(ssrc);
Store iterator and dereference if != end(), rather than doing the lookup twice.

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:56:
std::unordered_map<uint32_t, uint32_t> ssrc_to_high_seq_num_;
On 2016/05/24 13:28:24, danilchap wrote:
> PlayoutDelayOracle lives inside RtpSender that is responsible for single
remote
> point and has two ssrcs: one for normal packets, another - for rtx channel.
> It feels like you do not need maps or ssrc in this class. rtcp receiver should
> filter out unrelated ssrc when assembling receiver report list.

+1 to not have a map here if not necessary. Otherwise please have map from ssrc
to a struct of the context you need, rather than having four maps keyed the same
way.

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:529: sequence_number_);
On 2016/05/24 13:28:24, danilchap wrote:
> sequence_number_ here is 16 bit, i.e. wrapped, but playout_delay_oracle
expects
> it to be unwrapped 32bit.
> Probably need to update Oracle to support 16bit sequence number and compare
them
> there with IsNewerSequenceNumber function from
> modules/include/module_common_types.h

Or use SequenceNumberUnwrapper from module_common_types.h

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:178: bool marker_bit) const;
RtpSender should already know which ssrc is used?

[Irfan, 2016-05-25 09:31:06]

Patchset #2 (id:20001) has been deleted

[Irfan, 2016-05-25 09:32:53]

PS#2 addresses initial comments. I also fix a few things from testing under
webrtc repo. PS#1 had purely been testing from remoting on chromium.

PTAL.

https://codereview.chromium.org/2007743003/diff/1/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/common_types.h#newco...
webrtc/common_types.h:779: int16_t max_playout_delay_ms;
On 2016/05/24 14:46:08, språng wrote:
> Maybe you can have a small separate struct PlayoutDelay, with a min and max
> member. They seem to be always treated as an entity so it should simplify
things
> in some places.

Done.

https://codereview.chromium.org/2007743003/diff/1/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/config.cc#newcode58
webrtc/config.cc:58: const int RtpExtension::kPlayoutDelayId = 6;
On 2016/05/24 13:28:24, danilchap wrote:
> DefaultId

Done.

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc:26:
send_playout_delay_ssrc_.at(ssrc);
On 2016/05/24 14:46:08, språng wrote:
> Store iterator and dereference if != end(), rather than doing the lookup
twice.

I removed the map

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:14: #include <stdint.h>
On 2016/05/24 13:28:24, danilchap wrote:
> #include "webrtc/base/basictypes.h" instead of <stdint.h> directly

Done.

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:40: bool
ShouldIncludePlayoutDelayExtension(int ssrc) const;
On 2016/05/24 13:28:24, danilchap wrote:
> We use uint32_t for ssrcs (it is always 32bits)

Done.

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:40: bool
ShouldIncludePlayoutDelayExtension(int ssrc) const;
On 2016/05/24 13:28:24, danilchap wrote:
> We use uint32_t for ssrcs (it is always 32bits)

Done.

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:56:
std::unordered_map<uint32_t, uint32_t> ssrc_to_high_seq_num_;
On 2016/05/24 14:46:08, språng wrote:
> On 2016/05/24 13:28:24, danilchap wrote:
> > PlayoutDelayOracle lives inside RtpSender that is responsible for single
> remote
> > point and has two ssrcs: one for normal packets, another - for rtx channel.
> > It feels like you do not need maps or ssrc in this class. rtcp receiver
should
> > filter out unrelated ssrc when assembling receiver report list.
> 
> +1 to not have a map here if not necessary. Otherwise please have map from
ssrc
> to a struct of the context you need, rather than having four maps keyed the
same
> way.

Thanks for pointing this out Danil. Makes it much simpler.

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:529: sequence_number_);
On 2016/05/24 14:46:08, språng wrote:
> On 2016/05/24 13:28:24, danilchap wrote:
> > sequence_number_ here is 16 bit, i.e. wrapped, but playout_delay_oracle
> expects
> > it to be unwrapped 32bit.
> > Probably need to update Oracle to support 16bit sequence number and compare
> them
> > there with IsNewerSequenceNumber function from
> > modules/include/module_common_types.h
> 
> Or use SequenceNumberUnwrapper from module_common_types.h

good catch. done.

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1719: RTC_DCHECK(length ==
kPlayoutDelayLength);
On 2016/05/24 13:28:24, danilchap wrote:
> RTC_DCHECK_EQ

replaced with assert

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:178: bool marker_bit) const;
On 2016/05/24 14:46:08, språng wrote:
> RtpSender should already know which ssrc is used?

Removed

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:427:
header->extension.min_playout_delay_ms = min_playout_delay * 10;
On 2016/05/24 13:28:24, danilchap wrote:
> kPlayoutDelayGranularityMs instead of 10 here and next line

Done.

[Irfan, 2016-05-25 09:37:57]

Patchset #2 (id:40001) has been deleted

[hlundin-webrtc, 2016-05-25 10:58:45]

On 2016/05/25 09:32:53, Irfan wrote:
> PS#2 addresses initial comments. I also fix a few things from testing under
> webrtc repo. PS#1 had purely been testing from remoting on chromium.
> 
> PTAL.
> 
> https://codereview.chromium.org/2007743003/diff/1/webrtc/common_types.h
> File webrtc/common_types.h (right):
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/common_types.h#newco...
> webrtc/common_types.h:779: int16_t max_playout_delay_ms;
> On 2016/05/24 14:46:08, språng wrote:
> > Maybe you can have a small separate struct PlayoutDelay, with a min and max
> > member. They seem to be always treated as an entity so it should simplify
> things
> > in some places.
> 
> Done.
> 
> https://codereview.chromium.org/2007743003/diff/1/webrtc/config.cc
> File webrtc/config.cc (right):
> 
> https://codereview.chromium.org/2007743003/diff/1/webrtc/config.cc#newcode58
> webrtc/config.cc:58: const int RtpExtension::kPlayoutDelayId = 6;
> On 2016/05/24 13:28:24, danilchap wrote:
> > DefaultId
> 
> Done.
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc (right):
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc:26:
> send_playout_delay_ssrc_.at(ssrc);
> On 2016/05/24 14:46:08, språng wrote:
> > Store iterator and dereference if != end(), rather than doing the lookup
> twice.
> 
> I removed the map
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h (right):
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:14: #include <stdint.h>
> On 2016/05/24 13:28:24, danilchap wrote:
> > #include "webrtc/base/basictypes.h" instead of <stdint.h> directly
> 
> Done.
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:40: bool
> ShouldIncludePlayoutDelayExtension(int ssrc) const;
> On 2016/05/24 13:28:24, danilchap wrote:
> > We use uint32_t for ssrcs (it is always 32bits)
> 
> Done.
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:40: bool
> ShouldIncludePlayoutDelayExtension(int ssrc) const;
> On 2016/05/24 13:28:24, danilchap wrote:
> > We use uint32_t for ssrcs (it is always 32bits)
> 
> Done.
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:56:
> std::unordered_map<uint32_t, uint32_t> ssrc_to_high_seq_num_;
> On 2016/05/24 14:46:08, språng wrote:
> > On 2016/05/24 13:28:24, danilchap wrote:
> > > PlayoutDelayOracle lives inside RtpSender that is responsible for single
> > remote
> > > point and has two ssrcs: one for normal packets, another - for rtx
channel.
> > > It feels like you do not need maps or ssrc in this class. rtcp receiver
> should
> > > filter out unrelated ssrc when assembling receiver report list.
> > 
> > +1 to not have a map here if not necessary. Otherwise please have map from
> ssrc
> > to a struct of the context you need, rather than having four maps keyed the
> same
> > way.
> 
> Thanks for pointing this out Danil. Makes it much simpler.
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> webrtc/modules/rtp_rtcp/source/rtp_sender.cc:529: sequence_number_);
> On 2016/05/24 14:46:08, språng wrote:
> > On 2016/05/24 13:28:24, danilchap wrote:
> > > sequence_number_ here is 16 bit, i.e. wrapped, but playout_delay_oracle
> > expects
> > > it to be unwrapped 32bit.
> > > Probably need to update Oracle to support 16bit sequence number and
compare
> > them
> > > there with IsNewerSequenceNumber function from
> > > modules/include/module_common_types.h
> > 
> > Or use SequenceNumberUnwrapper from module_common_types.h
> 
> good catch. done.
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1719: RTC_DCHECK(length ==
> kPlayoutDelayLength);
> On 2016/05/24 13:28:24, danilchap wrote:
> > RTC_DCHECK_EQ
> 
> replaced with assert
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> webrtc/modules/rtp_rtcp/source/rtp_sender.h:178: bool marker_bit) const;
> On 2016/05/24 14:46:08, språng wrote:
> > RtpSender should already know which ssrc is used?
> 
> Removed
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):
> 
>
https://codereview.chromium.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sou...
> webrtc/modules/rtp_rtcp/source/rtp_utility.cc:427:
> header->extension.min_playout_delay_ms = min_playout_delay * 10;
> On 2016/05/24 13:28:24, danilchap wrote:
> > kPlayoutDelayGranularityMs instead of 10 here and next line
> 
> Done.

I see some changes being made to the audio side as well in this CL. But it seems
to have no real effect on the audio playout. Can you avoid any changes to the
audio side in this CL, or are they needed for the signaling?

[danilchap, 2016-05-25 19:08:49]

https://codereview.webrtc.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2007743003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1719: RTC_DCHECK(length ==
kPlayoutDelayLength);
On 2016/05/25 09:32:53, Irfan wrote:
> On 2016/05/24 13:28:24, danilchap wrote:
> > RTC_DCHECK_EQ
> 
> replaced with assert

Acknowledged.

https://codereview.webrtc.org/2007743003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc (right):

https://codereview.webrtc.org/2007743003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc:16: #include
"webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
includes in aplhabetical order:
include < source

https://codereview.webrtc.org/2007743003/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.webrtc.org/2007743003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:227: void
OnReceivedRtcpReceiverReport(const ReportBlockList& report_blocks);
probably better move this declaration out of group of methods related to NACK
(StorePackets/Resend - those are too related to nacks)

https://codereview.webrtc.org/2007743003/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:476: PlayoutDelayOracle
playout_delay_oracle_;
GUARDED_BY(send_critsect_) ?
(since it could be accessed on worker thread with OnReceivedReceiverReport and
on encoder thread with UpdateRequest/send_playout_delay)

[Irfan, 2016-05-26 05:51:48]

I went ahead and removed the capability support that existed for audio code.

It makes sense to add it as part of CL to do playout limits for audio.

https://codereview.chromium.org/2007743003/diff/60001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc (right):

https://codereview.chromium.org/2007743003/diff/60001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc:16: #include
"webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
On 2016/05/25 19:08:48, danilchap wrote:
> includes in aplhabetical order:
> include < source

Done.

https://codereview.chromium.org/2007743003/diff/60001/webrtc/modules/rtp_rtcp...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.chromium.org/2007743003/diff/60001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:227: void
OnReceivedRtcpReceiverReport(const ReportBlockList& report_blocks);
On 2016/05/25 19:08:48, danilchap wrote:
> probably better move this declaration out of group of methods related to NACK
> (StorePackets/Resend - those are too related to nacks)

done. the original grouping here was for rtcp feedback.

https://codereview.chromium.org/2007743003/diff/60001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:476: PlayoutDelayOracle
playout_delay_oracle_;
On 2016/05/25 19:08:49, danilchap wrote:
> GUARDED_BY(send_critsect_) ?
> (since it could be accessed on worker thread with OnReceivedReceiverReport and
> on encoder thread with UpdateRequest/send_playout_delay)

I moved the protection into the class - keeps it easier to understand and
document

[Irfan, 2016-05-26 05:57:36]

Patchset #3 (id:80001) has been deleted

[Irfan, 2016-05-26 05:58:24]

PS#3 removes adding playout delay capability to audio and addresses Danil
comments. PTAL.

[Irfan, 2016-05-26 06:06:06]

Patchset #3 (id:100001) has been deleted

[danilchap, 2016-05-26 09:41:58]

rtp part (modules/rtp_rtcp) lgtm

https://codereview.chromium.org/2007743003/diff/120001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/common_types.h#...
webrtc/common_types.h:764: struct PlayoutDelay {
May be you want to add a default constructor and initialize values to -1/-1
Currently explicit default constructor of RTPHeaderExtension leaves this two
values uninitialized.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc:21: :
high_sequence_number_(0),
send_playout_delay_ initialization is missing

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:72: // Data in this
section is accessed on the sending/encoder thread alone.
this is possible to check (see ThreadChecker) and annotate too, if you like.

[stefan-webrtc, 2016-05-28 05:01:45]

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h (right):

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:72: // Data in this
section is accessed on the sending/encoder thread alone.
On 2016/05/26 09:41:58, danilchap wrote:
> this is possible to check (see ThreadChecker) and annotate too, if you like.

I think using a thread checker would be a good idea.

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_header_extension.h (right):

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extension.h:36: const int
kPlayoutDelayMaxMs = 40950;
Initialize this to:
kPlayoutDelayGranularityMs * (1<<(kPlayoutDelayLength - 1))

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:529: if (rtp_hdr)
Add {} since this is a multi-line if (>2 lines).

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:832: rtc::CritScope
lock(&send_critsect_);
Do you really have to lock this? Had you not already protected the internals of
the oracle?

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1049: if
(playout_delay_oracle_.send_playout_delay())
{}

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1052:
playout_delay_oracle_.max_playout_delay_ms());
Are you sure you have to update here? Can the values change after you did Build?

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1477:
RTC_DCHECK_LE(max_playout_delay_ms, kPlayoutDelayMaxMs);
Maybe also check that min <= max?

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1497: data_buffer[pos++] =
max_playout & 0xff;
I think you can use ByteWriter for this?

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:426: uint16_t max_playout_delay =
((ptr[1] & 0xf) << 8) | ptr[2];
I guess these can be ints instead? No point making them uint16_t AFAICT.

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/frame_buffer.cc (right):

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/frame_buffer.cc:161: playout_delay =
packet.video_header.playout_delay;
Should you have a "has_playout_delay" field instead? Or maybe make playout_delay
Optional? Seems better than to assume it's set on the first packet?

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/timing.h (right):

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/timing.h:45: void set_min_playout_delay(uint32_t
min_playout_delay_ms);
I'd prefer these to all be ints instead since it doesn't matter whether they're
uint32_t or int.

[Sergey Ulanov, 2016-05-30 07:22:56]

https://codereview.chromium.org/2007743003/diff/120001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/common_types.h#...
webrtc/common_types.h:764: struct PlayoutDelay {
On 2016/05/26 09:41:58, danilchap wrote:
> May be you want to add a default constructor and initialize values to -1/-1
> Currently explicit default constructor of RTPHeaderExtension leaves this two
> values uninitialized.

Or just add default initalizers '= -1' in the field declarations.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/video_frame.h#n...
webrtc/video_frame.h:186: PlayoutDelay playout_delay;
add _ suffix.

[sprang, 2016-05-30 12:13:15]

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_header_extension.h (right):

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_header_extension.h:36: const int
kPlayoutDelayMaxMs = 40950;
On 2016/05/28 05:01:44, stefan-webrtc (holmer) wrote:
> Initialize this to:
> kPlayoutDelayGranularityMs * (1<<(kPlayoutDelayLength - 1))
Hm? kPlayoutDelayLength is the byte length of the whole extension (4 bytes),
right?

And even if kPlayoutDelayLength was 12, shouldn't that be
kPlayoutDelayGranularityMs * ((1 << kPlayoutDelayLength) - 1) ?

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2007743003/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1497: data_buffer[pos++] =
max_playout & 0xff;
On 2016/05/28 05:01:44, stefan-webrtc (holmer) wrote:
> I think you can use ByteWriter for this?

That's mostly useful for integers with full byte sizes, not 12 bit ones...
Maybe rtc::BitBufferWriter could be used though?

[Irfan, 2016-06-01 08:36:34]

Patchset #4 (id:140001) has been deleted

[Irfan, 2016-06-01 08:36:49]

Patchset #4 (id:160001) has been deleted

[Irfan, 2016-06-01 08:38:34]

https://codereview.chromium.org/2007743003/diff/120001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/common_types.h#...
webrtc/common_types.h:764: struct PlayoutDelay {
On 2016/05/30 07:22:56, Sergey Ulanov wrote:
> On 2016/05/26 09:41:58, danilchap wrote:
> > May be you want to add a default constructor and initialize values to -1/-1
> > Currently explicit default constructor of RTPHeaderExtension leaves this two
> > values uninitialized.
> 
> Or just add default initalizers '= -1' in the field declarations.

I have added a comment here, but this eventually ends up in a union that gets
created and used in places.  We have to let the owner control initialization.

http://www.stroustrup.com/C++11FAQ.html#unions

I think RTP header structures deserve a clean up moving away from a union.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc:21: :
high_sequence_number_(0),
On 2016/05/26 09:41:58, danilchap wrote:
> send_playout_delay_ initialization is missing

Done.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:72: // Data in this
section is accessed on the sending/encoder thread alone.
On 2016/05/26 09:41:58, danilchap wrote:
> this is possible to check (see ThreadChecker) and annotate too, if you like.

Done.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtp_header_extension.h (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_header_extension.h:36: const int
kPlayoutDelayMaxMs = 40950;
On 2016/05/30 12:13:15, sprang wrote:
> On 2016/05/28 05:01:44, stefan-webrtc (holmer) wrote:
> > Initialize this to:
> > kPlayoutDelayGranularityMs * (1<<(kPlayoutDelayLength - 1))
> Hm? kPlayoutDelayLength is the byte length of the whole extension (4 bytes),
> right?
> 
> And even if kPlayoutDelayLength was 12, shouldn't that be
> kPlayoutDelayGranularityMs * ((1 << kPlayoutDelayLength) - 1) ?

This is kPlayoutDelayGranularityMs * ( (1 << ((kPlayoutDelayLength - 1)  * 8/
2))  - 1). Am going to leave this as 40950 with the comment since that seems
more clear to me :-)

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:529: if (rtp_hdr)
On 2016/05/28 05:01:44, stefan-webrtc (holmer) wrote:
> Add {} since this is a multi-line if (>2 lines).

Done.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:832: rtc::CritScope
lock(&send_critsect_);
On 2016/05/28 05:01:44, stefan-webrtc (holmer) wrote:
> Do you really have to lock this? Had you not already protected the internals
of
> the oracle?

Good catch. Fixed.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1049: if
(playout_delay_oracle_.send_playout_delay())
On 2016/05/28 05:01:44, stefan-webrtc (holmer) wrote:
> {}

Done.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1052:
playout_delay_oracle_.max_playout_delay_ms());
On 2016/05/28 05:01:44, stefan-webrtc (holmer) wrote:
> Are you sure you have to update here? Can the values change after you did
Build?

This is indeed redundant. removed.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1477:
RTC_DCHECK_LE(max_playout_delay_ms, kPlayoutDelayMaxMs);
On 2016/05/28 05:01:44, stefan-webrtc (holmer) wrote:
> Maybe also check that min <= max?

Done.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1497: data_buffer[pos++] =
max_playout & 0xff;
On 2016/05/30 12:13:15, sprang wrote:
> On 2016/05/28 05:01:44, stefan-webrtc (holmer) wrote:
> > I think you can use ByteWriter for this?
> 
> That's mostly useful for integers with full byte sizes, not 12 bit ones...
> Maybe rtc::BitBufferWriter could be used though?

The current code is explicit about how bytes are filled up alongside the
illustration. Not sure the overhead of BitBufferWriter helps. Let me know if you
feel strongly.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:426: uint16_t max_playout_delay =
((ptr[1] & 0xf) << 8) | ptr[2];
On 2016/05/28 05:01:44, stefan-webrtc (holmer) wrote:
> I guess these can be ints instead? No point making them uint16_t AFAICT.

Done.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/video_c...
File webrtc/modules/video_coding/frame_buffer.cc (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/video_c...
webrtc/modules/video_coding/frame_buffer.cc:161: playout_delay =
packet.video_header.playout_delay;
On 2016/05/28 05:01:45, stefan-webrtc (holmer) wrote:
> Should you have a "has_playout_delay" field instead? Or maybe make
playout_delay
> Optional? Seems better than to assume it's set on the first packet?

The values of -1 are treated as play out delay being invalid/absent on packet.
This is clarified in the parsing as well as where we define play out delay.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/video_c...
File webrtc/modules/video_coding/timing.h (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/modules/video_c...
webrtc/modules/video_coding/timing.h:45: void set_min_playout_delay(uint32_t
min_playout_delay_ms);
On 2016/05/28 05:01:45, stefan-webrtc (holmer) wrote:
> I'd prefer these to all be ints instead since it doesn't matter whether
they're
> uint32_t or int.

I cleaned up all the delay variables here alongside so that we can get rid of a
bunch of casts that were being done.

https://codereview.chromium.org/2007743003/diff/120001/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.chromium.org/2007743003/diff/120001/webrtc/video_frame.h#n...
webrtc/video_frame.h:186: PlayoutDelay playout_delay;
On 2016/05/30 07:22:56, Sergey Ulanov wrote:
> add _ suffix.

I went with your suggestion - this class needs a clean up throughout.

[Irfan, 2016-06-01 08:39:01]

PS#4 addresses latest comments. PTAL.

[Irfan, 2016-06-01 20:23:43]

Patchset #5 (id:200001) has been deleted

[Irfan, 2016-06-01 20:23:56]

Patchset #5 (id:220001) has been deleted

[Irfan, 2016-06-02 06:37:20]

Patchset #5 (id:240001) has been deleted

[Irfan, 2016-06-02 06:46:44]

PS#5 addresses test failures. 

One particular issue that was interesting was that my change to build RTP header
first and then add the payload exposed an issue with VP9 where the last packet
marker is not always there (with spatial layers it appears) and the change was
basically causing sequence number to increment (from building the RTP header)
and then skipped. I have now addressed this by going to the previous order of
building the packet (payload first and then header) and adding a step to compute
the header length dynamically. 

I have a feeling this pattern can be used to improve behavior of not having
redundant extension always in other cases (like video rotation).

PTAL

[Stefan, 2016-06-02 08:07:08]

holmer@chromium.org changed reviewers:
+ holmer@chromium.org

[Stefan, 2016-06-02 08:07:09]

Looks OK to me, but it would be good to get Danil's eyes on these latest changes
too.

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1335: if
(rtp_header_extension_map_.GetId(kRtpExtensionTransmissionTimeOffset,
Would have been nice with a method which simply checked if the extension was
registered and not reuse GetId for that.

[danilchap, 2016-06-02 12:12:34]

Nice catch about potential sequence number gap! (I had a plan to make that
mistake too.)

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc (right):

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc:34:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
RTC_DCHECK_RUN_ON(&thread_checker_);
(just advertising my new macros)

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h (right):

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:82:
SequenceNumberUnwrapper unwrapper_;
SequenceNumberUnwrapper unwrapper_ ACCESS_ON(thread_checker_);  (would require
use thread_checker differently in .cc file (see comment there), but would
guarantee it is not forgotten)
(just advertising my new macros)

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc (right):

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc:19: int kSsrc =
100;
since they are constants, clearer mark them with constexpr

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1324: size_t
RTPSender::RtpHeaderExtensionCurrentLength() const {
this function sums size of all extensions, including the inactive, while
MaxLength skips inactive extension.
so RtpHeaderCurrentLength may be > RtpHeaderMaxLength
that is confusing.

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1335: if
(rtp_header_extension_map_.GetId(kRtpExtensionTransmissionTimeOffset,
On 2016/06/02 08:07:09, Stefan wrote:
> Would have been nice with a method which simply checked if the extension was
> registered and not reuse GetId for that.

rtp_header_extension_map_ has method IsRegistered for that.

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1353: if
(rtp_header_extension_map_.GetId(kRtpExtensionVideoRotation, &id) ==
this means video rotation extension would be counted even when inactive.
This way it might happen RtpHeaderCurrentLength > RtpHeaderMaxLength

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1372: default:
do not use default, this way compiler would warn when new value added to the
enum.

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1380: size_t padding_bytes =
RtpUtility::Word32Align(header_length) - header_length;
this three lines could be replaced with header_length =
RtpUtility::Word32Align(header_length);
or may be better to just
return RtpUtility::Word32Align(header_length) + kRtpOneByteHeaderLength;

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:229: videoType,
_rtpSender.MaxDataPayloadLength(),
Please note that packetizer use this value to decide how much bytes it can put
per packet. Current design doesn't allow to change this value mid-frame.
So there is not much point to put a lot of effort into reducing space used by
rtp header extensions during this function.

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:278:
_rtpSender.BuildRTPheader(
playout_delay_oracle_ is not protected by crit_
That means that extension might be changed after payload is filled, but before
this call to BuildRTPheader. BuildRTPheader then will use less bytes which would
calculate payload offset incorrectly.

May be would be clearer to protect playout_delay_oracle with crit_,
or set extension Active/Inactive, so that less mechanics could be used for
optional extensions.

Either way ensure rtp_header_length is unchanged during this function.


Do not try to be too nice about this code: I plan to change this code with
RtpPacket shortly (and probably replace inactive extension concept with
something less confusing).

[Irfan, 2016-06-02 18:15:53]

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc (right):

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.cc:34:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
On 2016/06/02 12:12:34, danilchap wrote:
> RTC_DCHECK_RUN_ON(&thread_checker_);
> (just advertising my new macros)

Done.

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h (right):

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle.h:82:
SequenceNumberUnwrapper unwrapper_;
On 2016/06/02 12:12:34, danilchap wrote:
> SequenceNumberUnwrapper unwrapper_ ACCESS_ON(thread_checker_);  (would require
> use thread_checker differently in .cc file (see comment there), but would
> guarantee it is not forgotten)
> (just advertising my new macros)

Done.

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc (right):

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc:19: int kSsrc =
100;
On 2016/06/02 12:12:34, danilchap wrote:
> since they are constants, clearer mark them with constexpr

Done.

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1324: size_t
RTPSender::RtpHeaderExtensionCurrentLength() const {
On 2016/06/02 12:12:34, danilchap wrote:
> this function sums size of all extensions, including the inactive, while
> MaxLength skips inactive extension.
> so RtpHeaderCurrentLength may be > RtpHeaderMaxLength
> that is confusing.

I did not realize TotalLength was already taking into consideration whether an
extension is active or inactive.

I have now removed the notion of "current length" since it makes sense to let
playout delay also be treated as active or inactive. This brings the behavior
along the lines of video rotation with the only difference being video rotation
only gets active once while playout delay can be inactive again.

At some point, it makes sense to let video rotation also be inactive when
unneeded to avoid sending unnecessary data.

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1335: if
(rtp_header_extension_map_.GetId(kRtpExtensionTransmissionTimeOffset,
On 2016/06/02 12:12:34, danilchap wrote:
> On 2016/06/02 08:07:09, Stefan wrote:
> > Would have been nice with a method which simply checked if the extension was
> > registered and not reuse GetId for that.
> 
> rtp_header_extension_map_ has method IsRegistered for that.

This is now removed in favour of just treating playout delay extension as
active/inactive

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1353: if
(rtp_header_extension_map_.GetId(kRtpExtensionVideoRotation, &id) ==
On 2016/06/02 12:12:34, danilchap wrote:
> this means video rotation extension would be counted even when inactive.
> This way it might happen RtpHeaderCurrentLength > RtpHeaderMaxLength

see above

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1372: default:
On 2016/06/02 12:12:34, danilchap wrote:
> do not use default, this way compiler would warn when new value added to the
> enum.

removed

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1380: size_t padding_bytes =
RtpUtility::Word32Align(header_length) - header_length;
On 2016/06/02 12:12:34, danilchap wrote:
> this three lines could be replaced with header_length =
> RtpUtility::Word32Align(header_length);
> or may be better to just
> return RtpUtility::Word32Align(header_length) + kRtpOneByteHeaderLength;

removed

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:229: videoType,
_rtpSender.MaxDataPayloadLength(),
On 2016/06/02 12:12:34, danilchap wrote:
> Please note that packetizer use this value to decide how much bytes it can put
> per packet. Current design doesn't allow to change this value mid-frame.
> So there is not much point to put a lot of effort into reducing space used by
> rtp header extensions during this function.

Right, with my updated change the length is treated as fixed.

https://codereview.chromium.org/2007743003/diff/260001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:278:
_rtpSender.BuildRTPheader(
On 2016/06/02 12:12:34, danilchap wrote:
> playout_delay_oracle_ is not protected by crit_
> That means that extension might be changed after payload is filled, but before
> this call to BuildRTPheader. BuildRTPheader then will use less bytes which
would
> calculate payload offset incorrectly.
> 
> May be would be clearer to protect playout_delay_oracle with crit_,
> or set extension Active/Inactive, so that less mechanics could be used for
> optional extensions.
> 
> Either way ensure rtp_header_length is unchanged during this function.
> 
> 
> Do not try to be too nice about this code: I plan to change this code with
> RtpPacket shortly (and probably replace inactive extension concept with
> something less confusing).

Thanks for pointing this out. With the updated change the playout delay
extension can get active or inactive only before each call to this method, so
this way we do not have the issue of it changing while preparing the header.

[Irfan, 2016-06-02 18:17:48]

PS#6 still needs to pass tests, but is now updated to address Danil's comments.

[Irfan, 2016-06-02 18:32:01]

Patchset #6 (id:280001) has been deleted

[danilchap, 2016-06-03 09:01:37]

rtp part (modules/rtp_rtcp) lgtm again % nit

https://codereview.webrtc.org/2007743003/diff/300001/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.webrtc.org/2007743003/diff/300001/webrtc/config.cc#newcode64
webrtc/config.cc:64: uri == webrtc::RtpExtension::kPlayoutDelayUri;
for now this extension is not used for Audio, may be remove it here.

https://codereview.webrtc.org/2007743003/diff/300001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc (right):

https://codereview.webrtc.org/2007743003/diff/300001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc:42:
EXPECT_EQ(playout_delay_oracle_.max_playout_delay_ms(), -1);
this is same line as next one

https://codereview.webrtc.org/2007743003/diff/300001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2007743003/diff/300001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1357:
_rtpRtcp.OnReceivedRtcpReceiverReport(
though _cbRtcpBandwidthObserver is rarely nullptr, still might be better to move
this callback outside if (_cbRtcpBandwidthObserver) block

https://codereview.webrtc.org/2007743003/diff/300001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2007743003/diff/300001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:269: // Note that RTP header
size is dynamically computed since there may be
probably should be removed now.

[Irfan, 2016-06-03 15:55:33]

https://codereview.chromium.org/2007743003/diff/300001/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.chromium.org/2007743003/diff/300001/webrtc/config.cc#newco...
webrtc/config.cc:64: uri == webrtc::RtpExtension::kPlayoutDelayUri;
On 2016/06/03 09:01:37, danilchap wrote:
> for now this extension is not used for Audio, may be remove it here.

Done.

https://codereview.chromium.org/2007743003/diff/300001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc (right):

https://codereview.chromium.org/2007743003/diff/300001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/playout_delay_oracle_unittest.cc:42:
EXPECT_EQ(playout_delay_oracle_.max_playout_delay_ms(), -1);
On 2016/06/03 09:01:37, danilchap wrote:
> this is same line as next one

Done.

https://codereview.chromium.org/2007743003/diff/300001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.chromium.org/2007743003/diff/300001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1357:
_rtpRtcp.OnReceivedRtcpReceiverReport(
On 2016/06/03 09:01:37, danilchap wrote:
> though _cbRtcpBandwidthObserver is rarely nullptr, still might be better to
move
> this callback outside if (_cbRtcpBandwidthObserver) block

Done.

https://codereview.chromium.org/2007743003/diff/300001/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.chromium.org/2007743003/diff/300001/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:269: // Note that RTP header
size is dynamically computed since there may be
On 2016/06/03 09:01:37, danilchap wrote:
> probably should be removed now.

Done.

[Irfan, 2016-06-03 15:58:27]

Patchset #7 (id:320001) has been deleted

[Irfan, 2016-06-03 15:59:07]

PS#7 addressed Danil's nits. Can others please take a look for lgtm ?

[Irfan, 2016-06-03 16:19:52]

Patchset #7 (id:340001) has been deleted

[stefan-webrtc, 2016-06-06 14:08:16]

In a follow-up I'd be happy if you also looked into adding this to our new
jitter buffer implementation (see packet_buffer.h/.cc and frame_buffer2.h/.cc)
which philipel@ is working on wiring up as a Chrome experiment.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/common_types.h#...
webrtc/common_types.h:763: // in the range (x, y) based on network jitter.
It's currently not possible to actually disable these constraints once they have
been applied. All you can do is to set them to the min and max values. Should we
have special interpretation of those to indicate that the constraints should be
disabled?

https://codereview.chromium.org/2007743003/diff/290042/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/config.cc#newco...
webrtc/config.cc:57:
"http://www.webrtc.org/experiments/rtp-hdrext/playout-delay";
We should make sure we add this page too.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1361:
_rtpRtcp.OnReceivedRtcpReceiverReport(
This sounds a bit wrong to me. Should it be OnReceivedRtcpReportBlocks()?
Technically I don't think an SR includes a receiver report.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:283: default:
No default, instead just list all types here.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/video_c...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/video_c...
webrtc/modules/video_coding/jitter_buffer.cc:501: VCMEncodedFrame*
encoded_frame;
Move this down to when it's actually used.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/video_c...
webrtc/modules/video_coding/jitter_buffer.cc:502: crit_sect_->Enter();
Could you change this to a scoped crit?

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/video_c...
webrtc/modules/video_coding/jitter_buffer.cc:543: encoded_frame =
decodable_frames_.Front();
Actually, might as well just return this directly if we use a scoped crit.

[Irfan, 2016-06-06 15:39:42]

https://codereview.chromium.org/2007743003/diff/290042/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/common_types.h#...
webrtc/common_types.h:763: // in the range (x, y) based on network jitter.
On 2016/06/06 14:08:15, stefan-webrtc (holmer) wrote:
> It's currently not possible to actually disable these constraints once they
have
> been applied. All you can do is to set them to the min and max values. Should
we
> have special interpretation of those to indicate that the constraints should
be
> disabled?

I think setting min = 0 and max = 10s (the current limit on the receiver used) 
is effectively disabling it. Do not think we need any special values.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/config.cc
File webrtc/config.cc (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/config.cc#newco...
webrtc/config.cc:57:
"http://www.webrtc.org/experiments/rtp-hdrext/playout-delay";
On 2016/06/06 14:08:15, stefan-webrtc (holmer) wrote:
> We should make sure we add this page too.

Yes, I captured this in the bug 5895 so it goes in before we close this issue
out

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1361:
_rtpRtcp.OnReceivedRtcpReceiverReport(
On 2016/06/06 14:08:15, stefan-webrtc (holmer) wrote:
> This sounds a bit wrong to me. Should it be OnReceivedRtcpReportBlocks()?
> Technically I don't think an SR includes a receiver report.

The current implementation seems to indicate the SR and RR *can* both include
this. Unclear if they do. For now, I am going to rename this as per your
suggestion.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:283: default:
On 2016/06/06 14:08:16, stefan-webrtc (holmer) wrote:
> No default, instead just list all types here.

Done.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/video_c...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/video_c...
webrtc/modules/video_coding/jitter_buffer.cc:501: VCMEncodedFrame*
encoded_frame;
On 2016/06/06 14:08:16, stefan-webrtc (holmer) wrote:
> Move this down to when it's actually used.

Done.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/video_c...
webrtc/modules/video_coding/jitter_buffer.cc:502: crit_sect_->Enter();
On 2016/06/06 14:08:16, stefan-webrtc (holmer) wrote:
> Could you change this to a scoped crit?

This is actually released at line 515 while doing a wait before being acquired
again. I think this is better left as is due to this dynamic behavior. lmk if
you think otherwise.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/video_c...
webrtc/modules/video_coding/jitter_buffer.cc:543: encoded_frame =
decodable_frames_.Front();
On 2016/06/06 14:08:16, stefan-webrtc (holmer) wrote:
> Actually, might as well just return this directly if we use a scoped crit.

Done.

[Irfan, 2016-06-06 15:41:36]

Stefan, PS#7 addresses your latest comments. PTAL.

[Irfan, 2016-06-06 15:42:45]

Patchset #8 (id:370001) has been deleted

[Irfan, 2016-06-06 15:43:41]

PS#8 is actually the latest update, not PS#7.

[Irfan, 2016-06-06 16:34:00]

Patchset #8 (id:390001) has been deleted

[stefan-webrtc, 2016-06-06 20:02:25]

lgtm, but consider the renaming, and follow up/sync with philipel@ about the new
jitter buffer.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/rtp_rtc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/rtp_rtc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1361:
_rtpRtcp.OnReceivedRtcpReceiverReport(
On 2016/06/06 15:39:41, Irfan wrote:
> On 2016/06/06 14:08:15, stefan-webrtc (holmer) wrote:
> > This sounds a bit wrong to me. Should it be OnReceivedRtcpReportBlocks()?
> > Technically I don't think an SR includes a receiver report.
> 
> The current implementation seems to indicate the SR and RR *can* both include
> this. Unclear if they do. For now, I am going to rename this as per your
> suggestion.

Ack, I'd prefer OnReceivedReportBlocks, especially since report blocks are what
you really pass in.

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/video_c...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.chromium.org/2007743003/diff/290042/webrtc/modules/video_c...
webrtc/modules/video_coding/jitter_buffer.cc:502: crit_sect_->Enter();
On 2016/06/06 15:39:42, Irfan wrote:
> On 2016/06/06 14:08:16, stefan-webrtc (holmer) wrote:
> > Could you change this to a scoped crit?
> 
> This is actually released at line 515 while doing a wait before being acquired
> again. I think this is better left as is due to this dynamic behavior. lmk if
> you think otherwise.

You are right, my mistake.

[Irfan, 2016-06-06 20:14:03]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2016-06-06 20:14:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.chromium.org/2007743003/#ps430001
(title: "Rename OnReceivedRtcpReport to OnReceivedRtcpReportBlocks")

[commit-bot: I haz the power, 2016-06-06 20:14:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007743003/430001

[commit-bot: I haz the power, 2016-06-06 20:17:43]

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

[commit-bot: I haz the power, 2016-06-06 20:17:46]

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

[Irfan, 2016-06-06 20:19:36]

isheriff@chromium.org changed reviewers:
+ mflodman@webrtc.org

[Irfan, 2016-06-06 20:21:21]

Magnus, need your lgtm for top-level files (config.h, config.cc,
common_types.cc, common_types.h).

Can you PTAL ?

[mflodman, 2016-06-08 06:37:44]

RS LGTM for the file in webrtc/ root.

[Irfan, 2016-06-08 06:43:01]

The CQ bit was checked by isheriff@chromium.org

[commit-bot: I haz the power, 2016-06-08 06:43:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007743003/430001

[commit-bot: I haz the power, 2016-06-08 07:24:26]

Description was changed from

==========
Add sender controlled playout delay limits

This CL adds support for an extension on RTP frames to allow the sender
to specify the minimum and maximum playout delay limits.

The receiver makes a best-effort attempt to keep the capture-to-render delay
within this range. This allows different types of application to specify
different end-to-end delay goals. For example gaming can support rendering
of frames as soon as received on receiver to minimize delay. A movie playback
application can specify a minimum playout delay to allow fixed buffering
in presence of network jitter.

There are no tests at this time and most of testing is done with chromium
webrtc prototype.

On chromoting performance tests, this extension helps bring down end-to-end
delay by about 150 ms on small frames.

BUG=webrtc:5895
==========

to

==========
Add sender controlled playout delay limits

This CL adds support for an extension on RTP frames to allow the sender
to specify the minimum and maximum playout delay limits.

The receiver makes a best-effort attempt to keep the capture-to-render delay
within this range. This allows different types of application to specify
different end-to-end delay goals. For example gaming can support rendering
of frames as soon as received on receiver to minimize delay. A movie playback
application can specify a minimum playout delay to allow fixed buffering
in presence of network jitter.

There are no tests at this time and most of testing is done with chromium
webrtc prototype.

On chromoting performance tests, this extension helps bring down end-to-end
delay by about 150 ms on small frames.

BUG=webrtc:5895
==========

[commit-bot: I haz the power, 2016-06-08 07:24:28]

Committed patchset #9 (id:430001)

[commit-bot: I haz the power, 2016-06-08 07:24:39]

Description was changed from

==========
Add sender controlled playout delay limits

This CL adds support for an extension on RTP frames to allow the sender
to specify the minimum and maximum playout delay limits.

The receiver makes a best-effort attempt to keep the capture-to-render delay
within this range. This allows different types of application to specify
different end-to-end delay goals. For example gaming can support rendering
of frames as soon as received on receiver to minimize delay. A movie playback
application can specify a minimum playout delay to allow fixed buffering
in presence of network jitter.

There are no tests at this time and most of testing is done with chromium
webrtc prototype.

On chromoting performance tests, this extension helps bring down end-to-end
delay by about 150 ms on small frames.

BUG=webrtc:5895
==========

to

==========
Add sender controlled playout delay limits

This CL adds support for an extension on RTP frames to allow the sender
to specify the minimum and maximum playout delay limits.

The receiver makes a best-effort attempt to keep the capture-to-render delay
within this range. This allows different types of application to specify
different end-to-end delay goals. For example gaming can support rendering
of frames as soon as received on receiver to minimize delay. A movie playback
application can specify a minimum playout delay to allow fixed buffering
in presence of network jitter.

There are no tests at this time and most of testing is done with chromium
webrtc prototype.

On chromoting performance tests, this extension helps bring down end-to-end
delay by about 150 ms on small frames.

BUG=webrtc:5895

Committed: https://crrev.com/6b4b5f37704cb414420d813eb39b90b25f5b2d6c
Cr-Commit-Position: refs/heads/master@{#13059}
==========

[commit-bot: I haz the power, 2016-06-08 07:24:41]

Patchset 9 (id:??) landed as
https://crrev.com/6b4b5f37704cb414420d813eb39b90b25f5b2d6c
Cr-Commit-Position: refs/heads/master@{#13059}

[kjellander_webrtc, 2016-08-17 15:23:41]

kjellander@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[kjellander_webrtc, 2016-08-17 15:23:43]

Edward found an error in this CL (test missing for the GN target). Please fix.

https://codereview.webrtc.org/2007743003/diff/430001/webrtc/modules/modules.gyp
File webrtc/modules/modules.gyp (right):

https://codereview.webrtc.org/2007743003/diff/430001/webrtc/modules/modules.g...
webrtc/modules/modules.gyp:302:
'rtp_rtcp/source/playout_delay_oracle_unittest.cc',
This entry is missing in webrtc/modules/BUILD.gn. Can you add it to
modules_unittests in there as well in a follow-up CL?