RTC Video Encoder: Use capturer timestamp

RTC Video Encoder: Use capturer timestamp

Add the capturer timestamp as a parameter in VEA::BitstreamBufferReady() so
RTCVideoEncoder can use it.

BUG=350106
TEST=Verify that apprtc loopback works on minnie.
R=bbudge@chromium.org, posciak@chromium.org, sandersd@chromium.org, wfh@chromium.org, wuchengli@chromium.org

Committed: https://crrev.com/5d967288829b022d533d839e4883902858615682
Cr-Commit-Position: refs/heads/master@{#396786}

[bbudge, 2016-05-19 09:29:50]

content/renderer/pepper lgtm

https://codereview.chromium.org/1996453003/diff/1/content/renderer/pepper/vid...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/1996453003/diff/1/content/renderer/pepper/vid...
content/renderer/pepper/video_encoder_shim.cc:490: // TODO: Pass real timestamp.
TODO(llandwerlin)

[shenghao, 2016-05-19 09:57:11]

On 2016/05/19 09:29:50, bbudge wrote:
> content/renderer/pepper lgtm
> 
>
https://codereview.chromium.org/1996453003/diff/1/content/renderer/pepper/vid...
> File content/renderer/pepper/video_encoder_shim.cc (right):
> 
>
https://codereview.chromium.org/1996453003/diff/1/content/renderer/pepper/vid...
> content/renderer/pepper/video_encoder_shim.cc:490: // TODO: Pass real
timestamp.
> TODO(llandwerlin)

Done

[Will Harris, 2016-05-19 09:57:57]

media/gpu/ipc/common/media_messages.h lgtm

[shenghao, 2016-05-19 10:01:53]

Description was changed from

==========
RTC Video Encoder: Use capturer timestamp

Add the capturer timestamp as a parameter in VEA::BitstreamBufferReady() so
RTCVideoEncoder can use it.

BUG=350106
TEST=Verify that apprtc loopback works on minnie.
R=bbudge@chromium.org, posciak@chromium.org, sandersd@chromium.org,
wfh@chromium.org, wuchengli@chromium.org
==========

to

==========
RTC Video Encoder: Use capturer timestamp

Add the capturer timestamp as a parameter in VEA::BitstreamBufferReady() so
RTCVideoEncoder can use it.

BUG=350106
TEST=Verify that apprtc loopback works on minnie.
R=bbudge@chromium.org, posciak@chromium.org, sandersd@chromium.org,
wfh@chromium.org, wuchengli@chromium.org
==========

[shenghao, 2016-05-19 10:01:53]

shenghao@chromium.org changed reviewers:
+ nasko@chromium.org
- wfh@chromium.org

[shenghao, 2016-05-19 10:02:48]

shenghao@chromium.org changed reviewers:
+ wfh@chromium.org
- nasko@chromium.org

[sandersd (OOO until July 31), 2016-05-19 20:27:12]

media/ LGTM % documentation nit.

https://codereview.chromium.org/1996453003/diff/40001/media/video/video_encod...
File media/video/video_encode_accelerator.h (right):

https://codereview.chromium.org/1996453003/diff/40001/media/video/video_encod...
media/video/video_encode_accelerator.h:81: //  |key_frame| is true if this
delivered frame is a keyframe.
Document |timestamp|.

[pbos, 2016-05-20 12:15:50]

pbos@chromium.org changed reviewers:
+ pbos@chromium.org

[pbos, 2016-05-20 12:15:51]

drive-by of RtcVideoEncoder from a WebRTC perspective.

We need the 90khz RTP timestamp that WebRTC provides to be copied over.

Can/should we expose this as a more opaque ID? Otherwise we need to be really
careful when converting this back and forth to a TimeDelta. And you need to take
care of wrapping.

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:480: const uint32_t rtp_timestamp =
This is the timestamp that should be copied over for webrtc's usage.

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:571:
base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms()));
This needs to copy over next_frame->timestamp(), not the NTP time. We need the
RTP timestamps to match.

