RtpFrameObject::GetCodecHeader now return rtc::Optional<RTPVideoTypeHeader>

RtpFrameObject::GetCodecHeader now return rtc::Optional<RTPVideoTypeHeader>

Since it is unsafe to hand out a pointer to a packet that might be removed/
overwritten at any time we now return a copy of the header if it exist.

BUG=webrtc:5514
Committed: https://crrev.com/88488287086bfada4c4cfe9b1119a4459b359b9d
Cr-Commit-Position: refs/heads/master@{#14920}

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

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

[terelius, 2016-11-02 13:08:19]

https://codereview.webrtc.org/2468183002/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_object.h (right):

https://codereview.webrtc.org/2468183002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_object.h:75: rtc::Optional<RTPVideoTypeHeader>
GetCodecHeader() const;
How large is the RTPVideoTypeHeader, and how often is the function called?
(I.e., will this have any performance impact?)

[philipel, 2016-11-02 13:12:24]

https://codereview.webrtc.org/2468183002/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_object.h (right):

https://codereview.webrtc.org/2468183002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_object.h:75: rtc::Optional<RTPVideoTypeHeader>
GetCodecHeader() const;
On 2016/11/02 13:08:18, terelius wrote:
> How large is the RTPVideoTypeHeader, and how often is the function called?
> (I.e., will this have any performance impact?)

It is 1624 bytes, and it is called ~1 time per frame (can be more if the frame
is retried in the RtpFrameReferenceFinder)

[terelius, 2016-11-02 13:18:38]

lgtm

https://codereview.webrtc.org/2468183002/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_object.h (right):

https://codereview.webrtc.org/2468183002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_object.h:75: rtc::Optional<RTPVideoTypeHeader>
GetCodecHeader() const;
On 2016/11/02 13:12:24, philipel wrote:
> On 2016/11/02 13:08:18, terelius wrote:
> > How large is the RTPVideoTypeHeader, and how often is the function called?
> > (I.e., will this have any performance impact?)
> 
> It is 1624 bytes, and it is called ~1 time per frame (can be more if the frame
> is retried in the RtpFrameReferenceFinder)

A 1.5 kB header seems like a lot. Does it have to be that large? Do you need the
entire header, or would it be possible to just copy parts of it as a future
optimization?

[philipel, 2016-11-02 13:26:15]

https://codereview.webrtc.org/2468183002/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_object.h (right):

https://codereview.webrtc.org/2468183002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_object.h:75: rtc::Optional<RTPVideoTypeHeader>
GetCodecHeader() const;
On 2016/11/02 13:18:38, terelius wrote:
> On 2016/11/02 13:12:24, philipel wrote:
> > On 2016/11/02 13:08:18, terelius wrote:
> > > How large is the RTPVideoTypeHeader, and how often is the function called?
> > > (I.e., will this have any performance impact?)
> > 
> > It is 1624 bytes, and it is called ~1 time per frame (can be more if the
frame
> > is retried in the RtpFrameReferenceFinder)
> 
> A 1.5 kB header seems like a lot. Does it have to be that large? Do you need
the
> entire header, or would it be possible to just copy parts of it as a future
> optimization?

It is quite large (in fact it is larger than the bitstream data that we want to
avoid copying) due to the GofInfoVP9 which contains multiple arrays and a matrix
that is mostly unused. Maybe the Gof can be stored more efficiently.

[terelius, 2016-11-02 13:34:49]

https://codereview.webrtc.org/2468183002/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_object.h (right):

https://codereview.webrtc.org/2468183002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_object.h:75: rtc::Optional<RTPVideoTypeHeader>
GetCodecHeader() const;
On 2016/11/02 13:26:14, philipel wrote:
> On 2016/11/02 13:18:38, terelius wrote:
> > On 2016/11/02 13:12:24, philipel wrote:
> > > On 2016/11/02 13:08:18, terelius wrote:
> > > > How large is the RTPVideoTypeHeader, and how often is the function
called?
> > > > (I.e., will this have any performance impact?)
> > > 
> > > It is 1624 bytes, and it is called ~1 time per frame (can be more if the
> frame
> > > is retried in the RtpFrameReferenceFinder)
> > 
> > A 1.5 kB header seems like a lot. Does it have to be that large? Do you need
> the
> > entire header, or would it be possible to just copy parts of it as a future
> > optimization?
> 
> It is quite large (in fact it is larger than the bitstream data that we want
to
> avoid copying) due to the GofInfoVP9 which contains multiple arrays and a
matrix
> that is mostly unused. Maybe the Gof can be stored more efficiently.

Ok. Can you file a feature request in the bugtracker for a more compact
representation of the Gof? A sparse representation of the matrix might be
useful.

[stefan-webrtc, 2016-11-03 13:46:02]

lgtm

[philipel, 2016-11-03 14:06:34]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-11-03 14:06:47]

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, 2016-11-03 14:36:41]

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

[commit-bot: I haz the power, 2016-11-03 14:36:43]

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

[philipel, 2016-11-03 15:17:01]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-11-03 15:17:05]

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, 2016-11-03 15:56:57]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-03 16:11:25]

Description was changed from

==========
RtpFrameObject::GetCodecHeader now return rtc::Optional<RTPVideoTypeHeader>

Since it is unsafe to hand out a pointer to a packet that might be removed/
overwritten at any time we now return a copy of the header if it exist.

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

to

==========
RtpFrameObject::GetCodecHeader now return rtc::Optional<RTPVideoTypeHeader>

Since it is unsafe to hand out a pointer to a packet that might be removed/
overwritten at any time we now return a copy of the header if it exist.

BUG=webrtc:5514

Committed: https://crrev.com/88488287086bfada4c4cfe9b1119a4459b359b9d
Cr-Commit-Position: refs/heads/master@{#14920}
==========

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

Patchset 1 (id:??) landed as
https://crrev.com/88488287086bfada4c4cfe9b1119a4459b359b9d
Cr-Commit-Position: refs/heads/master@{#14920}