Experiment for the nack module.

Experiment for the nack module.

Testing the nack module by implementing it into the current jitter buffer
under the experiment WebRTC-NewVideoJitterBuffer.

BUG=webrtc:5514
Committed: https://crrev.com/83f831a919ff4c2d736ec3c5a33c6afb4d5eea47
Cr-Commit-Position: refs/heads/master@{#11969}

[philipel, 2016-03-08 12:41:47]

philipel@webrtc.org changed reviewers:
+ sprang@webrtc.org, stefan@webrtc.org

[stefan-webrtc, 2016-03-10 14:20:40]

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_coding.h (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_coding.h:78: // Create method for the
new jitter buffer.
I think you can remove these comments mentioning the new jitter buffer. They
will be obsolete very quickly (since the new jitter buffer soon isn't new at
all)

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_coding.h:83: ProcessThread*
module_process_thread,
I think it's easier to simply have the VideoCodingModule's Process() method call
the nack module's Process() method, instead of registering it to the
ProcessThread directly. Some users of VideoCodingModule doesn't have
ProcessThreads, and for those it will be unnecessarily difficult to use the
module.

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:263: new NackModule(clock,
nack_sender, keyframe_request_sender));
Set in initializer list instead so that you can make nack_module_ const.

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:284: if
(field_trial::FindFullName("WebRTC-NewVideoJitterBuffer") == "Enabled")
if (nack_module_) instead, here and all places below.

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:375: void VCMJitterBuffer::Flush()
{
Notify the nack module about the fact that we no longer are interested in any
missing packets?

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:625: return frame;
If we extract a frame for decoding I think we should notify the nack module so
that it stops nacking packets we no longer need. Or maybe we should do that as
we move frames into decodable_frames_ at lines 811-812?

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:1067:
DropPacketsFromNackList(EstimatedLowSequenceNumber(*rit->second));
If we should run this code despite using the nack module, should we then notify
the nack module here?

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:1070: }
Should we really run lines 1017-1070 if using the nack module?

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer_unittest.cc (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer_unittest.cc:2517: ASSERT_EQ(2u,
nack_sent_.size());
Why do we have 2 here and 1 for the old behavior?

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.h:45: public KeyFrameRequestSender {
Is there a point in webrtc::RtpRtcp implementing NackSender if
VideoReceiveStream already does?

[philipel, 2016-03-10 16:39:28]

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/include/video_coding.h (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_coding.h:78: // Create method for the
new jitter buffer.
On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> I think you can remove these comments mentioning the new jitter buffer. They
> will be obsolete very quickly (since the new jitter buffer soon isn't new at
> all)

Done.

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/include/video_coding.h:83: ProcessThread*
module_process_thread,
On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> I think it's easier to simply have the VideoCodingModule's Process() method
call
> the nack module's Process() method, instead of registering it to the
> ProcessThread directly. Some users of VideoCodingModule doesn't have
> ProcessThreads, and for those it will be unnecessarily difficult to use the
> module.

:(((((((((((((((((((((((((((

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:263: new NackModule(clock,
nack_sender, keyframe_request_sender));
On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> Set in initializer list instead so that you can make nack_module_ const.

Can't do that since we have to check first if we are running the experiment.

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:284: if
(field_trial::FindFullName("WebRTC-NewVideoJitterBuffer") == "Enabled")
On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> if (nack_module_) instead, here and all places below.

Done.

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:375: void VCMJitterBuffer::Flush()
{
On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> Notify the nack module about the fact that we no longer are interested in any
> missing packets?

Done.

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:625: return frame;
On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> If we extract a frame for decoding I think we should notify the nack module so
> that it stops nacking packets we no longer need. Or maybe we should do that as
> we move frames into decodable_frames_ at lines 811-812?

Better to do it here since this is the actual point when we know for sure that
we aren't interested in older packets.

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:1067:
DropPacketsFromNackList(EstimatedLowSequenceNumber(*rit->second));
On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> If we should run this code despite using the nack module, should we then
notify
> the nack module here?

No, this is logic related to the old nack behavior, the nack module has its own
behavior.

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:1070: }
On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> Should we really run lines 1017-1070 if using the nack module?

Yes, the GetNackList interface does more than just return the nack list, it also
is a way for the jitter buffer to signal that it wants to request a keyframe.

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer_unittest.cc (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer_unittest.cc:2517: ASSERT_EQ(2u,
nack_sent_.size());
On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> Why do we have 2 here and 1 for the old behavior?

The old only look at how many packets that are in the nack list currently, while
the nack_sent_ is the total number of nacks sent.

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.h:45: public KeyFrameRequestSender {
On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> Is there a point in webrtc::RtpRtcp implementing NackSender if
> VideoReceiveStream already does?

No, not really, fixed

[stefan-webrtc, 2016-03-10 17:06:30]

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:263: new NackModule(clock,
nack_sender, keyframe_request_sender));
On 2016/03/10 16:39:27, philipel wrote:
> On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> > Set in initializer list instead so that you can make nack_module_ const.
> 
> Can't do that since we have to check first if we are running the experiment.


You can still do it:

nack_module_(IsInExperiment() ? new NackModule() : nullptr),

I think that's preferable

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:1070: }
On 2016/03/10 16:39:27, philipel wrote:
> On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> > Should we really run lines 1017-1070 if using the nack module?
> 
> Yes, the GetNackList interface does more than just return the nack list, it
also
> is a way for the jitter buffer to signal that it wants to request a keyframe.

Ok, comment on why we don't return earlier to avoid future problems. And maybe
add a TODO to remove the GetNackList() method entirely. Btw, where is the code
above going to move when that is done?

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer_unittest.cc (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer_unittest.cc:2517: ASSERT_EQ(2u,
nack_sent_.size());
On 2016/03/10 16:39:28, philipel wrote:
> On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> > Why do we have 2 here and 1 for the old behavior?
> 
> The old only look at how many packets that are in the nack list currently,
while
> the nack_sent_ is the total number of nacks sent.

Acknowledged.

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:257: // a jitter buffer is created
for the video send stream as well.
Change this comment, since when the refactoring is done so that this isn't the
case, we shouldn't have a comment lying around saying so. Better to just say
"Only create a nack module if we are going to send nacks"

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:259: RTC_DCHECK(nack_sender);
Indentation

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:1378: return 100000;
This is a bit ugly, but it might be necessary. Is it possible to return max of
int64_t? I have some memory that it might cause problems in ModuleProcessThread,
i.e., it will sleep for ever if this is the first module that is added... In
that case 100000 might not work well either.

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/nack_module.cc (right):

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/nack_module.cc:134: return
std::max<int64_t>(next_process_time_ms_ - clock_->TimeInMilliseconds(),
I just realized that maybe we should check for -1 here and return 0 if that is
the case?

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/nack_module.h (right):

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/nack_module.h:38: void Flush();
Can we call this Clear() instead? Flush is a bad name in my opinion since it
sounds like "send all buffered nacks" (e.g., fflush()).

[philipel, 2016-03-11 09:24:15]

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:263: new NackModule(clock,
nack_sender, keyframe_request_sender));
On 2016/03/10 17:06:29, stefan-webrtc (holmer) wrote:
> On 2016/03/10 16:39:27, philipel wrote:
> > On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> > > Set in initializer list instead so that you can make nack_module_ const.
> > 
> > Can't do that since we have to check first if we are running the experiment.
> 
> 
> You can still do it:
> 
> nack_module_(IsInExperiment() ? new NackModule() : nullptr),
> 
> I think that's preferable

Ok, it is possible, but don't you think it would be really ugly to include this
block of code in the initialization list?

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:1070: }
On 2016/03/10 17:06:29, stefan-webrtc (holmer) wrote:
> On 2016/03/10 16:39:27, philipel wrote:
> > On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> > > Should we really run lines 1017-1070 if using the nack module?
> > 
> > Yes, the GetNackList interface does more than just return the nack list, it
> also
> > is a way for the jitter buffer to signal that it wants to request a
keyframe.
> 
> Ok, comment on why we don't return earlier to avoid future problems. And maybe
> add a TODO to remove the GetNackList() method entirely. Btw, where is the code
> above going to move when that is done?

Expanded my comment.

This code won't be necessary as the jitter buffer will have a
KeyframeRequestSender available, so whenever the jitter buffer wants to request
a keyframe according to some rule, then it can simply request the keyframe
immediately and not having to set a variable that will be picked up later by
some other modules Process function.

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:257: // a jitter buffer is created
for the video send stream as well.
On 2016/03/10 17:06:29, stefan-webrtc (holmer) wrote:
> Change this comment, since when the refactoring is done so that this isn't the
> case, we shouldn't have a comment lying around saying so. Better to just say
> "Only create a nack module if we are going to send nacks"

Done.

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:259: RTC_DCHECK(nack_sender);
On 2016/03/10 17:06:30, stefan-webrtc (holmer) wrote:
> Indentation

Done.

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:1378: return 100000;
On 2016/03/10 17:06:29, stefan-webrtc (holmer) wrote:
> This is a bit ugly, but it might be necessary. Is it possible to return max of
> int64_t? I have some memory that it might cause problems in
ModuleProcessThread,
> i.e., it will sleep for ever if this is the first module that is added... In
> that case 100000 might not work well either.

Changed to int64_t max. Just testing using video_loopback I don't get any
strange (hanging) behavior.

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/nack_module.cc (right):

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/nack_module.cc:134: return
std::max<int64_t>(next_process_time_ms_ - clock_->TimeInMilliseconds(),
On 2016/03/10 17:06:30, stefan-webrtc (holmer) wrote:
> I just realized that maybe we should check for -1 here and return 0 if that is
> the case?

That is the current behavior, or am I missing something?

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/nack_module.h (right):

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/nack_module.h:38: void Flush();
On 2016/03/10 17:06:30, stefan-webrtc (holmer) wrote:
> Can we call this Clear() instead? Flush is a bad name in my opinion since it
> sounds like "send all buffered nacks" (e.g., fflush()).

Done.

[stefan-webrtc, 2016-03-11 09:57:11]

LG

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:263: new NackModule(clock,
nack_sender, keyframe_request_sender));
On 2016/03/11 09:24:14, philipel wrote:
> On 2016/03/10 17:06:29, stefan-webrtc (holmer) wrote:
> > On 2016/03/10 16:39:27, philipel wrote:
> > > On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> > > > Set in initializer list instead so that you can make nack_module_ const.
> > > 
> > > Can't do that since we have to check first if we are running the
experiment.
> > 
> > 
> > You can still do it:
> > 
> > nack_module_(IsInExperiment() ? new NackModule() : nullptr),
> > 
> > I think that's preferable
> 
> Ok, it is possible, but don't you think it would be really ugly to include
this
> block of code in the initialization list?

I think that's fine, just break it out to a static helper function. It makes it
a lot more clear that the nack module doesn't have to be protected by critical
sections.

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/nack_module.cc (right):

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/nack_module.cc:134: return
std::max<int64_t>(next_process_time_ms_ - clock_->TimeInMilliseconds(),
On 2016/03/11 09:24:14, philipel wrote:
> On 2016/03/10 17:06:30, stefan-webrtc (holmer) wrote:
> > I just realized that maybe we should check for -1 here and return 0 if that
is
> > the case?
> 
> That is the current behavior, or am I missing something?

Not explicitly. It happens to work like that because -1 -
clock_->TimeInMilliseconds() will become negative, but to make it more obvious
that we have taken care of that case I think it is reasonable to add a separate 
if (next_process_time_ms_ == -1) 
  return 0;

[philipel, 2016-03-11 10:13:00]

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.webrtc.org/1778503002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:263: new NackModule(clock,
nack_sender, keyframe_request_sender));
On 2016/03/11 09:57:11, stefan-webrtc (holmer) wrote:
> On 2016/03/11 09:24:14, philipel wrote:
> > On 2016/03/10 17:06:29, stefan-webrtc (holmer) wrote:
> > > On 2016/03/10 16:39:27, philipel wrote:
> > > > On 2016/03/10 14:20:40, stefan-webrtc (holmer) wrote:
> > > > > Set in initializer list instead so that you can make nack_module_
const.
> > > > 
> > > > Can't do that since we have to check first if we are running the
> experiment.
> > > 
> > > 
> > > You can still do it:
> > > 
> > > nack_module_(IsInExperiment() ? new NackModule() : nullptr),
> > > 
> > > I think that's preferable
> > 
> > Ok, it is possible, but don't you think it would be really ugly to include
> this
> > block of code in the initialization list?
> 
> I think that's fine, just break it out to a static helper function. It makes
it
> a lot more clear that the nack module doesn't have to be protected by critical
> sections.

Done.

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/nack_module.cc (right):

https://codereview.webrtc.org/1778503002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/nack_module.cc:134: return
std::max<int64_t>(next_process_time_ms_ - clock_->TimeInMilliseconds(),
On 2016/03/11 09:57:11, stefan-webrtc (holmer) wrote:
> On 2016/03/11 09:24:14, philipel wrote:
> > On 2016/03/10 17:06:30, stefan-webrtc (holmer) wrote:
> > > I just realized that maybe we should check for -1 here and return 0 if
that
> is
> > > the case?
> > 
> > That is the current behavior, or am I missing something?
> 
> Not explicitly. It happens to work like that because -1 -
> clock_->TimeInMilliseconds() will become negative, but to make it more obvious
> that we have taken care of that case I think it is reasonable to add a
separate 
> if (next_process_time_ms_ == -1) 
>   return 0;

Acknowledged.

[stefan-webrtc, 2016-03-11 10:13:35]

lgtm

[philipel, 2016-03-11 12:19:16]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-03-11 12:19:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1778503002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1778503002/80001

[commit-bot: I haz the power, 2016-03-11 12:28:37]

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

[commit-bot: I haz the power, 2016-03-11 12:28:38]

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

[philipel, 2016-03-11 13:07:44]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-03-11 13:07:45]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1778503002/#ps100001 (title:
"Added nack module to BUILD.gn")

[commit-bot: I haz the power, 2016-03-11 13:07:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1778503002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1778503002/100001

[commit-bot: I haz the power, 2016-03-11 13:12:05]

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

[commit-bot: I haz the power, 2016-03-11 13:12:06]

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

[philipel, 2016-03-11 13:26:24]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-03-11 13:26:24]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1778503002/#ps120001 (title:
"Added histogram to BUILD.gn.")

[philipel, 2016-03-11 13:26:33]

The CQ bit was unchecked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-03-11 13:26:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1778503002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1778503002/120001

[philipel, 2016-03-11 13:32:51]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-03-11 13:32:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1778503002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1778503002/120001

[commit-bot: I haz the power, 2016-03-11 15:33:20]

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

[commit-bot: I haz the power, 2016-03-11 15:33:21]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[philipel, 2016-03-12 11:28:36]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-03-12 11:28:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1778503002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1778503002/120001

[commit-bot: I haz the power, 2016-03-12 11:30:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-12 11:30:35]

Description was changed from

==========
Experiment for the nack module.

Testing the nack module by implementing it into the current jitter buffer
under the experiment WebRTC-NewVideoJitterBuffer.

BUG=webrtc:5514
==========

to

==========
Experiment for the nack module.

Testing the nack module by implementing it into the current jitter buffer
under the experiment WebRTC-NewVideoJitterBuffer.

BUG=webrtc:5514

Committed: https://crrev.com/83f831a919ff4c2d736ec3c5a33c6afb4d5eea47
Cr-Commit-Position: refs/heads/master@{#11969}
==========

[commit-bot: I haz the power, 2016-03-12 11:30:36]

Patchset 7 (id:??) landed as
https://crrev.com/83f831a919ff4c2d736ec3c5a33c6afb4d5eea47
Cr-Commit-Position: refs/heads/master@{#11969}