Use webrtc::VideoFrame timestamp in RTCVideoEncoder

Use webrtc::VideoFrame timestamp in RTCVideoEncoder

This CL fixes input timestamp mismatch in RTCVideoEncoder, which
broke googAvgEncodeMs and googEncodeUsagePercent stats in webrtc-internals
for hardware encoders.
With this change, we start using WebRTC given timestamp() so that
OveruseFrameDetector can match the timestamps and calculate the stats.

BUG=597087
TEST=googAvgEncodeMs and googEncodeUsagePercent works on Mac(H264) and
veyron_jerry(VP8).

Committed: https://crrev.com/e3195490a63d9545fb1bfe560aa21680ba0b5843
Cr-Commit-Position: refs/heads/master@{#414589}

[emircan, 2016-08-01 23:10:42]

Patchset #1 (id:1) has been deleted

[emircan, 2016-08-01 23:17:25]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2016-08-01 23:17:34]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-01 23:17:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-08-01 23:23:36]

Description was changed from

==========
fix timestamp mismatch

BUG=
==========

to

==========
Fix input timestamp mismatch in RTCVideoEncoder

This CL fixes input timestamp mismatch in RTCVideoEncoder, which
broke googAvgEncodeMs and googEncodeUsagePercent stats in webrtc-internals
for hardware encoders.
With this change, we start using WebRTC given timestamp() so that  
OveruseFrameDetector can match the timestamps and calculate the stats.

BUG=597087
TEST=googAvgEncodeMs and googEncodeUsagePercent works on Mac(H264) and
veyron_jerry(VP8).
==========

[emircan, 2016-08-01 23:23:36]

emircan@chromium.org changed reviewers:
+ pbos@chromium.org, wuchengli@chromium.org

[emircan, 2016-08-01 23:24:17]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2016-08-01 23:25:31]

Description was changed from

==========
Fix input timestamp mismatch in RTCVideoEncoder

This CL fixes input timestamp mismatch in RTCVideoEncoder, which
broke googAvgEncodeMs and googEncodeUsagePercent stats in webrtc-internals
for hardware encoders.
With this change, we start using WebRTC given timestamp() so that  
OveruseFrameDetector can match the timestamps and calculate the stats.

BUG=597087
TEST=googAvgEncodeMs and googEncodeUsagePercent works on Mac(H264) and
veyron_jerry(VP8).
==========

to

==========
Use webrtc::VideoFrame timestamp in RTCVideoEncoder

This CL fixes input timestamp mismatch in RTCVideoEncoder, which
broke googAvgEncodeMs and googEncodeUsagePercent stats in webrtc-internals
for hardware encoders.
With this change, we start using WebRTC given timestamp() so that  
OveruseFrameDetector can match the timestamps and calculate the stats.

BUG=597087
TEST=googAvgEncodeMs and googEncodeUsagePercent works on Mac(H264) and
veyron_jerry(VP8).
==========

[emircan, 2016-08-01 23:25:43]

PTAL.

[pbos, 2016-08-02 00:15:52]

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

https://codereview.chromium.org/2205623002/diff/40001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_encoder.cc:614: next_frame->timestamp() *
base::Time::kMicrosecondsPerMillisecond /
I think you can use base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms())
here, there isn't currently more precision in the RTP timestamp either way.

[pbos, 2016-08-02 00:16:52]

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

https://codereview.chromium.org/2205623002/diff/40001/media/gpu/video_encode_...
media/gpu/video_encode_accelerator_unittest.cc:1578: #if defined(OS_CHROMEOS)
Can you comment on why this would/should only apply to ChromeOS?

[emircan, 2016-08-02 21:58:01]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 21:58:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-08-02 21:59:23]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2016-08-02 21:59:32]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-08-02 22:00:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-08-02 22:05:40]

Re VerifyTimestamp test, I found out that we counted H264 encoded frames twice
which caused an issue on Mac. We iterate through the nal units and call
|frame_cb_| multiple times[0] which is VEAClient::HandleEncodedFrame. Inside the
function, we count each nal unit as a frame and as a result have mismatch in
comparison. I tried to fix it buy seperately counting |num_encoded_nalus_|.
wuchengli@ let me know if you have any other suggestion.

