Unwrap picture ids in the RtpFrameReferencerFinder.

Unwrap picture ids in the RtpFrameReferencerFinder.

First CL to avoid working with wrapping picture ids. After the references of
a frame has been determined they are then unwrapped.

BUG=webrtc:7874
Review-Url: https://codereview.webrtc.org/2985283002
Cr-Commit-Position: refs/heads/master@{#19663}
Committed: https://chromium.googlesource.com/external/webrtc/+/d4fac6957e14068f8dbf2ab4a20006b1df044274

[y2468, 2017-07-28 15:50:56]

yasin.soliman7@gmail.com changed reviewers:
+ yasin.soliman7@gmail.com

[y2468, 2017-07-28 15:50:56]

lgtm

[y2468, 2017-07-28 16:01:09]

Hi, the above comment was made in error while browsing the Code Review site.
Apologies for the inconvenience, please could you disregard the above message
and remove my reviewer tag on this issue? Thanks.

[philipel, 2017-07-28 16:07:12]

On 2017/07/28 16:01:09, ys2468 wrote:
> Hi, the above comment was made in error while browsing the Code Review site.
> Apologies for the inconvenience, please could you disregard the above message
> and remove my reviewer tag on this issue? Thanks.

No worries :)

[y2468, 2017-07-28 16:14:51]

On 2017/07/28 16:07:12, philipel wrote:
> On 2017/07/28 16:01:09, ys2468 wrote:
> > Hi, the above comment was made in error while browsing the Code Review site.
> > Apologies for the inconvenience, please could you disregard the above
message
> > and remove my reviewer tag on this issue? Thanks.
> 
> No worries :)

Thanks :)
Would it be possible to remove these comments (along with any mentions of my
email address) please?

[philipel, 2017-08-11 12:59:47]

philipel@webrtc.org changed reviewers:
+ terelius@webrtc.org
- yasin.soliman7@gmail.com

[philipel, 2017-08-18 11:23:39]

ping

[terelius, 2017-08-18 11:53:16]

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

https://codereview.webrtc.org/2985283002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder.cc:31:
generic_unwrapper_(100000),
Is this the starting point for unwrapped numbers? If so, is there a reason for
not taking something close to 2**62?

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

https://codereview.webrtc.org/2985283002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder.h:215:
SeqNumUnwrapper<uint16_t> generic_unwrapper_ GUARDED_BY(crit_);
It is not clear to me what should be unwrapped with unwrapper_ and what should
be unwrapped with generic_unwrapper_. Could the names be made more clear?

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

https://codereview.webrtc.org/2985283002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:219:
std::set<int64_t> actual_refs;
You could consider typedefing PictureId to int64_t. Here and in the production
code.

[philipel, 2017-08-24 10:46:11]

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

https://codereview.webrtc.org/2985283002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder.cc:31:
generic_unwrapper_(100000),
On 2017/08/18 11:53:16, terelius wrote:
> Is this the starting point for unwrapped numbers? If so, is there a reason for
> not taking something close to 2**62?

Changed the SeqNumUnwrapper default start value to 10000000000000000000UL, which
is larger than 2^63 and smaller than 2^64.

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

https://codereview.webrtc.org/2985283002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder.h:215:
SeqNumUnwrapper<uint16_t> generic_unwrapper_ GUARDED_BY(crit_);
On 2017/08/18 11:53:16, terelius wrote:
> It is not clear to me what should be unwrapped with unwrapper_ and what should
> be unwrapped with generic_unwrapper_. Could the names be made more clear?

Added comments.

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

https://codereview.webrtc.org/2985283002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:219:
std::set<int64_t> actual_refs;
On 2017/08/18 11:53:16, terelius wrote:
> You could consider typedefing PictureId to int64_t. Here and in the production
> code.

I think that might be a good idea, but I think it might require some discussion
before we do that.

[philipel, 2017-08-29 11:06:31]

ping

[terelius, 2017-08-30 11:59:33]

lgtm

https://codereview.webrtc.org/2985283002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right):

https://codereview.webrtc.org/2985283002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder.h:221:
SeqNumUnwrapper<uint16_t, kPicIdLength> unwrapper_ GUARDED_BY(crit_);
vpx_unwrapper_?

[philipel, 2017-08-30 12:11:20]

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

[commit-bot: I haz the power, 2017-08-30 12:11:28]

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-30 12:32:19]

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

[commit-bot: I haz the power, 2017-08-30 12:32:20]

Dry run: This issue passed the CQ dry run.

[philipel, 2017-08-30 12:33:49]

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

[philipel, 2017-08-30 12:33:50]

PTAL

[stefan-webrtc, 2017-09-01 07:25:09]

https://codereview.webrtc.org/2985283002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right):

https://codereview.webrtc.org/2985283002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder.cc:398: return kDrop;
Log error?

https://codereview.webrtc.org/2985283002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/sequence_number_util.h (right):

https://codereview.webrtc.org/2985283002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/sequence_number_util.h:96: // that is a power of 10
is because it is more human readable.
Comment also on why we need a large start value, and why as large as 2^63?

[philipel, 2017-09-01 08:55:46]

https://codereview.webrtc.org/2985283002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right):

https://codereview.webrtc.org/2985283002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder.cc:398: return kDrop;
On 2017/09/01 07:25:09, stefan-webrtc wrote:
> Log error?

Done.

https://codereview.webrtc.org/2985283002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/sequence_number_util.h (right):

https://codereview.webrtc.org/2985283002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/sequence_number_util.h:96: // that is a power of 10
is because it is more human readable.
On 2017/09/01 07:25:09, stefan-webrtc wrote:
> Comment also on why we need a large start value, and why as large as 2^63?

Done.

[stefan-webrtc, 2017-09-04 11:38:17]

lgtm

[philipel, 2017-09-04 11:48:24]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-09-04 11:48:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
yasin.soliman7@gmail.com, terelius@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2985283002/#ps100001 (title:
"Feedback")

[commit-bot: I haz the power, 2017-09-04 11:48:32]

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:03:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1504525704769010,
"parent_rev": "054b10882001974e419a808eea995f6d38a33ba3", "commit_rev":
"d4fac6957e14068f8dbf2ab4a20006b1df044274"}

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

Description was changed from

==========
Unwrap picture ids in the RtpFrameReferencerFinder.

First CL to avoid working with wrapping picture ids. After the references of
a frame has been determined they are then unwrapped.

BUG=webrtc:7874
==========

to

==========
Unwrap picture ids in the RtpFrameReferencerFinder.

First CL to avoid working with wrapping picture ids. After the references of
a frame has been determined they are then unwrapped.

BUG=webrtc:7874

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

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

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/d4fac6957e14068f8dbf2ab4a...