Be careful about rounding errors as well, we use this timestamp to match
incoming frames with outgoing frames.

[pbos, 2016-05-20 12:17:56]

Oh, and thanks a lot for taking care of this. We've been wanting this change for
a long long time. If we get it right the CPU adaptation, stats and some decode
issues should "just work" on ChromeOS after this change.

[wuchengli, 2016-05-23 02:54:57]

Removing myself from reviewers. Leaving this to Pawel.

[wuchengli, 2016-05-23 02:55:11]

wuchengli@chromium.org changed reviewers:
- wuchengli@chromium.org

[Pawel Osciak, 2016-05-23 07:15:53]

Please test this using a new test case in veaunittest as proposed in my comment
below. Thank you.

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:476: const int64_t capture_time_us =
timestamp.InMicroseconds();
Please remove this line and at l.489:

image.capture_time_ms_ = timestamp.InMilliseconds();

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:479: const int64_t capture_time_ms =
capture_time_us / 1000;
This could also be removed and l.480 modified accordingly.

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_video_encoder_host.cc:452:
PpapiPluginMsg_VideoEncoder_BitstreamBufferReady(
Could we add a TODO +crbug to add this to the pepper API?

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:492: base::TimeDelta());
Could we use frame.timestamp() here?

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:669:
static_cast<int64_t>(dqbuf.timestamp.tv_usec))));
Do we need the cast? Also, we need both tv_sec and tv_usec parts of
dqbuf.timestamp to recreate the original timestamp?

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:696: timestamp.tv_usec =
I'm wondering if we need the casts?

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:699: 
Do we need the temporary timestamp, could we just assign directly to
qbuf.timestamp ?

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/vaapi_video_e...
File media/gpu/vaapi_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/vaapi_video_e...
media/gpu/vaapi_video_encode_accelerator.cc:560: encode_job->keyframe,
base::TimeDelta()));
Could you implement this by adding timestamp to EncodeJob please?

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/video_encode_...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/video_encode_...
media/gpu/video_encode_accelerator_unittest.cc:1255: base::TimeDelta timestamp)
{
Please add a test case that tests input timestamps passed to Encode() are
properly propagated to BBR() (and in correct order).

https://codereview.chromium.org/1996453003/diff/40001/media/video/fake_video_...
File media/video/fake_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/media/video/fake_video_...
media/video/fake_video_encode_accelerator.cc:135: base::TimeDelta());
Perhaps Time::Now() ?

https://codereview.chromium.org/1996453003/diff/40001/media/video/video_encod...
File media/video/video_encode_accelerator.h (right):

https://codereview.chromium.org/1996453003/diff/40001/media/video/video_encod...
media/video/video_encode_accelerator.h:85: base::TimeDelta timestamp) = 0;
Perhaps const TimeDelta& here and in other places?

[shenghao, 2016-05-24 10:48:00]

