ccameron@, would you take a look at this issue (it's mostly Greek to me)?

I filed this bug from a discussion with "emir...@chromium.org" (I can't see his entire email) over in this bug https://bugs.chromium.org/p/chromium/issues/detail?id=500605&can=2&start=0&num=100&q=&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified&groupby=&sort= so I'd cc him.

I don't think this is Mac-specific.

@emir we've seen hardware encode issues on windows as well, resulting in the VDA error filed here: https://bugs.chromium.org/p/chromium/issues/detail?id=583704, but I don't remember seeing the stat problems (I could be wrong, it was a while ago), so assumed this was related to the videotoolbox changes you pushed.  Should I re-try allowing windows to use hw encode on windows 10?

Thanks for the report. We checked the issue on ChromeOS with VP8 HW encode as well, and these stats don't work there either. I can conclude that there isn't support for these stats when HW encoder is used. We can work on a generic solution for all HW encoder support, and I will change the title to reflect that.

Regarding #5, I am not sure what do you mean by HW encode in Windows. AFAIK, there isn't any HW encode support yet in Chrome. 

Ok, good to know.  As for windows, looks like I misremembered.  We worked around the problem by forcing software on the decode side, not encode.

Has there been any progress on the hw acceleration for 264 on mac (or windows?)

We measure these encode times based on incoming and outgoing rtp timestamps, RTCVideoEncoder doesn't preserve the input timestamp so we can't measure there.

What I have fund out so far is below. pbos@ can you PTAL and give input & cc who might be able to help.

On capture side:
- WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame sets render_time_ms from the incoming frame. [0]
- VideoCaptureInput::IncomingCapturedFrame adds delta_ntp_internal_ms_ to it, and sets timestamps based on that. [1]
- OveruseFrameDetector::FrameCaptured adds the incoming timestamp in a std::list. [2]
On encode side:
- RTCVideoEncoder::Impl::BitstreamBufferReady calculates rtp_timestamp from the timestamp coming from the encoder. [3]
- ViEEncoder::Encoded calls OveruseFrameDetector::FrameSent which receives rtp_timestamp and tries to match with the ones it received before. [4]
It cannot find a match since the delta_ntp_internal_ms_ is not included in the timestamp coming from capture side. I am not sure if we need to make changes on Chrome side or WebRTC side for this.

[0] https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvideoengine2.cc?rcl=0&l=1642
[1] https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_capture_input.cc?rcl=1469733765&l=65
[2] https://cs.chromium.org/chromium/src/third_party/webrtc/video/overuse_frame_detector.cc?rcl=0&l=253
[3] https://cs.chromium.org/chromium/src/content/renderer/media/gpu/rtc_video_encoder.cc?rcl=0&l=481
[4] https://cs.chromium.org/chromium/src/third_party/webrtc/video/overuse_frame_detector.cc?rcl=0&l=257

The encoder needs to propagate the timestamp. For RtcVideoEncoder I believe this was done by propagating ntp_time_ms as timestamp and then calculating the RTP timestamp based on this the same way VideoCaptureInput does. I'm not sure if all VEA implementations propagate the timestamp properly.

FWIW I think we should move away from RTP timestamps here and only forward ntp_time_ms, which possibly should be ntp_time_us, but that's a larger change and the VEA still needs to forward timestamp.

wuchengli@: Do you know if timestamp forwarding was ever implemented for Mac? Can you assist emircan@ here?

pbos@, VEA implementations and RTCVideoEncoder forward the timestamps now. Mac[0] has it and RTCVideoEncoder sends the timestamp if available[1]. However, we still do not get these stats.

As far as I understand, these stats are calculated within OveruseFrameDetector, by making a match between timestamps received in OveruseFrameDetector::FrameCaptured and OveruseFrameDetector::FrameSent. However, they wouldn't match as I explained above. On capture side VideoCaptureInput::IncomingCapturedFrame adds delta, whereas encode side just sends it as is. Is there any change we need to do on RTCVideoEncoder to accomodate the drift?

[0] https://codereview.chromium.org/2039423002
[1] https://codereview.chromium.org/2105683002

Shouldn't:

    const base::TimeDelta timestamp =
        frame ? frame->timestamp()
              : base::TimeDelta::FromMilliseconds(next_frame->ntp_time_ms());

Be frame->ntp_time_ms() instead of frame->timestamp()?

Does that fix the issue? I'm not sure I fully wrap my head around it. If we copy ntp_time_ms() the conversion back to RTP timestamps should be the same as the one done by VideoCaptureInput, right?

Nvm, I misunderstood the frame type. Can we sync offline? I'm not wrapping my head around this at all.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e3195490a63d9545fb1bfe560aa21680ba0b5843

commit e3195490a63d9545fb1bfe560aa21680ba0b5843
Author: emircan <emircan@chromium.org>
Date: Fri Aug 26 00:10:48 2016

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).

Review-Url: https://codereview.chromium.org/2205623002
Cr-Commit-Position: refs/heads/master@{#414589}

[modify] https://crrev.com/e3195490a63d9545fb1bfe560aa21680ba0b5843/content/renderer/media/gpu/rtc_video_encoder.cc
[modify] https://crrev.com/e3195490a63d9545fb1bfe560aa21680ba0b5843/media/gpu/video_encode_accelerator_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb436ddb8b313a81e758b5c5f97ca4349b5203a0

commit eb436ddb8b313a81e758b5c5f97ca4349b5203a0
Author: emircan <emircan@chromium.org>
Date: Wed Aug 31 21:14:18 2016

Revert of Use webrtc::VideoFrame timestamp in RTCVideoEncoder (patchset #7 id:260001 of https://codereview.chromium.org/2205623002/ )

Reason for revert:
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 

Original issue's description:
> 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}

TBR=wuchengli@chromium.org,pbos@chromium.org,posciak@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 597087 

Review-Url: https://codereview.chromium.org/2296273002
Cr-Commit-Position: refs/heads/master@{#415752}

[modify] https://crrev.com/eb436ddb8b313a81e758b5c5f97ca4349b5203a0/content/renderer/media/gpu/rtc_video_encoder.cc
[modify] https://crrev.com/eb436ddb8b313a81e758b5c5f97ca4349b5203a0/media/gpu/video_encode_accelerator_unittest.cc

Updating status after revert.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/21605fd6c714fb6b6b48cd64bd5b96b7e95f8e66

commit 21605fd6c714fb6b6b48cd64bd5b96b7e95f8e66
Author: emircan <emircan@chromium.org>
Date: Thu Sep 01 21:33:08 2016

Revert of Use webrtc::VideoFrame timestamp in RTCVideoEncoder (patchset #7 id:260001 of https://codereview.chromium.org/2205623002/ )

Reason for revert:
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 

Original issue's description:
> 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}

TBR=wuchengli@chromium.org,pbos@chromium.org,posciak@chromium.org
BUG= 597087 

Review-Url: https://codereview.chromium.org/2296273002
Cr-Commit-Position: refs/heads/master@{#415752}
(cherry picked from commit eb436ddb8b313a81e758b5c5f97ca4349b5203a0)

NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2303053002
Cr-Commit-Position: refs/branch-heads/2840@{#110}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/21605fd6c714fb6b6b48cd64bd5b96b7e95f8e66/content/renderer/media/gpu/rtc_video_encoder.cc
[modify] https://crrev.com/21605fd6c714fb6b6b48cd64bd5b96b7e95f8e66/media/gpu/video_encode_accelerator_unittest.cc

holmer@, as we discussed offline earlier, I want to make sure that we are aware of the side effects of this change before moving on. Can we make sure that enabling these stats for HW encode do not cause CPU adaptation or dropping frames? Here is the new CL for your reference:
https://codereview.chromium.org/2435693004

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/21605fd6c714fb6b6b48cd64bd5b96b7e95f8e66

commit 21605fd6c714fb6b6b48cd64bd5b96b7e95f8e66
Author: emircan <emircan@chromium.org>
Date: Thu Sep 01 21:33:08 2016

Revert of Use webrtc::VideoFrame timestamp in RTCVideoEncoder (patchset #7 id:260001 of https://codereview.chromium.org/2205623002/ )

Reason for revert:
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 

Original issue's description:
> 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}

TBR=wuchengli@chromium.org,pbos@chromium.org,posciak@chromium.org
BUG= 597087 

Review-Url: https://codereview.chromium.org/2296273002
Cr-Commit-Position: refs/heads/master@{#415752}
(cherry picked from commit eb436ddb8b313a81e758b5c5f97ca4349b5203a0)

NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2303053002
Cr-Commit-Position: refs/branch-heads/2840@{#110}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/21605fd6c714fb6b6b48cd64bd5b96b7e95f8e66/content/renderer/media/gpu/rtc_video_encoder.cc
[modify] https://crrev.com/21605fd6c714fb6b6b48cd64bd5b96b7e95f8e66/media/gpu/video_encode_accelerator_unittest.cc

Marking it as available as I do not have any other solution than my earlier attempts. As I explained #23, we need to understand the side effects of this change on WebRTC side regarding CPU adaptation or dropping frames moving on.

Re #25: What side effects do you need analyzed? This is in use for Android MediaCodec and software libvpx. Did you observe anything breaking?

pbos@, you can look at the reasons for my reverts to see the effects, see  issue 641230 . 
- The fix on  issue 350106  does not help anything as WebRTC expects RTP timestamp back from RTCVideoEncoder to match.
- HW encoders expect proper presentation timestamps and return that back with the encoded data. Passing RTP timestamp(90 kHz) here causes problem, converting back and forth loses accuracy. Hence I prepared CL https://codereview.chromium.org/2435693004 that keeps keeps a map.
- When we manage to fix these stats(googEncodeMs and googEncodeUsagePercent), we should make sure that this doesn't cause cpu adaptation or dropping frames. I was awaiting feedback on that before moving on.

+mflodman@ to help retriage/make the call here.

We use this for software encoding and Android hardware encoding. Last I checked (when implementing) we use a longer threshold for hardware encoders, because some pipeline encodings meaning that frames stay in the pipe for longer than 1/fps even though it can keep up 30fps.

I think you should use a deque with a pair (int64_t, int32_t) instead of an unordered map and just throw out old entries that the encoder dropped internally. That way the map doesn't indefinitely.

I think it would be good to resurrect that last CL. mflodman@ can hopefully make the call on what evaluation/QA would be needed to comfortably release this, but it's in use on Android and for software encoding. Perhaps we could do one platform at a time, I don't know.

Pinging mflodman@ for input as we discussed above.

pbos@ I updated the CL as you described: https://codereview.chromium.org/2435693004. 

It turns out that this is also related to a/v sync issues, at least on Mac, see: https://bugs.chromium.org/p/webrtc/issues/detail?id=7045.

The workaround in that issue causes increased delays and less accurate rendering on all platforms using HW encoders:
https://bugs.chromium.org/p/chromium/issues/detail?id=693509

so I think it's reasonable to increase the priority of fixing this.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2372de8b97b72d62e9ece294494a351ff633a2d8

commit 2372de8b97b72d62e9ece294494a351ff633a2d8
Author: emircan <emircan@chromium.org>
Date: Mon Mar 27 22:30:09 2017

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}

[modify] https://crrev.com/2372de8b97b72d62e9ece294494a351ff633a2d8/content/renderer/media/gpu/rtc_video_encoder.cc
[modify] https://crrev.com/2372de8b97b72d62e9ece294494a351ff633a2d8/content/renderer/media/gpu/rtc_video_encoder_unittest.cc
[modify] https://crrev.com/2372de8b97b72d62e9ece294494a351ff633a2d8/media/gpu/android_video_encode_accelerator.cc