Reland: Use webrtc::VideoFrame timestamp in RTCVideoEncoder

Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST=Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2435693004
Cr-Commit-Position: refs/heads/master@{#459908}
Committed: https://chromium.googlesource.com/chromium/src/+/2372de8b97b72d62e9ece294494a351ff633a2d8

[emircan, 2016-10-19 16:49:44]

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

[commit-bot: I haz the power, 2016-10-19 16:50: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-10-19 17:00:30]

Patchset #1 (id:1) has been deleted

[emircan, 2016-10-19 22:52:59]

Description was changed from

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

BUG=597087
==========

to

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

BUG=597087
==========

[emircan, 2016-10-19 22:58:19]

Description was changed from

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

BUG=597087
==========

to

==========
Reland: 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.

Note that we use base::TimeDelta::FromInternalValue() here:
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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. Therefore, we keep the value as internal in TimeDelta class.

BUG=597087
==========

[emircan, 2016-10-19 23:48:19]

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

[commit-bot: I haz the power, 2016-10-19 23:49:11]

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

[emircan, 2016-10-19 23:54:00]

Description was changed from

==========
Reland: 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.

Note that we use base::TimeDelta::FromInternalValue() here:
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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. Therefore, we keep the value as internal in TimeDelta class.

BUG=597087
==========

to

==========
Reland: 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 these stats.

Note that we use base::TimeDelta::FromInternalValue() in PS#1:
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as internal in TimeDelta class. However, this

TimeDelta causes issues in HW encoder as it cannot be used directly as 
presentation timestamp. Bitrate is visibly worse on Win&Mac using PS#1.

Therefore, I decided to hold onto presentation timestamp to RTP timestamp map
in RTCVideoEncoder and match values at the end, see PS#2.

BUG=597087
TEST-Tested AppRTC H264 loopback on Mac.
==========

[emircan, 2016-10-19 23:54:31]

Description was changed from

==========
Reland: 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 these stats.

Note that we use base::TimeDelta::FromInternalValue() in PS#1:
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as internal in TimeDelta class. However, this

TimeDelta causes issues in HW encoder as it cannot be used directly as 
presentation timestamp. Bitrate is visibly worse on Win&Mac using PS#1.

Therefore, I decided to hold onto presentation timestamp to RTP timestamp map
in RTCVideoEncoder and match values at the end, see PS#2.

BUG=597087
TEST-Tested AppRTC H264 loopback on Mac.
==========

to

==========
Reland: 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 these stats.

Note that we use base::TimeDelta::FromInternalValue() in PS#1:
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as internal in TimeDelta class. However, this

TimeDelta causes issues in HW encoder as it cannot be used directly as 
presentation timestamp. Bitrate is visibly worse on Win&Mac using PS#1.

Therefore, I decided to hold onto presentation timestamp to RTP timestamp map
in RTCVideoEncoder and match values at the end, see PS#2.

BUG=597087
TEST-Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
==========

[commit-bot: I haz the power, 2016-10-20 00:28:24]

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

[commit-bot: I haz the power, 2016-10-20 00:28:25]

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

[emircan, 2016-10-25 22:47:19]

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

[commit-bot: I haz the power, 2016-10-25 22:47:48]

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-10-25 22:50:13]

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

[commit-bot: I haz the power, 2016-10-25 22:50:17]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[emircan, 2016-10-26 07:02:03]

Patchset #4 (id:80001) has been deleted

[emircan, 2016-10-26 07:02:14]

Patchset #3 (id:60001) has been deleted

[emircan, 2017-01-10 20:26:22]

Patchset #1 (id:20001) has been deleted

[emircan, 2017-02-09 23:32:28]

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

[commit-bot: I haz the power, 2017-02-09 23:32:51]

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, 2017-02-09 23:57:45]

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

[commit-bot: I haz the power, 2017-02-09 23:57:46]

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

[pbos, 2017-02-10 00:05:07]

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

[pbos, 2017-02-10 00:05:08]

looks good overall, some nits

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

https://codereview.chromium.org/2435693004/diff/100001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:492: uint32_t rtp_timestamp = 0;
Can you make this optional?