[0]
https://cs.chromium.org/chromium/src/media/gpu/video_encode_accelerator_unitt...

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

https://codereview.chromium.org/2205623002/diff/40001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_encoder.cc:614: next_frame->timestamp() *
base::Time::kMicrosecondsPerMillisecond /
On 2016/08/02 00:15:52, pbos wrote:
> I think you can use
base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms())
> here, there isn't currently more precision in the RTP timestamp either way.

next_frame->ntp_time_ms() is 0 according to the logs. timestamp() is set in all
cases though.

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

https://codereview.chromium.org/2205623002/diff/40001/media/gpu/video_encode_...
media/gpu/video_encode_accelerator_unittest.cc:1578: #if defined(OS_CHROMEOS)
On 2016/08/02 00:16:52, pbos wrote:
> Can you comment on why this would/should only apply to ChromeOS?

I removed ifdef and explained above.

[commit-bot: I haz the power, 2016-08-03 00:46:33]

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

[commit-bot: I haz the power, 2016-08-03 00:46:34]

Dry run: This issue passed the CQ dry run.

[wuchengli, 2016-08-03 14:03:57]

https://codereview.chromium.org/2205623002/diff/80001/content/renderer/media/...
File content/renderer/media/gpu/rtc_video_encoder.cc (right):

https://codereview.chromium.org/2205623002/diff/80001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_encoder.cc:571:
next_frame->video_frame_buffer()->native_handle());
What's the difference between these three timestamps?
(1) next_frame->timestamp()
(2) next_frame->ntp_time_ms()
(3) frame->timestamp(), the timestamp of native handle

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

https://codereview.chromium.org/2205623002/diff/80001/media/gpu/video_encode_...
media/gpu/video_encode_accelerator_unittest.cc:1543: num_encoded_frames_ =
num_encoded_nalus_ / 2;
Hmm. Maybe we should increase num_encoded_frames_ only when BitstreamBufferReady
returns a timestamp different from the last one. Let me discuss with Pawel
tomorrow.

[pbos, 2016-08-03 16:11:30]

https://codereview.chromium.org/2205623002/diff/80001/content/renderer/media/...
File content/renderer/media/gpu/rtc_video_encoder.cc (right):

https://codereview.chromium.org/2205623002/diff/80001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_encoder.cc:571:
next_frame->video_frame_buffer()->native_handle());
On 2016/08/03 14:03:57, wuchengli wrote:
> What's the difference between these three timestamps?
> (1) next_frame->timestamp()
> (2) next_frame->ntp_time_ms()
> (3) frame->timestamp(), the timestamp of native handle

(1) is the RTP timestamp (without random offsetting), which with the current
code is 90*ntp_time_ms.
(2) NTP time of frame capture, according to WebRTC.
(3) Other capture timestamp, taken by Chromium somewhere I guess?

WebRTC needs outgoing (1) to match incoming (1) for CPU adaptation purposes,
that's how we match incoming with outgoing frames.

[emircan, 2016-08-03 22:22:49]

https://codereview.chromium.org/2205623002/diff/80001/content/renderer/media/...
File content/renderer/media/gpu/rtc_video_encoder.cc (right):

https://codereview.chromium.org/2205623002/diff/80001/content/renderer/media/...
content/renderer/media/gpu/rtc_video_encoder.cc:571:
next_frame->video_frame_buffer()->native_handle());
On 2016/08/03 16:11:30, pbos wrote:
> On 2016/08/03 14:03:57, wuchengli wrote:
> > What's the difference between these three timestamps?
> > (1) next_frame->timestamp()
> > (2) next_frame->ntp_time_ms()
> > (3) frame->timestamp(), the timestamp of native handle
> 
> (1) is the RTP timestamp (without random offsetting), which with the current
> code is 90*ntp_time_ms.
> (2) NTP time of frame capture, according to WebRTC.
> (3) Other capture timestamp, taken by Chromium somewhere I guess?
> 
> WebRTC needs outgoing (1) to match incoming (1) for CPU adaptation purposes,
> that's how we match incoming with outgoing frames.

