Add support for handling reordered SS data on the receive-side for VP9.

Add support for handling reordered SS data on the receive-side for VP9.

BUG=chromium:500602
Committed: https://crrev.com/9a4cd876405212e5e718a49eee2cf2702c71c12c
Cr-Commit-Position: refs/heads/master@{#10383}

[åsapersson, 2015-10-12 14:14:40]

asapersson@webrtc.org changed reviewers:
+ stefan@webrtc.org

[åsapersson, 2015-10-13 10:29:51]

Patchset #8 (id:140001) has been deleted

[stefan-webrtc, 2015-10-13 13:49:43]

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:140: }
You may be able to implement this with ss_map_.lower_bound(timestamp) in some
way, not sure if it would be worth it though. Probably not since you will only
have one or two SS here, right?

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:147: if
(!Find(timestamp, &ss_timestamp))
It would be nice if Find() instead could return an iterator so that you can
simply use that to do an erase.

ss_map_.erase(ss_map_.begin(), it);

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:154: bool
Vp9SsMap::TimeForPeriodicUpdate(uint32_t timestamp) const {
What exactly is a periodic update?

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:158: uint32_t diff_sec
= (timestamp - ss_map_.begin()->first) / 90000U;
Name the constant 90000. We should already have that constant somewhere, right?

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:173: ss_map_[timestamp]
= gof;
How is this supposed to work? Looks like we are changing the timestamp of the
oldest gof? Why?

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:176: bool
Vp9SsMap::UpdatePacket(const VCMPacket* packet)  {
Can this method be const?

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:189:
RTPVideoTypeHeader* hdr = const_cast<RTPVideoTypeHeader*>(
Preferably no const_casts... Can we do it in some other way? At least do the
const_cast when calling the method.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:203: for
(FrameList::iterator it = frames->begin(); it != frames->end(); ++it) {
for (auto& it : *frames)

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:210: auto ss_iter =
ss_map_.find(ss_timestamp);
Wouldn't it be better to have Find() return this iterator?

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:212: continue;
Does this mean we haven't received the right SS yet? Or should we log a warning?

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:707:
vp9_ss_map_.RemoveOld(last_decoded_state_.time_stamp());
Can you explain why this is needed now?

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.h (right):

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.h:80: ~Vp9SsMap() {}
No need for these constructors, right?

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.h:93: bool Find(uint32_t
timestamp, uint32_t* ss_timestamp) const;
Not a big fan of this type of testing. Isn't it possible to verify in some other
way?

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/session_info.cc (right):

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/session_info.cc:121: return;
Should we get here if flexible_mode is true? Otherwise maybe we should be
logging?

[åsapersson, 2015-10-14 13:58:46]

Patchset #9 (id:180001) has been deleted

[åsapersson, 2015-10-14 15:09:01]

Patchset #9 (id:200001) has been deleted

[åsapersson, 2015-10-14 15:13:51]

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:140: }
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> You may be able to implement this with ss_map_.lower_bound(timestamp) in some
> way, not sure if it would be worth it though. Probably not since you will only
> have one or two SS here, right?

Should be around one or two (or as many that are received during a minute (old
currently cleaned up each minute)).

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:147: if
(!Find(timestamp, &ss_timestamp))
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> It would be nice if Find() instead could return an iterator so that you can
> simply use that to do an erase.
> 
> ss_map_.erase(ss_map_.begin(), it);

Done.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:154: bool
Vp9SsMap::TimeForPeriodicUpdate(uint32_t timestamp) const {
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> What exactly is a periodic update?

Renamed this function. Looks for if it is time to do a clean up. Thought it
should not be done too often? This is also advancing the oldest SS data to
handle timestamp wrap in case the SS data are received very seldom (e.g. only
once in beginning, second when IsNewerTimestamp not true).

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:158: uint32_t diff_sec
= (timestamp - ss_map_.begin()->first) / 90000U;
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> Name the constant 90000. We should already have that constant somewhere,
right?

Done.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:173: ss_map_[timestamp]
= gof;
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> How is this supposed to work? Looks like we are changing the timestamp of the
> oldest gof? Why?

See comment above.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:176: bool
Vp9SsMap::UpdatePacket(const VCMPacket* packet)  {
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> Can this method be const?

Done.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:189:
RTPVideoTypeHeader* hdr = const_cast<RTPVideoTypeHeader*>(
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> Preferably no const_casts... Can we do it in some other way? At least do the
> const_cast when calling the method.

Moved const_cast to when calling function.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:203: for
(FrameList::iterator it = frames->begin(); it != frames->end(); ++it) {
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> for (auto& it : *frames)

Done.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:210: auto ss_iter =
ss_map_.find(ss_timestamp);
On 2015/10/13 13:49:42, stefan-webrtc (holmer) wrote:
> Wouldn't it be better to have Find() return this iterator?

Done.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:212: continue;
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> Does this mean we haven't received the right SS yet? Or should we log a
warning?

The right not yet received, added a comment.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:707:
vp9_ss_map_.RemoveOld(last_decoded_state_.time_stamp());
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> Can you explain why this is needed now?

To not make the map possibly grow too large.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.h (right):

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.h:80: ~Vp9SsMap() {}
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> No need for these constructors, right?

Done.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/session_info.cc (right):

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/session_info.cc:121: return;
On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> Should we get here if flexible_mode is true? Otherwise maybe we should be
> logging?

If switching between flexible and non-flexible mode i think it should be
possible to get here.

[stefan-webrtc, 2015-10-16 06:45:06]

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:140: }
On 2015/10/14 15:13:51, åsapersson wrote:
> On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> > You may be able to implement this with ss_map_.lower_bound(timestamp) in
some
> > way, not sure if it would be worth it though. Probably not since you will
only
> > have one or two SS here, right?
> 
> Should be around one or two (or as many that are received during a minute (old
> currently cleaned up each minute)).

Acknowledged.

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:154: bool
Vp9SsMap::TimeForPeriodicUpdate(uint32_t timestamp) const {
On 2015/10/14 15:13:51, åsapersson wrote:
> On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> > What exactly is a periodic update?
> 
> Renamed this function. Looks for if it is time to do a clean up. Thought it
> should not be done too often? This is also advancing the oldest SS data to
> handle timestamp wrap in case the SS data are received very seldom (e.g. only
> once in beginning, second when IsNewerTimestamp not true).

Please add this as a comment to the method in the .h file.

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:153:
AdvanceFront(timestamp);
Is there any risk that we may be changing the order of the elements in the
ss_map_ here?

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:183: RTPVideoHeaderVP9&
vp9 = packet->codecSpecificHeader.codecHeader.VP9;
Make this a pointer instead.

[åsapersson, 2015-10-16 10:42:41]

Patchset #10 (id:240001) has been deleted

[åsapersson, 2015-10-16 10:45:19]

Patchset #10 (id:260001) has been deleted

[åsapersson, 2015-10-16 10:48:01]

Patchset #10 (id:280001) has been deleted

[åsapersson, 2015-10-16 10:52:26]

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:154: bool
Vp9SsMap::TimeForPeriodicUpdate(uint32_t timestamp) const {
On 2015/10/16 06:45:05, stefan-webrtc (holmer) wrote:
> On 2015/10/14 15:13:51, åsapersson wrote:
> > On 2015/10/13 13:49:43, stefan-webrtc (holmer) wrote:
> > > What exactly is a periodic update?
> > 
> > Renamed this function. Looks for if it is time to do a clean up. Thought it
> > should not be done too often? This is also advancing the oldest SS data to
> > handle timestamp wrap in case the SS data are received very seldom (e.g.
only
> > once in beginning, second when IsNewerTimestamp not true).
> 
> Please add this as a comment to the method in the .h file.

Done.

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:153:
AdvanceFront(timestamp);
On 2015/10/16 06:45:06, stefan-webrtc (holmer) wrote:
> Is there any risk that we may be changing the order of the elements in the
> ss_map_ here?

Don't think so. Only one SS that is older than timestamp should remain. This one
is moved forward. No packets or SS data with older timestamps than timetamp
should be inserted.

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:183: RTPVideoHeaderVP9&
vp9 = packet->codecSpecificHeader.codecHeader.VP9;
On 2015/10/16 06:45:06, stefan-webrtc (holmer) wrote:
> Make this a pointer instead.

Done.

[stefan-webrtc, 2015-10-16 12:26:11]

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:153:
AdvanceFront(timestamp);
On 2015/10/16 10:52:26, åsapersson wrote:
> On 2015/10/16 06:45:06, stefan-webrtc (holmer) wrote:
> > Is there any risk that we may be changing the order of the elements in the
> > ss_map_ here?
> 
> Don't think so. Only one SS that is older than timestamp should remain. This
one
> is moved forward. No packets or SS data with older timestamps than timetamp
> should be inserted.

You are right. Maybe it's still worth adding an DCHECK for that?

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:120: bool
Vp9SsMap::Insert(const VCMPacket& packet) {
I think we should break this class out to its own file. Feel free to do that in
a follow-up

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc (right):

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:55:
rtc::scoped_ptr<VCMPacket> packet_;
No need for a scoped_ptr

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:188: 
Maybe we should have a test with an incoming structure that we don't support on
the send-side? And maybe one with spatial layers? Might be a better fit for a
JitterBufferTest though?

[stefan-webrtc, 2015-10-16 12:27:02]

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

[stefan-webrtc, 2015-10-16 12:27:03]

Philip, you may want to take a look at this CL too as you have been doing work
related to this (flexible mode).

[philipel1, 2015-10-16 13:37:07]

philipel@google.com changed reviewers:
+ philipel@google.com

[philipel1, 2015-10-16 13:37:08]

Just nitpicking over variable names and comments, otherwise it LGTM.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:136: *ss_it = it;
A bit more descriptive variable names would be nice, maybe rename 'ss_it' to
'ss_it_out' and 'it' to 'ss_it'.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:147:
SsMap::const_iterator it;
Rename 'it' to 'ss_it'.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:180: return false;  //
Corresponding SS not yet received.
Can't differentiate between invalid input (incorrect gof_idx) and not yet
received SS. Change comment to 'Assume corresponding SS not yet received'.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:195: for (const auto&
it : *frames) {
Rename 'it' to 'frame_it'.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:203: continue;  //
Corresponding SS not yet received.
Again, assuming SS has not been received yet.

[åsapersson, 2015-10-20 11:35:06]

Patchset #12 (id:340001) has been deleted

[åsapersson, 2015-10-20 13:21:37]

Patchset #12 (id:360001) has been deleted

[åsapersson, 2015-10-20 13:22:02]

Patchset #12 (id:380001) has been deleted

[åsapersson, 2015-10-20 13:32:20]

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:153:
AdvanceFront(timestamp);
On 2015/10/16 12:26:11, stefan-webrtc (holmer) wrote:
> On 2015/10/16 10:52:26, åsapersson wrote:
> > On 2015/10/16 06:45:06, stefan-webrtc (holmer) wrote:
> > > Is there any risk that we may be changing the order of the elements in the
> > > ss_map_ here?
> > 
> > Don't think so. Only one SS that is older than timestamp should remain. This
> one
> > is moved forward. No packets or SS data with older timestamps than timetamp
> > should be inserted.
> 
> You are right. Maybe it's still worth adding an DCHECK for that?

I think the decoding state can be reset (when getting nack list) and in that
case an old packet could be inserted. Added a check in Insert for that.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:120: bool
Vp9SsMap::Insert(const VCMPacket& packet) {
On 2015/10/16 12:26:11, stefan-webrtc (holmer) wrote:
> I think we should break this class out to its own file. Feel free to do that
in
> a follow-up

Ok. Will do in a follow up CL.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:136: *ss_it = it;
On 2015/10/16 13:37:08, philipel1 wrote:
> A bit more descriptive variable names would be nice, maybe rename 'ss_it' to
> 'ss_it_out' and 'it' to 'ss_it'. 

Renamed to it_out.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:180: return false;  //
Corresponding SS not yet received.
On 2015/10/16 13:37:08, philipel1 wrote:
> Can't differentiate between invalid input (incorrect gof_idx) and not yet
> received SS. Change comment to 'Assume corresponding SS not yet received'. 

Done.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:195: for (const auto&
it : *frames) {
On 2015/10/16 13:37:08, philipel1 wrote:
> Rename 'it' to 'frame_it'.

Done.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:203: continue;  //
Corresponding SS not yet received.
On 2015/10/16 13:37:08, philipel1 wrote:
> Again, assuming SS has not been received yet.

Done.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc (right):

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:55:
rtc::scoped_ptr<VCMPacket> packet_;
On 2015/10/16 12:26:11, stefan-webrtc (holmer) wrote:
> No need for a scoped_ptr

Done.

https://codereview.webrtc.org/1386903002/diff/300001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:188: 
On 2015/10/16 12:26:11, stefan-webrtc (holmer) wrote:
> Maybe we should have a test with an incoming structure that we don't support
on
> the send-side? And maybe one with spatial layers? Might be a better fit for a
> JitterBufferTest though?

Added test with spatial layers.

[åsapersson, 2015-10-21 07:11:41]

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right):

https://codereview.webrtc.org/1386903002/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/jitter_buffer.cc:153:
AdvanceFront(timestamp);
On 2015/10/20 13:32:20, åsapersson wrote:
> On 2015/10/16 12:26:11, stefan-webrtc (holmer) wrote:
> > On 2015/10/16 10:52:26, åsapersson wrote:
> > > On 2015/10/16 06:45:06, stefan-webrtc (holmer) wrote:
> > > > Is there any risk that we may be changing the order of the elements in
the
> > > > ss_map_ here?
> > > 
> > > Don't think so. Only one SS that is older than timestamp should remain.
This
> > one
> > > is moved forward. No packets or SS data with older timestamps than
timetamp
> > > should be inserted.
> > 
> > You are right. Maybe it's still worth adding an DCHECK for that?
> 
> I think the decoding state can be reset (when getting nack list) and in that
> case an old packet could be inserted. Added a check in Insert for that.

Removed the check. An older SS could possibly be inserted be should not be
possible to be reordered right.

[stefan-webrtc, 2015-10-22 09:36:01]

lgtm

[åsapersson, 2015-10-23 07:12:44]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2015-10-23 07:12:44]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@google.com,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1386903002/#ps460001 (title:
"rebase")

[commit-bot: I haz the power, 2015-10-23 07:12:56]

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

[commit-bot: I haz the power, 2015-10-23 07:27:19]

Committed patchset #15 (id:460001)

[commit-bot: I haz the power, 2015-10-23 07:27:34]

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