https://codereview.chromium.org/2435693004/diff/100001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:498: while
(!pending_timestamps_.empty()) {
This should never be empty, right? If so there's a bug in the VEA?

https://codereview.chromium.org/2435693004/diff/100001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:507: }
Can you DCHECK after this brace that rtp_timestamp actually got set?

https://codereview.chromium.org/2435693004/diff/100001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:515: if (rtp_timestamp == 0) {
0 is a valid RTP timestamp. Can you gate this on whether rtp_timestamp is set or
not instead?

[emircan, 2017-02-10 00:16:54]

Description was changed from

==========
Reland: 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 these stats.

Note that we use base::TimeDelta::FromInternalValue() in PS#1:
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as internal in TimeDelta class. However, this

TimeDelta causes issues in HW encoder as it cannot be used directly as 
presentation timestamp. Bitrate is visibly worse on Win&Mac using PS#1.

Therefore, I decided to hold onto presentation timestamp to RTP timestamp map
in RTCVideoEncoder and match values at the end, see PS#2.

BUG=597087
TEST-Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
==========

to

==========
Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST-Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
==========

[emircan, 2017-02-10 00:17:04]

Description was changed from

==========
Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST-Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
==========

to

==========
Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST=Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
==========

[emircan, 2017-02-10 17:11:23]

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

[commit-bot: I haz the power, 2017-02-10 17:11:57]

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

[emircan, 2017-02-10 17:14:55]

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

https://codereview.chromium.org/2435693004/diff/100001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:492: uint32_t rtp_timestamp = 0;
On 2017/02/10 00:05:08, pbos wrote:
> Can you make this optional?

Done.

https://codereview.chromium.org/2435693004/diff/100001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:498: while
(!pending_timestamps_.empty()) {
On 2017/02/10 00:05:08, pbos wrote:
> This should never be empty, right? If so there's a bug in the VEA?

Yes. However I want to loop until all the dropped frames' timestamps are cleared
and I just use this as the condition to parse until everything is finished.
Suppose HW VEA is somewhat broken and returns a different timestamp then given,
then we would empty all timestamps and not let this queue grow.

https://codereview.chromium.org/2435693004/diff/100001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:507: }
On 2017/02/10 00:05:07, pbos wrote:
> Can you DCHECK after this brace that rtp_timestamp actually got set?

Again, suppose HW VEA is somewhat broken and returns a different timestamp then
given,
then we would empty all timestamps and not let this queue grow. rtp_timestamp()
wouldn't be set then and we would fall to l.515.

https://codereview.chromium.org/2435693004/diff/100001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:515: if (rtp_timestamp == 0) {
On 2017/02/10 00:05:08, pbos wrote:
> 0 is a valid RTP timestamp. Can you gate this on whether rtp_timestamp is set
or
> not instead?

Done.

[pbos, 2017-02-10 17:24:44]

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

https://codereview.chromium.org/2435693004/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:507: }
I'm worried we'll ever go from RTPTimestamps to non-RTP timestamps (some match
and some don't).

If some VEAs are broken aren't we breaking them completely now? Or is
rtp_timestamp approximately the generated one?

[commit-bot: I haz the power, 2017-02-10 17:55:31]

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

[commit-bot: I haz the power, 2017-02-10 17:55:32]

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

[emircan, 2017-02-10 18:33:08]

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

[emircan, 2017-02-10 22:46:34]

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

[commit-bot: I haz the power, 2017-02-10 22:46:58]

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

[emircan, 2017-02-10 22:48:37]

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

https://codereview.chromium.org/2435693004/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:507: }
On 2017/02/10 17:24:43, pbos wrote:
> I'm worried we'll ever go from RTPTimestamps to non-RTP timestamps (some match
> and some don't).
> 
> If some VEAs are broken aren't we breaking them completely now? Or is
> rtp_timestamp approximately the generated one?

I am considering the cases where VEA is consistently giving the wrong
timestamps. Look at AndroidVEA for instance[0]. It returns the time when encode
is completed. We can't match that with anything. So, we should just empty the
queue and set rtp timestamp using l.515. This is same as what was happening
before.

Also, I added unittest to verify the match. 

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

https://codereview.chromium.org/2435693004/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:606: :
base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms());
Do you think it is still right to use ntp_time_ms here?

[commit-bot: I haz the power, 2017-02-10 23:48:25]

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

[commit-bot: I haz the power, 2017-02-10 23:48:25]

Dry run: This issue passed the CQ dry run.

[pbos, 2017-02-13 03:35:51]

Nisse can you look at the timestamp question. :)

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

https://codereview.chromium.org/2435693004/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:507: }
On 2017/02/10 22:48:37, emircan wrote:
> On 2017/02/10 17:24:43, pbos wrote:
> > I'm worried we'll ever go from RTPTimestamps to non-RTP timestamps (some
match
> > and some don't).
> > 
> > If some VEAs are broken aren't we breaking them completely now? Or is
> > rtp_timestamp approximately the generated one?
> 
> I am considering the cases where VEA is consistently giving the wrong
> timestamps. Look at AndroidVEA for instance[0]. It returns the time when
encode
> is completed. We can't match that with anything. So, we should just empty the
> queue and set rtp timestamp using l.515. This is same as what was happening
> before.
> 
> Also, I added unittest to verify the match. 
> 
> [0]
>
https://cs.chromium.org/chromium/src/media/gpu/android_video_encode_accelerat...

I would like to never ever use pending_timestamps_ if they ever fail so that we
don't end up in a case where we flipflop back and forth adding jitter. What do
you think?

If so, can we add logging for how often that actually fails? E.g. something
HISTOGRAMS?

[nisse-chromium (ooo August 14), 2017-02-13 07:40:32]

I'm afraid I'm not familiar at all with this code.

Some comments below, but it's hard for me to say what's right.

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

https://codereview.chromium.org/2435693004/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:518: capture_time_us * 90 /
base::Time::kMicrosecondsPerMillisecond);
Note that webrtc sets the rtp time differently, based on ntp time. See 

https://cs.chromium.org/chromium/src/third_party/webrtc/video/vie_encoder.cc?...

Not sure if that's a problem (not changed in this cl).

https://codereview.chromium.org/2435693004/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:606: :
base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms());
On 2017/02/10 22:48:37, emircan wrote:
> Do you think it is still right to use ntp_time_ms here?

I see no docs on what epoch media::VideoFrame::timestamp_ is 
supposed to use, so it's not clear to me what's right.

But since nothing else in this file appears to use ntp time, it looks odd.

[emircan, 2017-02-13 20:22:44]

Description was changed from

==========
Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST=Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
==========

to

==========
Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST=Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[emircan, 2017-02-13 20:22:48]

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

[emircan, 2017-02-13 20:23:20]

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

https://codereview.chromium.org/2435693004/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:507: }
On 2017/02/13 03:35:51, pbos wrote:
> On 2017/02/10 22:48:37, emircan wrote:
> > On 2017/02/10 17:24:43, pbos wrote:
> > > I'm worried we'll ever go from RTPTimestamps to non-RTP timestamps (some
> match
> > > and some don't).
> > > 
> > > If some VEAs are broken aren't we breaking them completely now? Or is
> > > rtp_timestamp approximately the generated one?
> > 
> > I am considering the cases where VEA is consistently giving the wrong
> > timestamps. Look at AndroidVEA for instance[0]. It returns the time when
> encode
> > is completed. We can't match that with anything. So, we should just empty
the
> > queue and set rtp timestamp using l.515. This is same as what was happening
> > before.
> > 
> > Also, I added unittest to verify the match. 
> > 
> > [0]
> >
>
https://cs.chromium.org/chromium/src/media/gpu/android_video_encode_accelerat...
> 
> I would like to never ever use pending_timestamps_ if they ever fail so that
we
> don't end up in a case where we flipflop back and forth adding jitter. What do
> you think?
> 
> If so, can we add logging for how often that actually fails? E.g. something
> HISTOGRAMS?

I see, you are considering the case where we can have one odd match using the
wrong value.I can add a boolean to track that. 

I actually looked closer to AndroidVEA. It looks like none of the clients make
use that given current time. We can just pass an empty timestamp there instead
and fall to the else case here. Then, I think I can add
DCHECK(rtp_timestamp.has_value()) as you first suggested.

I dont think histograms are necessary. There are VEA unittests to make sure that
it is contained in Mac/Win/CrOS. And Android always fails.

https://codereview.chromium.org/2435693004/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:518: capture_time_us * 90 /
base::Time::kMicrosecondsPerMillisecond);
On 2017/02/13 07:40:32, nisse-chromium wrote:
> Note that webrtc sets the rtp time differently, based on ntp time. See 
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/video/vie_encoder.cc?...
> 
> Not sure if that's a problem (not changed in this cl).

This is the case where HW encoder actually drops timestamp information and we do
not have any indication, i.e. Android. We do not have ntp_time either. We just
make up an rtp_timestamp and capture_time_ms from current time. I don't know how
else we can do it.

https://codereview.chromium.org/2435693004/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:606: :
base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms());
On 2017/02/13 07:40:32, nisse-chromium wrote:
> On 2017/02/10 22:48:37, emircan wrote:
> > Do you think it is still right to use ntp_time_ms here?
> 
> I see no docs on what epoch media::VideoFrame::timestamp_ is 
> supposed to use, so it's not clear to me what's right.
> 
> But since nothing else in this file appears to use ntp time, it looks odd.