On 2016/05/23 07:15:53, Pawel Osciak wrote:
> Please test this using a new test case in veaunittest as proposed in my
comment
> below. Thank you.
> 
>
https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
> File content/renderer/media/rtc_video_encoder.cc (right):
> 
>
https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
> content/renderer/media/rtc_video_encoder.cc:476: const int64_t capture_time_us
=
> timestamp.InMicroseconds();
> Please remove this line and at l.489:
> 
> image.capture_time_ms_ = timestamp.InMilliseconds();
> 
>
https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
> content/renderer/media/rtc_video_encoder.cc:479: const int64_t capture_time_ms
=
> capture_time_us / 1000;
> This could also be removed and l.480 modified accordingly.
> 
>
https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
> File content/renderer/pepper/pepper_video_encoder_host.cc (right):
> 
>
https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
> content/renderer/pepper/pepper_video_encoder_host.cc:452:
> PpapiPluginMsg_VideoEncoder_BitstreamBufferReady(
> Could we add a TODO +crbug to add this to the pepper API?
> 
>
https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
> File content/renderer/pepper/video_encoder_shim.cc (right):
> 
>
https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
> content/renderer/pepper/video_encoder_shim.cc:492: base::TimeDelta());
> Could we use frame.timestamp() here?
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
> File media/gpu/v4l2_video_encode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
> media/gpu/v4l2_video_encode_accelerator.cc:669:
> static_cast<int64_t>(dqbuf.timestamp.tv_usec))));
> Do we need the cast? Also, we need both tv_sec and tv_usec parts of
> dqbuf.timestamp to recreate the original timestamp?
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
> media/gpu/v4l2_video_encode_accelerator.cc:696: timestamp.tv_usec =
> I'm wondering if we need the casts?
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
> media/gpu/v4l2_video_encode_accelerator.cc:699: 
> Do we need the temporary timestamp, could we just assign directly to
> qbuf.timestamp ?
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/gpu/vaapi_video_e...
> File media/gpu/vaapi_video_encode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/gpu/vaapi_video_e...
> media/gpu/vaapi_video_encode_accelerator.cc:560: encode_job->keyframe,
> base::TimeDelta()));
> Could you implement this by adding timestamp to EncodeJob please?
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/gpu/video_encode_...
> File media/gpu/video_encode_accelerator_unittest.cc (right):
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/gpu/video_encode_...
> media/gpu/video_encode_accelerator_unittest.cc:1255: base::TimeDelta
timestamp)
> {
> Please add a test case that tests input timestamps passed to Encode() are
> properly propagated to BBR() (and in correct order).
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/video/fake_video_...
> File media/video/fake_video_encode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/video/fake_video_...
> media/video/fake_video_encode_accelerator.cc:135: base::TimeDelta());
> Perhaps Time::Now() ?
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/video/video_encod...
> File media/video/video_encode_accelerator.h (right):
> 
>
https://codereview.chromium.org/1996453003/diff/40001/media/video/video_encod...
> media/video/video_encode_accelerator.h:85: base::TimeDelta timestamp) = 0;
> Perhaps const TimeDelta& here and in other places?

[shenghao, 2016-05-24 10:50:20]

https://codereview.chromium.org/1996453003/diff/1/content/renderer/pepper/vid...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/1996453003/diff/1/content/renderer/pepper/vid...
content/renderer/pepper/video_encoder_shim.cc:490: // TODO: Pass real timestamp.
On 2016/05/19 09:29:49, bbudge wrote:
> TODO(llandwerlin)

Done.

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:476: const int64_t capture_time_us =
timestamp.InMicroseconds();
On 2016/05/23 07:15:52, Pawel Osciak wrote:
> Please remove this line and at l.489:
> 
> image.capture_time_ms_ = timestamp.InMilliseconds();

Done.

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:479: const int64_t capture_time_ms =
capture_time_us / 1000;
On 2016/05/23 07:15:52, Pawel Osciak wrote:
> This could also be removed and l.480 modified accordingly.

Done.

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:480: const uint32_t rtp_timestamp =
On 2016/05/20 12:15:51, pbos wrote:
> This is the timestamp that should be copied over for webrtc's usage.
Talked offline. We would calculate rtp_timestamp from ntp_timestamp here.

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:571:
base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms()));
On 2016/05/20 12:15:51, pbos wrote:
> This needs to copy over next_frame->timestamp(), not the NTP time. We need the
> RTP timestamps to match.
> 
> Be careful about rounding errors as well, we use this timestamp to match
> incoming frames with outgoing frames.

Talked offline. We agreed to use ntp_time_ms()

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
content/renderer/pepper/pepper_video_encoder_host.cc:452:
PpapiPluginMsg_VideoEncoder_BitstreamBufferReady(
On 2016/05/23 07:15:52, Pawel Osciak wrote:
> Could we add a TODO +crbug to add this to the pepper API?

Done.

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/content/renderer/pepper...
content/renderer/pepper/video_encoder_shim.cc:492: base::TimeDelta());
On 2016/05/23 07:15:52, Pawel Osciak wrote:
> Could we use frame.timestamp() here?