In short, I can say we are interested in (1).
(1) It is in 90khz. As pbos@ pointed out, WebRTC needs to match (1) to calculate
stats. It is always set in webrtc::VideoFrame in ctor[0].
(2) It is (1) converted from 90kHz to ms[1]. It might not be set in
webrtc::VideoFrame[2].
(3) Chromium timestamp. It gets modified within WebRTC[1].    

[0]
https://cs.chromium.org/chromium/src/third_party/webrtc/common_video/video_fr...
[1]
https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_capture_i...
[2]
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...

[wuchengli, 2016-08-05 02:10:29]

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

https://codereview.chromium.org/2205623002/diff/80001/media/gpu/video_encode_...
media/gpu/video_encode_accelerator_unittest.cc:1543: num_encoded_frames_ =
num_encoded_nalus_ / 2;
On 2016/08/03 14:03:57, wuchengli wrote:
> Hmm. Maybe we should increase num_encoded_frames_ only when
BitstreamBufferReady
> returns a timestamp different from the last one. Let me discuss with Pawel
> tomorrow.
I discussed with Pawel. Normally we get one slice NAL. Sometimes we may get
multiple. But in general this may be any number of NALUs. We can't make any
assumptions about this. For example, we could get 20 type 6 NALUs or 10 SPSes in
a row. All is valid and up to spec.

The solution is we should only increase num_encoded_frames_ when the timestamps
of BitstreamBufferReady is different from the last one. In
VEAClient::CreateFrame (line 1433), we make sure the timestamps of every input
buffer is different. We can count the number of timestamps of
BitstreamBufferReady to know num_encoded_frames_.

The increment of num_encoded_frames_ should be moved to
VEAClient::BitstreamBufferReady because we have timetsamps information there.

[emircan, 2016-08-09 21:55:42]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 21:56:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-08-09 22:00:37]

Patchset #3 (id:100001) has been deleted

[emircan, 2016-08-09 22:05:01]

On 2016/08/05 02:10:29, wuchengli wrote:
>
https://codereview.chromium.org/2205623002/diff/80001/media/gpu/video_encode_...
> File media/gpu/video_encode_accelerator_unittest.cc (right):
> 
>
https://codereview.chromium.org/2205623002/diff/80001/media/gpu/video_encode_...
> media/gpu/video_encode_accelerator_unittest.cc:1543: num_encoded_frames_ =
> num_encoded_nalus_ / 2;
> On 2016/08/03 14:03:57, wuchengli wrote:
> > Hmm. Maybe we should increase num_encoded_frames_ only when
> BitstreamBufferReady
> > returns a timestamp different from the last one. Let me discuss with Pawel
> > tomorrow.
> I discussed with Pawel. Normally we get one slice NAL. Sometimes we may get
> multiple. But in general this may be any number of NALUs. We can't make any
> assumptions about this. For example, we could get 20 type 6 NALUs or 10 SPSes
in
> a row. All is valid and up to spec.
> 
> The solution is we should only increase num_encoded_frames_ when the
timestamps
> of BitstreamBufferReady is different from the last one. In
> VEAClient::CreateFrame (line 1433), we make sure the timestamps of every input
> buffer is different. We can count the number of timestamps of
> BitstreamBufferReady to know num_encoded_frames_.
> 
> The increment of num_encoded_frames_ should be moved to
> VEAClient::BitstreamBufferReady because we have timetsamps information there.

Thanks. 

Moving counting logic inside VEAClient::BitstreamBufferReady() isn't enough
since H264Validator::ProcessStreamBuffer would still call HandleEncodedFrames()
which is |frame_cb_| multiple times. I need to change most of the functions
called there.

Instead what I suggest is calling HandleEncodedFrames() only once per frame from
H264Validator::ProcessStreamBuffer() as I did in the new patch. WDYT?

[commit-bot: I haz the power, 2016-08-09 23:55:20]

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

[commit-bot: I haz the power, 2016-08-09 23:55:21]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[emircan, 2016-08-10 17:24:36]

emircan@chromium.org changed reviewers:
+ posciak@chromium.org

[emircan, 2016-08-10 17:24:52]