This here represents the case where WebRTC dropped the reference to the Chrome's
original media::VideoFrame. We need a strictly increasing presentation timestamp
to pass to the HW encoder and we make one from ntp_time_ms(). It also looked odd
to me, but I guess we should keep it as is in case of a regression.

Also, can you let me know what to do on l.527? Should |capture_time_ms_| carry
the value from |render_time_ms|?

[commit-bot: I haz the power, 2017-02-13 20:23:55]

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, 2017-02-13 23:45:49]

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

[commit-bot: I haz the power, 2017-02-13 23:45:50]

Dry run: This issue passed the CQ dry run.

[nisse-chromium (ooo August 14), 2017-02-14 07:45:04]

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

https://codereview.chromium.org/2435693004/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:527: image.capture_time_ms_ =
capture_time_ms;
I think we must use a time consistent with rtc::TimeMicros(), since, iiuc, this
time is going to be compared to clock_->TimeInMicroseconds() down in the
rtp/rtcp code. Raw camera time (+ offset) won't work.

So either always use current time, or if you can get render_time_ms() (aka
timestamp_us() / 1000) from the corresponding uncompressed webrtc::VideoFrame.
Or -1, and then the rtp code should substitute current time later.

[emircan, 2017-02-14 20:07:33]

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

https://codereview.chromium.org/2435693004/diff/120001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:527: image.capture_time_ms_ =
capture_time_ms;
On 2017/02/14 07:45:04, nisse-chromium wrote:
> I think we must use a time consistent with rtc::TimeMicros(), since, iiuc,
this
> time is going to be compared to clock_->TimeInMicroseconds() down in the
> rtp/rtcp code. Raw camera time (+ offset) won't work.
> 
> So either always use current time, or if you can get render_time_ms() (aka
> timestamp_us() / 1000) from the corresponding uncompressed webrtc::VideoFrame.
> Or -1, and then the rtp code should substitute current time later. 
We can't guarantee returning render_time_ms() as some HW encoders actually don't
preserve timestamp info, i.e. Android.