Done.

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:669:
static_cast<int64_t>(dqbuf.timestamp.tv_usec))));
On 2016/05/23 07:15:53, Pawel Osciak wrote:
> Do we need the cast? Also, we need both tv_sec and tv_usec parts of
> dqbuf.timestamp to recreate the original timestamp?

Removed the cast. I think we only need the usec to recreate the original
timestamp

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:696: timestamp.tv_usec =
On 2016/05/23 07:15:52, Pawel Osciak wrote:
> I'm wondering if we need the casts?

Done.

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:699: 
On 2016/05/23 07:15:52, Pawel Osciak wrote:
> Do we need the temporary timestamp, could we just assign directly to
> qbuf.timestamp ?

Done.

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/vaapi_video_e...
File media/gpu/vaapi_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/vaapi_video_e...
media/gpu/vaapi_video_encode_accelerator.cc:560: encode_job->keyframe,
base::TimeDelta()));
On 2016/05/23 07:15:53, Pawel Osciak wrote:
> Could you implement this by adding timestamp to EncodeJob please?

Done.

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/video_encode_...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/video_encode_...
media/gpu/video_encode_accelerator_unittest.cc:1255: base::TimeDelta timestamp)
{
On 2016/05/23 07:15:53, Pawel Osciak wrote:
> Please add a test case that tests input timestamps passed to Encode() are
> properly propagated to BBR() (and in correct order).

Done.

https://codereview.chromium.org/1996453003/diff/40001/media/video/fake_video_...
File media/video/fake_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/media/video/fake_video_...
media/video/fake_video_encode_accelerator.cc:135: base::TimeDelta());
On 2016/05/23 07:15:53, Pawel Osciak wrote:
> Perhaps Time::Now() ?

Done.

https://codereview.chromium.org/1996453003/diff/40001/media/video/video_encod...
File media/video/video_encode_accelerator.h (right):

https://codereview.chromium.org/1996453003/diff/40001/media/video/video_encod...
media/video/video_encode_accelerator.h:81: //  |key_frame| is true if this
delivered frame is a keyframe.
On 2016/05/19 20:27:12, sandersd wrote:
> Document |timestamp|.

Done.

https://codereview.chromium.org/1996453003/diff/40001/media/video/video_encod...
media/video/video_encode_accelerator.h:85: base::TimeDelta timestamp) = 0;
On 2016/05/23 07:15:53, Pawel Osciak wrote:
> Perhaps const TimeDelta& here and in other places?

All time classes are copyable, assignable, and occupy 64-bits per instance.
Thus, they can be efficiently passed by-value (as opposed to
by-reference)
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&q...

[pbos, 2016-05-24 13:02:21]

lgtm, thanks a lot

[pbos, 2016-05-24 13:02:36]

On 2016/05/24 13:02:21, pbos wrote:
> lgtm, thanks a lot

(I only checked RTCVideoEncoder.)

[Pawel Osciak, 2016-05-26 07:39:24]

https://chromiumcodereview.appspot.com/1996453003/diff/40001/media/gpu/v4l2_v...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1996453003/diff/40001/media/gpu/v4l2_v...
media/gpu/v4l2_video_encode_accelerator.cc:669:
static_cast<int64_t>(dqbuf.timestamp.tv_usec))));
On 2016/05/24 10:50:19, shenghao wrote:
> On 2016/05/23 07:15:53, Pawel Osciak wrote:
> > Do we need the cast? Also, we need both tv_sec and tv_usec parts of
> > dqbuf.timestamp to recreate the original timestamp?
> 
> Removed the cast. I think we only need the usec to recreate the original
> timestamp

We need both, usec only stores < 1s part of the value.

