Logic for finding frame references moved from PacketBuffer to new class

Logic for finding frame references moved from PacketBuffer to new class
RtpFrameReferenceFinder.

BUG=webrtc:5514
Committed: https://crrev.com/02447bc40822a2e3e0e53713d2c8653dd8e5555e
Cr-Commit-Position: refs/heads/master@{#12725}

[philipel, 2016-05-09 15:19:52]

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

[terelius, 2016-05-09 16:39:18]

lgtm with one nit/question below.

https://codereview.webrtc.org/1961053002/diff/1/webrtc/modules/video_coding/r...
File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right):

https://codereview.webrtc.org/1961053002/diff/1/webrtc/modules/video_coding/r...
webrtc/modules/video_coding/rtp_frame_reference_finder.cc:28:
current_ss_idx_(0),
Is ss_idx an established name, or could we find something more descriptive?

[philipel, 2016-05-11 09:40:32]

https://codereview.webrtc.org/1961053002/diff/1/webrtc/modules/video_coding/r...
File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right):

https://codereview.webrtc.org/1961053002/diff/1/webrtc/modules/video_coding/r...
webrtc/modules/video_coding/rtp_frame_reference_finder.cc:28:
current_ss_idx_(0),
On 2016/05/09 16:39:18, terelius wrote:
> Is ss_idx an established name, or could we find something more descriptive?

Kind of, it is not as established as a GOF or GOP but it is used in the Vp9 Rtp
specification.

[philipel, 2016-05-12 11:25:46]

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

[commit-bot: I haz the power, 2016-05-12 11:25:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1961053002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1961053002/1

[stefan-webrtc, 2016-05-12 11:52:07]

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

https://codereview.webrtc.org/1961053002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_object.cc:64: return nullptr;
DCHECK instead?

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

https://codereview.webrtc.org/1961053002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:209: return nullptr;
Should we DCHECK here, or is this a valid case?

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

https://codereview.webrtc.org/1961053002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.h:20: #include
"webrtc/modules/video_coding/rtp_frame_reference_finder.h"
Order

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

https://codereview.webrtc.org/1961053002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder.cc:261: void
RtpFrameReferenceFinder::ManageFrameVp9(
Is it the plan to further refactor this into a vp9 class and vp8 class, etc?

[philipel, 2016-05-12 13:34:49]

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

https://codereview.webrtc.org/1961053002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_object.cc:64: return nullptr;
On 2016/05/12 11:52:07, stefan-webrtc (holmer) wrote:
> DCHECK instead?

The reason i return nullptr istead of using a DCHECK is because a FrameObject
can become invalid after it was created. This is also the reason why
GetBitStream returns a bool instead of being a void function.

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

https://codereview.webrtc.org/1961053002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:209: return nullptr;
On 2016/05/12 11:52:07, stefan-webrtc (holmer) wrote:
> Should we DCHECK here, or is this a valid case?

This is a valid case, we return nullptr if the packet has been
removed/overwritten.

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

https://codereview.webrtc.org/1961053002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.h:20: #include
"webrtc/modules/video_coding/rtp_frame_reference_finder.h"
On 2016/05/12 11:52:07, stefan-webrtc (holmer) wrote:
> Order

Done.

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

https://codereview.webrtc.org/1961053002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder.cc:261: void
RtpFrameReferenceFinder::ManageFrameVp9(
On 2016/05/12 11:52:07, stefan-webrtc (holmer) wrote:
> Is it the plan to further refactor this into a vp9 class and vp8 class, etc?

I thought about that and I don't think we gain anything by doing so. If we have
a generic reference finder which both the Vp8 and Vp9 version inherits then we
will end up with three instanced of the generic class in the packet buffer,
since we need a generic, vp8 and vp9 frame reference finder.

[stefan-webrtc, 2016-05-13 11:10:23]

lgtm

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

https://codereview.webrtc.org/1961053002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:209: return nullptr;
On 2016/05/12 13:34:49, philipel wrote:
> On 2016/05/12 11:52:07, stefan-webrtc (holmer) wrote:
> > Should we DCHECK here, or is this a valid case?
> 
> This is a valid case, we return nullptr if the packet has been
> removed/overwritten.

Makes sense

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

https://codereview.webrtc.org/1961053002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/rtp_frame_reference_finder.cc:261: void
RtpFrameReferenceFinder::ManageFrameVp9(
On 2016/05/12 13:34:49, philipel wrote:
> On 2016/05/12 11:52:07, stefan-webrtc (holmer) wrote:
> > Is it the plan to further refactor this into a vp9 class and vp8 class, etc?
> 
> I thought about that and I don't think we gain anything by doing so. If we
have
> a generic reference finder which both the Vp8 and Vp9 version inherits then we
> will end up with three instanced of the generic class in the packet buffer,
> since we need a generic, vp8 and vp9 frame reference finder.

Can't we have vp8 and vp9 both inherit from generic? Let's leave it for now, it
just seemed like nice refactoring to make. Especially if we'd like this to be
pluggable at some point (if someone wants to build webrtc without vp9 support,
or with support for some other codec).

[philipel, 2016-05-13 11:21:10]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-05-13 11:21:10]

The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1961053002/#ps40001 (title:
"Includes in order.")

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

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

[commit-bot: I haz the power, 2016-05-13 13:01:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-13 13:01:15]

Description was changed from

==========
Logic for finding frame references moved from PacketBuffer to new class
RtpFrameReferenceFinder.

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

to

==========
Logic for finding frame references moved from PacketBuffer to new class
RtpFrameReferenceFinder.

BUG=webrtc:5514

Committed: https://crrev.com/02447bc40822a2e3e0e53713d2c8653dd8e5555e
Cr-Commit-Position: refs/heads/master@{#12725}
==========

[commit-bot: I haz the power, 2016-05-13 13:01:16]

Patchset 3 (id:??) landed as
https://crrev.com/02447bc40822a2e3e0e53713d2c8653dd8e5555e
Cr-Commit-Position: refs/heads/master@{#12725}