I will make that change in a separate CL, please review 
https://codereview.chromium.org/2692853009/. This CL is addressing rtp_timestamp
which is related to stats, see
https://bugs.chromium.org/p/chromium/issues/detail?id=597087. We have concerns
that it might trigger some unwanted side effects as you can see in the bug, and
may be reverted like the attempts before.

[nisse-chromium (ooo August 14), 2017-02-15 08:03:29]

On 2017/02/14 20:07:33, emircan wrote:

> We can't guarantee returning render_time_ms() as some HW encoders actually
don't
> preserve timestamp info, i.e. Android.

That ought to be fixed. I think webrtc's android encoder preserves timestamp by
using a separate
queue with frame metadata. See
https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a...

I'm not so familiar with the code, it looks a bit similar to the RTCTimestamps
list in this cl,
but the androidmediaencoder_jni code seems to simply assumes fifo order, which
differs 
from the way RTCTimestamps are searched.

lgtm.

[emircan, 2017-02-15 18:44:01]

On 2017/02/15 08:03:29, nisse-chromium wrote:
> On 2017/02/14 20:07:33, emircan wrote:
> 
> > We can't guarantee returning render_time_ms() as some HW encoders actually
> don't
> > preserve timestamp info, i.e. Android.
> 
> That ought to be fixed. I think webrtc's android encoder preserves timestamp
by
> using a separate
> queue with frame metadata. See
>
https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/a...
> 
> I'm not so familiar with the code, it looks a bit similar to the RTCTimestamps
> list in this cl,
> but the androidmediaencoder_jni code seems to simply assumes fifo order, which
> differs 
> from the way RTCTimestamps are searched.
> 
> lgtm.

Thanks. I will make a bug referring to that.

pbos@ PTAL.

[pbos-webrtc, 2017-02-15 20:44:49]

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

[pbos-webrtc, 2017-02-15 20:44:50]

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

https://codereview.chromium.org/2435693004/diff/160001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:507: }
Can we say that if pending_timestamps_ ever does not find a match we don't try
again in a release build? E.g. bool failed_timestamp_search_ = true, and don't
use the pending_timestamps_ loop again?

