Add flag enabling more packets to be retransmittable.

Add flag enabling more packets to be retransmittable.

If not indicated otherwise, allow adding a packet to the retransmission
history at least every 1/15s in order to reduce frame dropping.

BUG=webrtc:7694
Review-Url: https://codereview.webrtc.org/2999063002
Cr-Commit-Position: refs/heads/master@{#19665}
Committed: https://chromium.googlesource.com/external/webrtc/+/a8ae6f2acaef86e63f349442dbde8e81a9bd041e

[sprang_webrtc, 2017-08-16 15:09:41]

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

[commit-bot: I haz the power, 2017-08-16 15:09:55]

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-08-16 15:21:21]

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

[commit-bot: I haz the power, 2017-08-16 15:21:24]

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/20245)

[sprang_webrtc, 2017-08-29 14:39:03]

Description was changed from

==========
Add flag enabling more packets to be retransmittable.

If not indicated otherwise, allow adding a packet to the retransmission
history at least every 1/15s in order to reduce frame dropping.

BUG=
==========

to

==========
Add flag enabling more packets to be retransmittable.

If not indicated otherwise, allow adding a packet to the retransmission
history at least every 1/15s in order to reduce frame dropping.

BUG=webrtc:7694
==========

[sprang_webrtc, 2017-08-29 14:40:52]

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

[danilchap, 2017-08-29 17:31:20]