Friendly PING.

[pbos, 2016-08-10 17:56:52]

https://codereview.chromium.org/2205623002/diff/120001/content/renderer/media...
File content/renderer/media/gpu/rtc_video_encoder.cc (right):

https://codereview.chromium.org/2205623002/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:614: next_frame->timestamp() *
base::Time::kMicrosecondsPerMillisecond /
Does this have wrapping issues? timestamp() is a 32bit right? so timestamp() *
1000 should wrap, should this be cast to a int64_t before being passed to
FromMicroseconds?

[emircan, 2016-08-10 22:11:01]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2016-08-10 22:11:52]

https://codereview.chromium.org/2205623002/diff/120001/content/renderer/media...
File content/renderer/media/gpu/rtc_video_encoder.cc (right):

https://codereview.chromium.org/2205623002/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:614: next_frame->timestamp() *
base::Time::kMicrosecondsPerMillisecond /
On 2016/08/10 17:56:52, pbos wrote:
> Does this have wrapping issues? timestamp() is a 32bit right? so timestamp() *
> 1000 should wrap, should this be cast to a int64_t before being passed to
> FromMicroseconds?

Thanks for pointing this out. Actually we have problems even with int64_t for
some numeric cases.

Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we set Chrome's
frame->timestamp() in microseconds as (30000/90*1000)= 333333.333333. This drops
the decimal bits. Even if we use FromMillisecondsD(), we fail to cover all the
information. As a result, we may not get the same timestamp after converting
back, which results in random jumps in googEncodeMs. 

Instead, I changed it to use base::TimeDelta::FromInternalValue() ctor and keep
it in 90 kHZ. This removes the convertions overall.

[commit-bot: I haz the power, 2016-08-10 22:12:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-08-10 22:12:48]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2016-08-10 22:13:27]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2016-08-10 22:13:44]

Patchset #4 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-08-10 22:14:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 01:18:48]

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

[commit-bot: I haz the power, 2016-08-11 01:18:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wuchengli, 2016-08-11 06:06:04]

Pawel is OOO today. I'll check with him about the comment in
H264Validator::ProcessStreamBuffer tomorrow.

https://codereview.chromium.org/2205623002/diff/160001/content/renderer/media...
File content/renderer/media/gpu/rtc_video_encoder.cc (right):

https://codereview.chromium.org/2205623002/diff/160001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:489: rtp_timestamp =
capture_time_ms * kMsToRtpTimestamp;
We should multiply by 90 first to have the higher precision.
rtp_timestamp = rtc::TimeMicros() * kMsToRtpTimestamp /
base::Time::kMicrosecondsPerMillisecond;

https://codereview.chromium.org/2205623002/diff/160001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:497: image._timeStamp =
static_cast<int32_t>(rtp_timestamp);
The type of _timeStamp is uint32_t.
https://cs.chromium.org/chromium/src/third_party/webrtc/video_frame.h?q=Encod...

https://codereview.chromium.org/2205623002/diff/160001/media/gpu/video_encode...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2205623002/diff/160001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:588: // fallthrough
This is fallthough. If there's only one kIDRSlice in the buffer, it's strange to
set |seen_nonidr| to true when actually only idr is seen.

https://codereview.chromium.org/2205623002/diff/160001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:593: if
(!frame_cb_.Run(keyframe))
If we only call frame_cb_ once, we should probably do it after switch clause in
line 617. So we can pass keyframe==true if there's kIDRSlice. Otherwise, suppose
the first NALU is kNonIDRSlice and the second NALU is kIDRSlice. We will call
|frame_cb_| with false in that case. But I'd like to check with Pawel. He's OOO
today and will be back tomorrow. Let me get back to you tomorrow.

[emircan, 2016-08-11 16:48:00]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 16:48:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-08-11 16:49:15]

https://codereview.chromium.org/2205623002/diff/160001/content/renderer/media...
File content/renderer/media/gpu/rtc_video_encoder.cc (right):