[emircan, 2017-02-15 21:03:11]

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

[emircan, 2017-02-15 21:03:16]

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

https://codereview.chromium.org/2435693004/diff/160001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:507: }
On 2017/02/15 20:44:50, pbos-webrtc wrote:
> Can we say that if pending_timestamps_ ever does not find a match we don't try
> again in a release build? E.g. bool failed_timestamp_search_ = true, and don't
> use the pending_timestamps_ loop again?

Done.

[commit-bot: I haz the power, 2017-02-15 21:04:34]

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, 2017-02-15 23:47:23]

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

[commit-bot: I haz the power, 2017-02-15 23:47:24]

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

[nisse-chromium (ooo August 14), 2017-02-16 07:49:50]

(Still lgtm, if you agree stats are important, that can be a separate cl).

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

https://codereview.chromium.org/2435693004/diff/160001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:507: }
On 2017/02/15 20:44:50, pbos-webrtc wrote:
> Can we say that if pending_timestamps_ ever does not find a match we don't try
> again in a release build? E.g. bool failed_timestamp_search_ = true, and don't
> use the pending_timestamps_ loop again?

That sounds like a failure event we might want some stats for.

[pbos, 2017-02-16 19:07:18]

lgtm, I think logging an impression for failures would be a good idea though

[emircan, 2017-02-16 19:52:14]

Description was changed from

==========
Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST=Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST=Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[emircan, 2017-02-16 19:52:14]

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

[Stefan, 2017-02-22 08:31:15]

This solution looks a bit fragile to me. Isn't it a better choice to actually
pass the timestamp to the encoder if possible and get it back when the frame is
done? I know this is at least possible on iOS as we do it on mobile:

https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/objc/Framework/Cl...

There we actually pass a FrameEncodeParams object to
VTCompressionSessionEncodeFrame() and retrieve it in the
VTCompressionOutputCallback(). Probably a larger change, but maybe it makes
sense to do?

[emircan, 2017-02-22 18:09:29]

On 2017/02/22 08:31:15, Stefan wrote:
> This solution looks a bit fragile to me. Isn't it a better choice to actually
> pass the timestamp to the encoder if possible and get it back when the frame
is
> done? I know this is at least possible on iOS as we do it on mobile:
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/objc/Framework/Cl...
> 
> There we actually pass a FrameEncodeParams object to
> VTCompressionSessionEncodeFrame() and retrieve it in the
> VTCompressionOutputCallback(). Probably a larger change, but maybe it makes
> sense to do?

We do pass the timestamp to the encoder and get it back. We do the exact same
thing for Mac as the link you gave [0]. Timestamps is also preserved in
CrOS[1][2]. It is only not preserved in Android. However, what we are passing
here is media::VideoFrame timestamp which is the timestamp we get from camera in
capture time. Then, we are using that to make the match in rtc_video_encoder and
get the corresponding rtp timestamp. The reasons for doing this:
- scoped_refptr<VideoFrame> can be used by somebody else in the mean time and
modifying timestamp can cause issues.
- rtp timestamp (90 kHz) cannot be converted back and forth to a system clock
time without losing precision, see CL description. We need it in
microseconds[1].

[0]
https://cs.chromium.org/chromium/src/media/gpu/vt_video_encode_accelerator_ma...
[1]
https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_encode_accelerator....
[2]
https://cs.chromium.org/chromium/src/media/gpu/vaapi_video_encode_accelerator...

[pbos, 2017-02-22 18:14:24]

On 2017/02/22 18:09:29, emircan wrote:
> On 2017/02/22 08:31:15, Stefan wrote:
> > This solution looks a bit fragile to me. Isn't it a better choice to
actually
> > pass the timestamp to the encoder if possible and get it back when the frame
> is
> > done? I know this is at least possible on iOS as we do it on mobile:
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/objc/Framework/Cl...
> > 
> > There we actually pass a FrameEncodeParams object to
> > VTCompressionSessionEncodeFrame() and retrieve it in the
> > VTCompressionOutputCallback(). Probably a larger change, but maybe it makes
> > sense to do?
> 
> We do pass the timestamp to the encoder and get it back. We do the exact same
> thing for Mac as the link you gave [0]. Timestamps is also preserved in
> CrOS[1][2]. It is only not preserved in Android. However, what we are passing
> here is media::VideoFrame timestamp which is the timestamp we get from camera
in
> capture time. Then, we are using that to make the match in rtc_video_encoder
and
> get the corresponding rtp timestamp. The reasons for doing this:
> - scoped_refptr<VideoFrame> can be used by somebody else in the mean time and
> modifying timestamp can cause issues.
> - rtp timestamp (90 kHz) cannot be converted back and forth to a system clock
> time without losing precision, see CL description. We need it in
> microseconds[1].
> 
> [0]
>
https://cs.chromium.org/chromium/src/media/gpu/vt_video_encode_accelerator_ma...
> [1]
>
https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_encode_accelerator....
> [2]
>
https://cs.chromium.org/chromium/src/media/gpu/vaapi_video_encode_accelerator...