https://chromiumcodereview.appspot.com/1996453003/diff/80001/media/gpu/vaapi_...
File media/gpu/vaapi_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1996453003/diff/80001/media/gpu/vaapi_...
media/gpu/vaapi_video_encode_accelerator.cc:590:
encoder_input_queue_.front()->frame->InMicroseconds();
I would prefer not accessing encoder_input_queue_ from here for cleaner code (no
dependency between pop() in EncodeFrameTask() and this front()). Perhaps we
could pass timestamp as an argument to this method.

Also, I think you meant frame->timestamp().InMicroseconds()?

But I'd prefer if you just stored base::TimeDelta, i.e. frame->timestamp() in
EncodeJob, to be more portable.

https://chromiumcodereview.appspot.com/1996453003/diff/80001/media/gpu/video_...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/1996453003/diff/80001/media/gpu/video_...
media/gpu/video_encode_accelerator_unittest.cc:1000: unsigned int
output_frame_count_;
Perhaps we could use a queue for frame_timestamps_ and just pop each time,
without having to maintain the counter?

https://chromiumcodereview.appspot.com/1996453003/diff/80001/media/video/fake...
File media/video/fake_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1996453003/diff/80001/media/video/fake...
media/video/fake_video_encode_accelerator.cc:136:
base::TimeTicks::Now().ToInternalValue()));
base::Time::Now() - base::Time()

https://chromiumcodereview.appspot.com/1996453003/diff/80001/media/video/vide...
File media/video/video_encode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1996453003/diff/80001/media/video/vide...
media/video/video_encode_accelerator.h:82: //  |timestamp| is the captured NTP
timestamp in milliseconds.
TimeDelta is not necessarily in milliseconds, nor it has to be NTP. We should
just say that this is the same timestamp as the timestamp in VideoFrame passed
to Encode() please.

[shenghao, 2016-05-26 10:38:20]

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1996453003/diff/40001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:669:
static_cast<int64_t>(dqbuf.timestamp.tv_usec))));
On 2016/05/26 07:39:23, Pawel Osciak wrote:
> On 2016/05/24 10:50:19, shenghao wrote:
> > On 2016/05/23 07:15:53, Pawel Osciak wrote:
> > > Do we need the cast? Also, we need both tv_sec and tv_usec parts of
> > > dqbuf.timestamp to recreate the original timestamp?
> > 
> > Removed the cast. I think we only need the usec to recreate the original
> > timestamp
> 
> We need both, usec only stores < 1s part of the value.

Done.

https://codereview.chromium.org/1996453003/diff/80001/media/gpu/vaapi_video_e...
File media/gpu/vaapi_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1996453003/diff/80001/media/gpu/vaapi_video_e...
media/gpu/vaapi_video_encode_accelerator.cc:590:
encoder_input_queue_.front()->frame->InMicroseconds();
On 2016/05/26 07:39:23, Pawel Osciak wrote:
> I would prefer not accessing encoder_input_queue_ from here for cleaner code
(no
> dependency between pop() in EncodeFrameTask() and this front()). Perhaps we
> could pass timestamp as an argument to this method.
> 
> Also, I think you meant frame->timestamp().InMicroseconds()?
> 
> But I'd prefer if you just stored base::TimeDelta, i.e. frame->timestamp() in
> EncodeJob, to be more portable.

Done.

https://codereview.chromium.org/1996453003/diff/80001/media/gpu/video_encode_...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1996453003/diff/80001/media/gpu/video_encode_...
media/gpu/video_encode_accelerator_unittest.cc:1000: unsigned int
output_frame_count_;
On 2016/05/26 07:39:23, Pawel Osciak wrote:
> Perhaps we could use a queue for frame_timestamps_ and just pop each time,
> without having to maintain the counter?

Done.

https://codereview.chromium.org/1996453003/diff/80001/media/video/fake_video_...
File media/video/fake_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1996453003/diff/80001/media/video/fake_video_...
media/video/fake_video_encode_accelerator.cc:136:
base::TimeTicks::Now().ToInternalValue()));
On 2016/05/26 07:39:23, Pawel Osciak wrote:
> base::Time::Now() - base::Time()