https://codereview.chromium.org/2205623002/diff/160001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:489: rtp_timestamp =
capture_time_ms * kMsToRtpTimestamp;
On 2016/08/11 06:06:03, wuchengli wrote:
> We should multiply by 90 first to have the higher precision.
> rtp_timestamp = rtc::TimeMicros() * kMsToRtpTimestamp /
> base::Time::kMicrosecondsPerMillisecond;

Done.

https://codereview.chromium.org/2205623002/diff/160001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:497: image._timeStamp =
static_cast<int32_t>(rtp_timestamp);
On 2016/08/11 06:06:03, wuchengli wrote:
> The type of _timeStamp is uint32_t.
>
https://cs.chromium.org/chromium/src/third_party/webrtc/video_frame.h?q=Encod...

Done.

https://codereview.chromium.org/2205623002/diff/160001/media/gpu/video_encode...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2205623002/diff/160001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:593: if
(!frame_cb_.Run(keyframe))
On 2016/08/11 06:06:04, wuchengli wrote:
> If we only call frame_cb_ once, we should probably do it after switch clause
in
> line 617. So we can pass keyframe==true if there's kIDRSlice. Otherwise,
suppose
> the first NALU is kNonIDRSlice and the second NALU is kIDRSlice. We will call
> |frame_cb_| with false in that case. But I'd like to check with Pawel. He's
OOO
> today and will be back tomorrow. Let me get back to you tomorrow.

I thought of that case, but |seen_idr_| is only set when kIDRSlice is seen and
we have an assert here. Therefore, it should be fine to assume kNonIDRSlice does
not come before kIDRSlice. WDYT?

[commit-bot: I haz the power, 2016-08-11 17:05:38]

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

[commit-bot: I haz the power, 2016-08-11 17:05:39]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[emircan, 2016-08-11 18:36:10]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2016-08-11 18:36:28]

Patchset #5 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-08-11 18:36:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 18:50:21]

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

[commit-bot: I haz the power, 2016-08-11 18:50:22]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[wuchengli, 2016-08-12 15:12:25]

https://codereview.chromium.org/2205623002/diff/160001/media/gpu/video_encode...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2205623002/diff/160001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:590: ASSERT_TRUE(seen_idr_);
Please add some comments to explain the assumption here. We assume one
BitstreamBufferReady can contain zero or one frame, not no more than one. So we
only call frame callback once.

FYI. Pawel suggested we could follow the code in
H264Decoder::IsNewPrimaryCodedPicture (#0) to decide if a slice is the start of
a new frame. That will solve the case when one BitstreamBufferReady contains
multiple slices or multiple frame. I think it's complicated and it's ok to just
have the assumption.

#0:
https://cs.chromium.org/chromium/src/media/gpu/h264_decoder.cc?rcl=0&l=1191

https://codereview.chromium.org/2205623002/diff/160001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:593: if
(!frame_cb_.Run(keyframe))
On 2016/08/11 16:49:15, emircan wrote:
> On 2016/08/11 06:06:04, wuchengli wrote:
> > If we only call frame_cb_ once, we should probably do it after switch clause
> in
> > line 617. So we can pass keyframe==true if there's kIDRSlice. Otherwise,
> suppose
> > the first NALU is kNonIDRSlice and the second NALU is kIDRSlice. We will
call
> > |frame_cb_| with false in that case. But I'd like to check with Pawel. He's
> OOO
> > today and will be back tomorrow. Let me get back to you tomorrow.
> 
> I thought of that case, but |seen_idr_| is only set when kIDRSlice is seen and
> we have an assert here. Therefore, it should be fine to assume kNonIDRSlice
does
> not come before kIDRSlice. WDYT?
Yes. You are right. I didn't see the assertion.

Suppose the firts NALU is kIDRSlice and the second is kNonIDRSlice.
|seen_nonidr| will be true after the first NALU, but nonidr is not seen. How
about renaming |seen_nonidr| to |seen_slice| or |first_slice| or
|frame_cb_called|?

https://codereview.chromium.org/2205623002/diff/200001/media/gpu/video_encode...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2205623002/diff/200001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:590: ASSERT_TRUE(seen_idr_);
You accidentally reverted the code?
if (!seen_nonidr) {
  seen_nonidr = true;
  if (!frame_cb_.Run(keyframe))
    return;
}

[emircan, 2016-08-24 21:27:49]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2016-08-24 21:28:24]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 21:28:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-08-24 21:29:17]