A higher-level answer is that VEA doesn't allow passing arbitrary data but only
a base::TimeStamp, right?

[pbos-webrtc, 2017-03-16 18:20:45]

pbos@webrtc.org changed reviewers:
- pbos@webrtc.org

[emircan, 2017-03-24 21:20:24]

Description was changed from

==========
Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST=Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST=Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[emircan, 2017-03-24 21:20:26]

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

[commit-bot: I haz the power, 2017-03-24 21:21:00]

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

[emircan, 2017-03-24 21:38:32]

emircan@chromium.org changed reviewers:
+ liberato@chromium.org
- mcasas@chromium.org

[emircan, 2017-03-24 21:38:33]

Rebased it. After offline discussion, we decided to go ahead with this fix while
keeping an eye on cpu adaptation, see
https://bugs.chromium.org/p/webrtc/issues/detail?id=7304 for cpu adaptation
discussion.

Added liberato@ for media/gpu/android_video_encode_accelerator.cc. PTAL.

[commit-bot: I haz the power, 2017-03-24 23:27:10]

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

[commit-bot: I haz the power, 2017-03-24 23:27:11]

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

[liberato (no reviews please), 2017-03-27 19:31:49]

lgtm % nit.

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

https://codereview.chromium.org/2435693004/diff/200001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:43: const int64_t
media_timestamp_in_microseconds;
why not use base::TimeDelta for this?  seems like almost every reference to it
includes InMicroseconds() on some other TimeDelta.

[emircan, 2017-03-27 20:19:26]

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

[commit-bot: I haz the power, 2017-03-27 20:20:01]

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

[emircan, 2017-03-27 20:20:32]

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

https://codereview.chromium.org/2435693004/diff/200001/content/renderer/media...
content/renderer/media/gpu/rtc_video_encoder.cc:43: const int64_t
media_timestamp_in_microseconds;
On 2017/03/27 19:31:49, liberato wrote:
> why not use base::TimeDelta for this?  seems like almost every reference to it
> includes InMicroseconds() on some other TimeDelta.

Done.

[emircan, 2017-03-27 20:58:41]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2017-03-27 20:58:42]

The CQ bit was checked by emircan@chromium.org

[emircan, 2017-03-27 20:58:42]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@chromium.org,
pbos@chromium.org, liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2435693004/#ps220001
(title: " ")

[commit-bot: I haz the power, 2017-03-27 20:59:38]

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, 2017-03-27 22:30:14]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490648322019340,
"parent_rev": "5f9c4837f93b8c54dcc4c9f68d1a93e09a6e1e96", "commit_rev":
"2372de8b97b72d62e9ece294494a351ff633a2d8"}

[commit-bot: I haz the power, 2017-03-27 22:31:05]

Description was changed from

==========
Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST=Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Reland: 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 these stats.

Note that we can't directly use RTP timestamp via base::TimeDelta.
Suppose WebRTC's timestamp() is 30000 (in 90 kHZ). Then, we would set Chrome's
base::TimeDelta 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 return the same timestamp after converting
back. Therefore, we keep the value as it is. Moreover, using RTP timestamp's 
converted value or 90 kHz value in HW encoder causes problems as HW encoder 
expects presentation timestamp. Bitrate gets visibly worse on Win&Mac.

Therefore, I decided to hold onto a queue of presentation timestamp and RTP 
timestamp in RTCVideoEncoder and match values at the end.

BUG=597087
TEST=Tested AppRTC H264 loopback on Mac. googAvgEncodeMs and 
googEncodeUsagePercent stats are non-zero.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2435693004
Cr-Commit-Position: refs/heads/master@{#459908}
Committed:
https://chromium.googlesource.com/chromium/src/+/2372de8b97b72d62e9ece294494a...
==========

[commit-bot: I haz the power, 2017-03-27 22:31:07]

Committed patchset #8 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/2372de8b97b72d62e9ece294494a...