1st round mostly with style comments.
Will take another round tomorrow.

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:121:
kConditionallyRetransmitHigherLayers = 0x8,
can you add some comment what 'conditionally' mean.
(this mode is too unobvious)

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:227: if (hdr_info_.temporalIdx
== kNoTemporalIdx)
if retransmission_settings == kRetransmitFECPackets, should this packet be
retransmitted?
Or odd configurations are not supported?

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:568:
TEST_F(RtpPacketizerVp8Test, RetransmitTL1_2) {
_ is discouraged in test names.
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#why-s...

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:590:
TEST_F(RtpPacketizerVp8Test, RetransmitTL3) {
how TL3 is different from TL2/TL1?

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:410: default:
it is recommended not to have default to get compile-time error when a new enum
value will be added.

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:258: RTPSender* rtpSender,
rtp_sender

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:267:
std::unique_ptr<RtpPacketizer> packetizer(RtpPacketizer::Create(
or
auto packetizer = rtc::WrapUnique(RtpPacketizer::Create(

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1683: const int64_t
kWindowSize = 2500;
kWindowSizeMs?

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1686: const int32_t
kSettings =
may be uint8_t instead of int32_t

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1709: ADD_FAILURE() <<
"Bad temporal idx.";
with kPatternLength = 4 and i >= 0 this look impossible.

I wonder if if a small array can better show your intent:
constexpr int kPattern[] = {0, 2, 1, 2};
for(int i = 0; i < arraysize(kPattern) * kNumRepetition; ++i) {
  header.codecHeader.VP8.temporalIdx = kPattern[i % arraysize(kPattern)];
  ...
}

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1768: constexpr int
kPatternLength = 4;
and here 
kPattern = {0, 1, 1, 1};?

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:462:
RTPSenderVideo::TemporalLayerStats* RTPSenderVideo::GetTemporalLayerStats(
since return type can't be nullptr, may return reference:
 RTPSenderVideo::TemporalLayerStats& RTPSenderVideo::GetTemporalLayerStats(int
temporal_id) {
  return frame_stats_by_temporal_layer_.emplace(temporal_id,
kWindowSizeMs).first->second;
  // If you'll add default constructor to TemporalLayerStats, even easier:
  return frame_stats_by_temporal_layer_[temporal_id];
}

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:467: it =
frame_stats_by_temporal_layer_.emplace(temporal_id, kWindowSizeMs)
you do not need to search before emplace, so can remove double lookup:
return &frame_stats_by_temporal_layer_.emplace(temporal_id,
kWindowSizeMs).first->second;

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:543: retransmission_settings
|= kRetransmitHigherLayers;
you already checked temporal_layer > 0.
why not return StorageType::kAllowRetransmission directly?

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:92: int64_t last_frame_time_;
struct members shouldn't have '_' in the end.

[danilchap, 2017-08-30 13:52:29]

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:437:
&expected_retransmission_time_ms, nullptr,
can add comment which of many rtts you are taking here, e.g.
RTT(remote_ssrc, nullptr, /*avg_rtt=*/&expected_retransmission_time_ms, nullptr,
...);

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:473: StorageType
RTPSenderVideo::GetStorageType(
Looks strange to have GetStorageType both in codec-dependent Packetizers and
codec-independent RtpSenderVideo

may be move all the functionality here and remove GetStorageType from
packetizers.

(e.g. your cl fixes H264: it would currently retransmit even with
kRetransmitOff)

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:488: int temporal_layer = 0;
may be = kNoTemporalIdx

[sprang_webrtc, 2017-08-31 15:54:27]

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

[sprang_webrtc, 2017-08-31 15:54:29]

+brandtr in case there are any concerns for fec re refactoring of
GetProtectionType

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:121:
kConditionallyRetransmitHigherLayers = 0x8,
On 2017/08/29 17:31:19, danilchap wrote:
> can you add some comment what 'conditionally' mean.
> (this mode is too unobvious)

Done.

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8.cc:227: if (hdr_info_.temporalIdx
== kNoTemporalIdx)
On 2017/08/29 17:31:19, danilchap wrote:
> if retransmission_settings == kRetransmitFECPackets, should this packet be
> retransmitted?
> Or odd configurations are not supported?

I don't think fec makes sense here. This method is only used to determine the
storage type for the frame this packetizer contains. FEC packet generation lives
separately.

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:568:
TEST_F(RtpPacketizerVp8Test, RetransmitTL1_2) {
On 2017/08/29 17:31:19, danilchap wrote:
> _ is discouraged in test names.
>
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#why-s...

Done.

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:590:
TEST_F(RtpPacketizerVp8Test, RetransmitTL3) {
On 2017/08/29 17:31:19, danilchap wrote:
> how TL3 is different from TL2/TL1?

It was only different in an earlier revision of this cl. Removed.

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:437:
&expected_retransmission_time_ms, nullptr,
On 2017/08/30 13:52:29, danilchap wrote:
> can add comment which of many rtts you are taking here, e.g.
> RTT(remote_ssrc, nullptr, /*avg_rtt=*/&expected_retransmission_time_ms,
nullptr,
> ...);

Done.

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:410: default:
On 2017/08/29 17:31:19, danilchap wrote:
> it is recommended not to have default to get compile-time error when a new
enum
> value will be added.

Done.

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:258: RTPSender* rtpSender,
On 2017/08/29 17:31:19, danilchap wrote:
> rtp_sender

Done.

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:267:
std::unique_ptr<RtpPacketizer> packetizer(RtpPacketizer::Create(
On 2017/08/29 17:31:19, danilchap wrote:
> or
> auto packetizer = rtc::WrapUnique(RtpPacketizer::Create(

Done.

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1683: const int64_t
kWindowSize = 2500;
On 2017/08/29 17:31:19, danilchap wrote:
> kWindowSizeMs?

Done.

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1686: const int32_t
kSettings =
On 2017/08/29 17:31:20, danilchap wrote:
> may be uint8_t instead of int32_t

Done.

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1709: ADD_FAILURE() <<
"Bad temporal idx.";
On 2017/08/29 17:31:20, danilchap wrote:
> with kPatternLength = 4 and i >= 0 this look impossible.
> 
> I wonder if if a small array can better show your intent:
> constexpr int kPattern[] = {0, 2, 1, 2};
> for(int i = 0; i < arraysize(kPattern) * kNumRepetition; ++i) {
>   header.codecHeader.VP8.temporalIdx = kPattern[i % arraysize(kPattern)];
>   ...
> }

Yes, nice suggestion!

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1768: constexpr int
kPatternLength = 4;
On 2017/08/29 17:31:20, danilchap wrote:
> and here 
> kPattern = {0, 1, 1, 1};?

Done.

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:462:
RTPSenderVideo::TemporalLayerStats* RTPSenderVideo::GetTemporalLayerStats(
On 2017/08/29 17:31:20, danilchap wrote:
> since return type can't be nullptr, may return reference:
>  RTPSenderVideo::TemporalLayerStats& RTPSenderVideo::GetTemporalLayerStats(int
> temporal_id) {
>   return frame_stats_by_temporal_layer_.emplace(temporal_id,
> kWindowSizeMs).first->second;
>   // If you'll add default constructor to TemporalLayerStats, even easier:
>   return frame_stats_by_temporal_layer_[temporal_id];
> }

Are we ok with non-const references then?
Changed to default constructor and made the window size referencable from test.

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:467: it =
frame_stats_by_temporal_layer_.emplace(temporal_id, kWindowSizeMs)
On 2017/08/29 17:31:20, danilchap wrote:
> you do not need to search before emplace, so can remove double lookup:
> return &frame_stats_by_temporal_layer_.emplace(temporal_id,
> kWindowSizeMs).first->second;

Acknowledged.

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:473: StorageType
RTPSenderVideo::GetStorageType(
On 2017/08/30 13:52:29, danilchap wrote:
> Looks strange to have GetStorageType both in codec-dependent Packetizers and
> codec-independent RtpSenderVideo
> 
> may be move all the functionality here and remove GetStorageType from
> packetizers.
> 
> (e.g. your cl fixes H264: it would currently retransmit even with
> kRetransmitOff)

Yeah, that makes sense. Moved things around.

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:488: int temporal_layer = 0;
On 2017/08/30 13:52:29, danilchap wrote:
> may be = kNoTemporalIdx

Done.

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:543: retransmission_settings
|= kRetransmitHigherLayers;
On 2017/08/29 17:31:20, danilchap wrote:
> you already checked temporal_layer > 0.
> why not return StorageType::kAllowRetransmission directly?

Since this class doesn't know of the codec dependent bits would override that
for some reason. But this comment is now moot as I've GetStorageType() from the
packetizers.

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:92: int64_t last_frame_time_;
On 2017/08/29 17:31:20, danilchap wrote:
> struct members shouldn't have '_' in the end.

Done.

[danilchap, 2017-09-04 09:03:05]

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:462:
RTPSenderVideo::TemporalLayerStats* RTPSenderVideo::GetTemporalLayerStats(
On 2017/08/31 15:54:29, sprang_webrtc wrote:
> On 2017/08/29 17:31:20, danilchap wrote:
> > since return type can't be nullptr, may return reference:
> >  RTPSenderVideo::TemporalLayerStats&
RTPSenderVideo::GetTemporalLayerStats(int
> > temporal_id) {
> >   return frame_stats_by_temporal_layer_.emplace(temporal_id,
> > kWindowSizeMs).first->second;
> >   // If you'll add default constructor to TemporalLayerStats, even easier:
> >   return frame_stats_by_temporal_layer_[temporal_id];
> > }
> 
> Are we ok with non-const references then?
> Changed to default constructor and made the window size referencable from
test.

the rule states "All parameters passed by reference must be labeled const."

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc (left):

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:98: 
may be undo this line deletion to make CL one file shorter

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:22: #include
"webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h"
where is it used?

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:269: auto packetizer =
rtc::WrapUnique(RtpPacketizer::Create(
since packetizer is no longer needed, it might cleaner not to introduce
TestRtpSenderVideo at all.

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

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:404: const uint8_t
temporal_id =
this statement can be removed - temporal_id already calculated above

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:470: int32_t
retransmission_settings,
since retransmission_settings is a bitmask, may be unsigned type better be used.
is it possible to use RetransmissionMode directly?

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:471: RtpPacketizer*
packetizer,
this parameter is no longer needed

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:472: int64_t
expected_retransmission_time) {
add _ms

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:481:
(UpdateConditionalRetransmit(temporal_id,
extra () around function call can be removed

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right):

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:83: uint8_t
GetTemporalId(const RTPVideoHeader& header) const;
look like this one can be static or free function inside .cc

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:95: int64_t last_frame_time;
add _ms

[brandtr, 2017-09-04 10:29:22]

GetProtectionType changes lgtm

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

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:404: const uint8_t
temporal_id =
Reuse |temporal_id| from line 373?

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right):

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:92: :
frame_rate_fp1000s(kTLRateWindowSizeMs, 1000 * 1000),
Nice unit :)

[sprang_webrtc, 2017-09-04 11:09:29]

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

https://codereview.webrtc.org/2999063002/diff/120001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:462:
RTPSenderVideo::TemporalLayerStats* RTPSenderVideo::GetTemporalLayerStats(
On 2017/09/04 09:03:04, danilchap wrote:
> On 2017/08/31 15:54:29, sprang_webrtc wrote:
> > On 2017/08/29 17:31:20, danilchap wrote:
> > > since return type can't be nullptr, may return reference:
> > >  RTPSenderVideo::TemporalLayerStats&
> RTPSenderVideo::GetTemporalLayerStats(int
> > > temporal_id) {
> > >   return frame_stats_by_temporal_layer_.emplace(temporal_id,
> > > kWindowSizeMs).first->second;
> > >   // If you'll add default constructor to TemporalLayerStats, even easier:
> > >   return frame_stats_by_temporal_layer_[temporal_id];
> > > }
> > 
> > Are we ok with non-const references then?
> > Changed to default constructor and made the window size referencable from
> test.
> 
> the rule states "All parameters passed by reference must be labeled const."

Acknowledged.

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc (left):

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc:98: 
On 2017/09/04 09:03:04, danilchap wrote:
> may be undo this line deletion to make CL one file shorter

Whoops, yes. Had set my diff tool to ignore whitespace so missed I did this.

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:22: #include
"webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h"
On 2017/09/04 09:03:04, danilchap wrote:
> where is it used?

It's not in this revision. Removed.

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:269: auto packetizer =
rtc::WrapUnique(RtpPacketizer::Create(
On 2017/09/04 09:03:04, danilchap wrote:
> since packetizer is no longer needed, it might cleaner not to introduce
> TestRtpSenderVideo at all.

It's still needed in order to access the protected GetStorageType() method
though.
Or would you prefer I make RtpSenderVideoTest a friend class?

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

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:404: const uint8_t
temporal_id =
On 2017/09/04 09:03:04, danilchap wrote:
> this statement can be removed - temporal_id already calculated above

Done.

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:404: const uint8_t
temporal_id =
On 2017/09/04 10:29:22, brandtr wrote:
> Reuse |temporal_id| from line 373?

Done.

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:470: int32_t
retransmission_settings,
On 2017/09/04 09:03:04, danilchap wrote:
> since retransmission_settings is a bitmask, may be unsigned type better be
used.
> is it possible to use RetransmissionMode directly?

Using RetransmissionMode directly forces a lot of explicit casts to be done. It
would also be weird to mask two values together and then cast it to a
RetransmissionMode value that is not declared by the enum.

I could potentially make it unsigned (or uint8_t, which is what
RetransmissionMode is typed as), but am hesitant to change the interface at this
time. I suspect there are downstream dependencies...

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:471: RtpPacketizer*
packetizer,
On 2017/09/04 09:03:04, danilchap wrote:
> this parameter is no longer needed

Done.

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:472: int64_t
expected_retransmission_time) {
On 2017/09/04 09:03:04, danilchap wrote:
> add _ms

Done.

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:481:
(UpdateConditionalRetransmit(temporal_id,
On 2017/09/04 09:03:04, danilchap wrote:
> extra () around function call can be removed

Done.

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.h (right):

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:83: uint8_t
GetTemporalId(const RTPVideoHeader& header) const;
On 2017/09/04 09:03:05, danilchap wrote:
> look like this one can be static or free function inside .cc

Made it static. Call from unit test.

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:92: :
frame_rate_fp1000s(kTLRateWindowSizeMs, 1000 * 1000),
On 2017/09/04 10:29:22, brandtr wrote:
> Nice unit :)

Hehe, thanks :)
Maybe I'll add a comment explicitly stating it is to increase precision at low
frame rates.

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.h:95: int64_t last_frame_time;
On 2017/09/04 09:03:05, danilchap wrote:
> add _ms

Done.

[danilchap, 2017-09-04 11:19:10]

lgtm

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right):

https://codereview.webrtc.org/2999063002/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:269: auto packetizer =
rtc::WrapUnique(RtpPacketizer::Create(
On 2017/09/04 11:09:28, sprang_webrtc wrote:
> On 2017/09/04 09:03:04, danilchap wrote:
> > since packetizer is no longer needed, it might cleaner not to introduce
> > TestRtpSenderVideo at all.
> 
> It's still needed in order to access the protected GetStorageType() method
> though.
> Or would you prefer I make RtpSenderVideoTest a friend class?

missed that reason. Yep, keep it then, or make StorageType/GetTemporalId
functions public.

[sprang_webrtc, 2017-09-04 12:08:53]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-09-04 12:08:54]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2999063002/#ps160001 (title:
"Addressed comments")

[commit-bot: I haz the power, 2017-09-04 12:09:00]

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-09-04 14:23:56]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1504526933688790,
"parent_rev": "791a8f611da8cc6b0668ccc03bf7525e6ba72a46", "commit_rev":
"a8ae6f2acaef86e63f349442dbde8e81a9bd041e"}

[commit-bot: I haz the power, 2017-09-04 14:24:02]

Description was changed from

==========
Add flag enabling more packets to be retransmittable.

If not indicated otherwise, allow adding a packet to the retransmission
history at least every 1/15s in order to reduce frame dropping.

BUG=webrtc:7694
==========

to

==========
Add flag enabling more packets to be retransmittable.

If not indicated otherwise, allow adding a packet to the retransmission
history at least every 1/15s in order to reduce frame dropping.

BUG=webrtc:7694

Review-Url: https://codereview.webrtc.org/2999063002
Cr-Commit-Position: refs/heads/master@{#19665}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a8ae6f2acaef86e63f349442d...
==========

[commit-bot: I haz the power, 2017-09-04 14:24:04]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/a8ae6f2acaef86e63f349442d...