Very clever!

https://codereview.chromium.org/1996453003/diff/80001/media/video/video_encod...
File media/video/video_encode_accelerator.h (right):

https://codereview.chromium.org/1996453003/diff/80001/media/video/video_encod...
media/video/video_encode_accelerator.h:82: //  |timestamp| is the captured NTP
timestamp in milliseconds.
On 2016/05/26 07:39:23, Pawel Osciak wrote:
> TimeDelta is not necessarily in milliseconds, nor it has to be NTP. We should
> just say that this is the same timestamp as the timestamp in VideoFrame passed
> to Encode() please.

Done.

[Pawel Osciak, 2016-05-27 04:46:39]

lgtm % nits please
also, please update the TEST= line

https://codereview.chromium.org/1996453003/diff/120001/media/gpu/v4l2_video_e...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1996453003/diff/120001/media/gpu/v4l2_video_e...
media/gpu/v4l2_video_encode_accelerator.cc:669:
base::TimeDelta::FromMicroseconds(
Perhaps we could use Time::FromTimeVal()...

https://codereview.chromium.org/1996453003/diff/120001/media/gpu/v4l2_video_e...
media/gpu/v4l2_video_encode_accelerator.cc:696: qbuf.timestamp.tv_sec =
static_cast<time_t>(frame->timestamp().InSeconds());
... and Time::ToTimeVal().

https://codereview.chromium.org/1996453003/diff/120001/media/gpu/vaapi_video_...
File media/gpu/vaapi_video_encode_accelerator.h (right):

https://codereview.chromium.org/1996453003/diff/120001/media/gpu/vaapi_video_...
media/gpu/vaapi_video_encode_accelerator.h:76: // Captured timestamp.
Nit: this doesn't have to be "captured". Let's say "source timestamp" please.

https://codereview.chromium.org/1996453003/diff/120001/media/video/video_enco...
File media/video/video_encode_accelerator.h (right):

https://codereview.chromium.org/1996453003/diff/120001/media/video/video_enco...
media/video/video_encode_accelerator.h:82: //  |timestamp| is the same timestamp
VideoFrame passed to Encode().
s/timestamp VideoFrame/timestamp as in VideoFrame/

[shenghao, 2016-05-30 09:18:02]

https://codereview.chromium.org/1996453003/diff/120001/media/gpu/v4l2_video_e...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1996453003/diff/120001/media/gpu/v4l2_video_e...
media/gpu/v4l2_video_encode_accelerator.cc:669:
base::TimeDelta::FromMicroseconds(
On 2016/05/27 04:46:38, Pawel Osciak wrote:
> Perhaps we could use Time::FromTimeVal()...

Talked offline. Time::FromTimeVal() does not fit our use case here.

https://codereview.chromium.org/1996453003/diff/120001/media/gpu/v4l2_video_e...
media/gpu/v4l2_video_encode_accelerator.cc:696: qbuf.timestamp.tv_sec =
static_cast<time_t>(frame->timestamp().InSeconds());
On 2016/05/27 04:46:38, Pawel Osciak wrote:
> ... and Time::ToTimeVal().

Talked offline. Time::ToTimeVal() does not fit our use case here.

https://codereview.chromium.org/1996453003/diff/120001/media/gpu/vaapi_video_...
File media/gpu/vaapi_video_encode_accelerator.h (right):

https://codereview.chromium.org/1996453003/diff/120001/media/gpu/vaapi_video_...
media/gpu/vaapi_video_encode_accelerator.h:76: // Captured timestamp.
On 2016/05/27 04:46:38, Pawel Osciak wrote:
> Nit: this doesn't have to be "captured". Let's say "source timestamp" please.

Done.

https://codereview.chromium.org/1996453003/diff/120001/media/video/video_enco...
File media/video/video_encode_accelerator.h (right):

https://codereview.chromium.org/1996453003/diff/120001/media/video/video_enco...
media/video/video_encode_accelerator.h:82: //  |timestamp| is the same timestamp
VideoFrame passed to Encode().
On 2016/05/27 04:46:38, Pawel Osciak wrote:
> s/timestamp VideoFrame/timestamp as in VideoFrame/

Done.

[shenghao, 2016-05-30 09:18:09]

The CQ bit was checked by shenghao@chromium.org

[shenghao, 2016-05-30 09:18:10]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
sandersd@chromium.org, wfh@chromium.org, pbos@chromium.org, posciak@chromium.org
Link to the patchset: https://codereview.chromium.org/1996453003/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-05-30 09:18:24]

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

[commit-bot: I haz the power, 2016-05-30 09:28:49]

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

[commit-bot: I haz the power, 2016-05-30 09:28:51]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[shenghao, 2016-05-30 10:12:55]

The CQ bit was checked by shenghao@chromium.org

[shenghao, 2016-05-30 10:12:56]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
sandersd@chromium.org, pbos@chromium.org, posciak@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1996453003/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-30 10:13:05]

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

[commit-bot: I haz the power, 2016-05-30 10:24:52]

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

[commit-bot: I haz the power, 2016-05-30 10:24:54]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[shenghao, 2016-05-31 03:43:09]

The CQ bit was checked by shenghao@chromium.org

[shenghao, 2016-05-31 03:43:09]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
sandersd@chromium.org, pbos@chromium.org, posciak@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1996453003/#ps200001
(title: " ")

[commit-bot: I haz the power, 2016-05-31 03:43:24]

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

[commit-bot: I haz the power, 2016-05-31 04:59:48]

Description was changed from

==========
RTC Video Encoder: Use capturer timestamp

Add the capturer timestamp as a parameter in VEA::BitstreamBufferReady() so
RTCVideoEncoder can use it.

BUG=350106
TEST=Verify that apprtc loopback works on minnie.
R=bbudge@chromium.org, posciak@chromium.org, sandersd@chromium.org,
wfh@chromium.org, wuchengli@chromium.org
==========

to

==========
RTC Video Encoder: Use capturer timestamp

Add the capturer timestamp as a parameter in VEA::BitstreamBufferReady() so
RTCVideoEncoder can use it.

BUG=350106
TEST=Verify that apprtc loopback works on minnie.
R=bbudge@chromium.org, posciak@chromium.org, sandersd@chromium.org,
wfh@chromium.org, wuchengli@chromium.org
==========

[commit-bot: I haz the power, 2016-05-31 04:59:50]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-05-31 05:02:22]