PTAL. Sorry for the delay. I was OOO last week.

https://codereview.chromium.org/2205623002/diff/160001/media/gpu/video_encode...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2205623002/diff/160001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:590: ASSERT_TRUE(seen_idr_);
On 2016/08/12 15:12:24, wuchengli wrote:
> Please add some comments to explain the assumption here. We assume one
> BitstreamBufferReady can contain zero or one frame, not no more than one. So
we
> only call frame callback once.
> 
> FYI. Pawel suggested we could follow the code in
> H264Decoder::IsNewPrimaryCodedPicture (#0) to decide if a slice is the start
of
> a new frame. That will solve the case when one BitstreamBufferReady contains
> multiple slices or multiple frame. I think it's complicated and it's ok to
just
> have the assumption.
> 
> #0:
> https://cs.chromium.org/chromium/src/media/gpu/h264_decoder.cc?rcl=0&l=1191

I am adding a comment about the assumption.

https://codereview.chromium.org/2205623002/diff/160001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:593: if
(!frame_cb_.Run(keyframe))
On 2016/08/12 15:12:25, wuchengli wrote:
> On 2016/08/11 16:49:15, emircan wrote:
> > On 2016/08/11 06:06:04, wuchengli wrote:
> > > If we only call frame_cb_ once, we should probably do it after switch
clause
> > in
> > > line 617. So we can pass keyframe==true if there's kIDRSlice. Otherwise,
> > suppose
> > > the first NALU is kNonIDRSlice and the second NALU is kIDRSlice. We will
> call
> > > |frame_cb_| with false in that case. But I'd like to check with Pawel.
He's
> > OOO
> > > today and will be back tomorrow. Let me get back to you tomorrow.
> > 
> > I thought of that case, but |seen_idr_| is only set when kIDRSlice is seen
and
> > we have an assert here. Therefore, it should be fine to assume kNonIDRSlice
> does
> > not come before kIDRSlice. WDYT?
> Yes. You are right. I didn't see the assertion.
> 
> Suppose the firts NALU is kIDRSlice and the second is kNonIDRSlice.
> |seen_nonidr| will be true after the first NALU, but nonidr is not seen. How
> about renaming |seen_nonidr| to |seen_slice| or |first_slice| or
> |frame_cb_called|?

I agree |frame_cb_called| sounds better. Done.

https://codereview.chromium.org/2205623002/diff/200001/media/gpu/video_encode...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2205623002/diff/200001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:590: ASSERT_TRUE(seen_idr_);
On 2016/08/12 15:12:25, wuchengli wrote:
> You accidentally reverted the code?
> if (!seen_nonidr) {
>   seen_nonidr = true;
>   if (!frame_cb_.Run(keyframe))
>     return;
> }

Done.

[commit-bot: I haz the power, 2016-08-24 23:18:05]

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

[commit-bot: I haz the power, 2016-08-24 23:18:06]

Dry run: This issue passed the CQ dry run.

[wuchengli, 2016-08-25 06:15:30]

lgtm

[emircan, 2016-08-25 06:28:46]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2016-08-25 06:29:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 06:35:25]

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

[commit-bot: I haz the power, 2016-08-25 06:35:27]

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

[emircan, 2016-08-25 15:28:18]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2016-08-25 15:28:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 15:35:38]

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

[commit-bot: I haz the power, 2016-08-25 15:35:40]

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

[emircan, 2016-08-25 18:04:17]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[emircan, 2016-08-25 18:04:50]

Patchset #7 (id:240001) has been deleted

[commit-bot: I haz the power, 2016-08-25 18:05:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-08-25 18:09:33]

The CQ bit was checked by emircan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 18:10:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emircan, 2016-08-25 19:51:04]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2016-08-25 19:51:11]

The CQ bit was checked by emircan@chromium.org