Description was changed from

==========
RTC Video Encoder: Use capturer timestamp

Add the capturer timestamp as a parameter in VEA::BitstreamBufferReady() so
RTCVideoEncoder can use it.

BUG=350106
TEST=Verify that apprtc loopback works on minnie.
R=bbudge@chromium.org, posciak@chromium.org, sandersd@chromium.org,
wfh@chromium.org, wuchengli@chromium.org
==========

to

==========
RTC Video Encoder: Use capturer timestamp

Add the capturer timestamp as a parameter in VEA::BitstreamBufferReady() so
RTCVideoEncoder can use it.

BUG=350106
TEST=Verify that apprtc loopback works on minnie.
R=bbudge@chromium.org, posciak@chromium.org, sandersd@chromium.org,
wfh@chromium.org, wuchengli@chromium.org

Committed: https://crrev.com/5d967288829b022d533d839e4883902858615682
Cr-Commit-Position: refs/heads/master@{#396786}
==========

[commit-bot: I haz the power, 2016-05-31 05:02:24]

Patchset 11 (id:??) landed as
https://crrev.com/5d967288829b022d533d839e4883902858615682
Cr-Commit-Position: refs/heads/master@{#396786}

[wuchengli, 2016-06-17 05:16:40]

Patchset #12 (id:220001) has been deleted