[emircan, 2016-08-25 19:51:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
wuchengli@chromium.org
Link to the patchset: https://codereview.chromium.org/2205623002/#ps260001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-08-25 19:52:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 22:22:53]

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

[commit-bot: I haz the power, 2016-08-25 22:22:55]

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

[emircan, 2016-08-25 22:41:01]

The CQ bit was checked by emircan@chromium.org

[commit-bot: I haz the power, 2016-08-25 22:42:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 00:11:47]

Description was changed from

==========
Use webrtc::VideoFrame timestamp in RTCVideoEncoder

This CL fixes input timestamp mismatch in RTCVideoEncoder, which
broke googAvgEncodeMs and googEncodeUsagePercent stats in webrtc-internals
for hardware encoders.
With this change, we start using WebRTC given timestamp() so that  
OveruseFrameDetector can match the timestamps and calculate the stats.

BUG=597087
TEST=googAvgEncodeMs and googEncodeUsagePercent works on Mac(H264) and
veyron_jerry(VP8).
==========

to

==========
Use webrtc::VideoFrame timestamp in RTCVideoEncoder

This CL fixes input timestamp mismatch in RTCVideoEncoder, which
broke googAvgEncodeMs and googEncodeUsagePercent stats in webrtc-internals
for hardware encoders.
With this change, we start using WebRTC given timestamp() so that  
OveruseFrameDetector can match the timestamps and calculate the stats.

BUG=597087
TEST=googAvgEncodeMs and googEncodeUsagePercent works on Mac(H264) and
veyron_jerry(VP8).
==========

[commit-bot: I haz the power, 2016-08-26 00:11:49]

Committed patchset #7 (id:260001)

[commit-bot: I haz the power, 2016-08-26 00:13:09]

Description was changed from

==========
Use webrtc::VideoFrame timestamp in RTCVideoEncoder

This CL fixes input timestamp mismatch in RTCVideoEncoder, which
broke googAvgEncodeMs and googEncodeUsagePercent stats in webrtc-internals
for hardware encoders.
With this change, we start using WebRTC given timestamp() so that  
OveruseFrameDetector can match the timestamps and calculate the stats.

BUG=597087
TEST=googAvgEncodeMs and googEncodeUsagePercent works on Mac(H264) and
veyron_jerry(VP8).
==========

to

==========
Use webrtc::VideoFrame timestamp in RTCVideoEncoder

This CL fixes input timestamp mismatch in RTCVideoEncoder, which
broke googAvgEncodeMs and googEncodeUsagePercent stats in webrtc-internals
for hardware encoders.
With this change, we start using WebRTC given timestamp() so that
OveruseFrameDetector can match the timestamps and calculate the stats.

BUG=597087
TEST=googAvgEncodeMs and googEncodeUsagePercent works on Mac(H264) and
veyron_jerry(VP8).

Committed: https://crrev.com/e3195490a63d9545fb1bfe560aa21680ba0b5843
Cr-Commit-Position: refs/heads/master@{#414589}
==========

[commit-bot: I haz the power, 2016-08-26 00:13:11]

Patchset 7 (id:??) landed as
https://crrev.com/e3195490a63d9545fb1bfe560aa21680ba0b5843
Cr-Commit-Position: refs/heads/master@{#414589}

[Pawel Osciak, 2016-08-26 02:00:45]

https://codereview.chromium.org/2205623002/diff/260001/media/gpu/video_encode...
File media/gpu/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2205623002/diff/260001/media/gpu/video_encode...
media/gpu/video_encode_accelerator_unittest.cc:596: // Stream may contain at
most one frame.
If we require that, I think we should ASSERT on it, instead of the if() below,
which could hide a potential issue in the encoder (producing multiple frames per
buffer), which this validator should catch?

[emircan, 2016-08-31 17:53:24]

A revert of this CL (patchset #7 id:260001) has been created in
https://codereview.chromium.org/2296273002/ by emircan@chromium.org.

The reason for reverting is: This CL caused some regressions regarding BWE stats
and HW encoder
performance. Reasons include:
1) Modifying scoped_refptr<media::VideoFrame>'s timestamp causes problems
for other clients using the same media::VideoFrame.
2) WebRTC's RTP timestamp isn't suitable for using as presentation timestamp
in Mac and Win HW encoders.

BUG=641230.