Measure encoding time on encode callbacks.

Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating overload.

Merges EncodingTimeObserver into EncodedFrameObserver to have one post-encode
callback.

BUG=webrtc:5042, webrtc:5132
R=asapersson@webrtc.org, mflodman@webrtc.org

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

[pbos-webrtc, 2016-01-07 15:45:43]

PTAL!

[pbos-webrtc, 2016-01-13 13:54:21]

add extended overuse time

[pbos-webrtc, 2016-01-13 13:56:55]

Description was changed from

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

BUG=webrtc:5132
R=asapersson@webrtc.org
==========

to

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating CPU
overuse.

BUG=webrtc:5132
R=asapersson@webrtc.org
==========

[pbos-webrtc, 2016-01-13 13:57:43]

Added extended time which permits running 30fps on Nexus 6, while still adapting
down for Moto G, which is running on its knees.

[pbos-webrtc, 2016-01-13 13:58:07]

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

[pbos-webrtc, 2016-01-13 13:58:08]

+R mflodman for root header (and anything you wanna take a look at)

[pbos-webrtc, 2016-01-13 14:00:27]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-13 14:00:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1569853002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1569853002/20001

[pbos-webrtc, 2016-01-13 14:01:11]

Description was changed from

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating CPU
overuse.

BUG=webrtc:5132
R=asapersson@webrtc.org
==========

to

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating CPU/load
overuse.

BUG=webrtc:5132
R=asapersson@webrtc.org
==========

[pbos-webrtc, 2016-01-13 14:01:30]

Description was changed from

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating CPU/load
overuse.

BUG=webrtc:5132
R=asapersson@webrtc.org
==========

to

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating overload.

BUG=webrtc:5132
R=asapersson@webrtc.org
==========

[commit-bot: I haz the power, 2016-01-13 16:00:51]

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

[commit-bot: I haz the power, 2016-01-13 16:00:52]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[mflodman, 2016-01-21 08:00:05]

I didn't spend a lot of time on the tests this round, I'll get back to that.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/encoder_stat...
File webrtc/video/encoder_state_feedback_unittest.cc (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/encoder_stat...
webrtc/video/encoder_state_feedback_unittest.cc:25: #include
"webrtc/video/overuse_frame_detector.h"
Forward declaring OveruseFrameDetector should be enough.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:19: #include "webrtc/base/logging.h"
What is this needed for?

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:145: EncodedFrameTimeMeasured(0);
Is this really needed? For the callback or to actually set values?

1. If it is to set the values, it would be better to set them as default right
away instead.

2. If it is for the callback, we can just call the callback instead.

Or both 1 and 2.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:152: void
OveruseFrameDetector::EncodedFrameTimeMeasured(int encode_time_ms) {
I'd prefer encoded_time_ms or encoder_process_time or something telling this is
the time it took to encode a frame rather than a wall clock time.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:184:
EncodedFrameTimeMeasured(last_encode_time_ms_);
Do we need this here? What is the benefit compared to just calling with 0 as
done initially? (i.e. is it worth having this member for this?)

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:201:
frame_timing_.push_back(FrameTiming(frame.timestamp(), now));
Is it better to use timestamp than capture_time_ms_?

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:211: static const int64_t
kEncodingTimeDelayMs = 1000;
I'd prefer to call this something indicating this is the max delay used to
measure, now this can be confused with the actual encoding time. I'll give this
some more thoughts.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
File webrtc/video/overuse_frame_detector.h (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.h:94: void FrameSent(const EncodedImage&
encoded_image);
I'd prefer to only have the timing value as an argument, we don't use anything
else from the frame.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.h:139: int last_encode_time_ms_
GUARDED_BY(crit_);
I prefer int64_t, following the rest of this class.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/video_captur...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:137: // TODO(pbos): Move
encoding_time_observer into OveruseFrameDetector.
I think this CL is touching a lot of files already, shouldn't this be done right
away instead to get the same encoding times in this callback as used for overuse
detection?

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:27: #include
"webrtc/modules/video_coding/encoded_frame.h"
Shouldn't this be webrtc/video_frame.h?

[pbos-webrtc, 2016-01-21 09:46:31]

rebase

[mflodman, 2016-01-21 11:20:32]

On 2016/01/21 09:46:31, pbos-webrtc wrote:
> rebase

I'ts normally easier to address comments in a first ps and then rebase, makes it
easier to compare diffs for just the comments and nothing else.

[pbos-webrtc, 2016-01-21 11:32:30]

On 2016/01/21 11:20:32, mflodman wrote:
> On 2016/01/21 09:46:31, pbos-webrtc wrote:
> > rebase
> 
> I'ts normally easier to address comments in a first ps and then rebase, makes
it
> easier to compare diffs for just the comments and nothing else.

Ah, I thought just having them in their own patchset would be enough?

[pbos-webrtc, 2016-01-21 14:18:33]

PTAL

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/encoder_stat...
File webrtc/video/encoder_state_feedback_unittest.cc (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/encoder_stat...
webrtc/video/encoder_state_feedback_unittest.cc:25: #include
"webrtc/video/overuse_frame_detector.h"
On 2016/01/21 08:00:04, mflodman wrote:
> Forward declaring OveruseFrameDetector should be enough.

Done.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:19: #include "webrtc/base/logging.h"
On 2016/01/21 08:00:04, mflodman wrote:
> What is this needed for?

Local printf debugging. Gone.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:145: EncodedFrameTimeMeasured(0);
On 2016/01/21 08:00:04, mflodman wrote:
> Is this really needed? For the callback or to actually set values?
> 
> 1. If it is to set the values, it would be better to set them as default right
> away instead.
> 
> 2. If it is for the callback, we can just call the callback instead.
> 
> Or both 1 and 2.

Removed, I don't think we should have to report the current values before we've
measured them.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:152: void
OveruseFrameDetector::EncodedFrameTimeMeasured(int encode_time_ms) {
On 2016/01/21 08:00:04, mflodman wrote:
> I'd prefer encoded_time_ms or encoder_process_time or something telling this
is
> the time it took to encode a frame rather than a wall clock time.

encode_duration_ms done

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:184:
EncodedFrameTimeMeasured(last_encode_time_ms_);
On 2016/01/21 08:00:05, mflodman wrote:
> Do we need this here? What is the benefit compared to just calling with 0 as
> done initially? (i.e. is it worth having this member for this?)

No don't think we need to re-report here even.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:201:
frame_timing_.push_back(FrameTiming(frame.timestamp(), now));
On 2016/01/21 08:00:04, mflodman wrote:
> Is it better to use timestamp than capture_time_ms_?

We don't get capture_time_ms out of the encoder after encoding. If we're lucky
we get correct RTP timestamps (true except for ChromeOS).

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:211: static const int64_t
kEncodingTimeDelayMs = 1000;
On 2016/01/21 08:00:04, mflodman wrote:
> I'd prefer to call this something indicating this is the max delay used to
> measure, now this can be confused with the actual encoding time. I'll give
this
> some more thoughts.

kEncodingTimeMeasureWindowsMs, let me know if you have something
shorter/snappier.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
File webrtc/video/overuse_frame_detector.h (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.h:94: void FrameSent(const EncodedImage&
encoded_image);
On 2016/01/21 08:00:05, mflodman wrote:
> I'd prefer to only have the timing value as an argument, we don't use anything
> else from the frame.

Done.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.h:139: int last_encode_time_ms_
GUARDED_BY(crit_);
On 2016/01/21 08:00:05, mflodman wrote:
> I prefer int64_t, following the rest of this class.

Removed per other feedback.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/video_captur...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/video_captur...
webrtc/video/video_capture_input.cc:137: // TODO(pbos): Move
encoding_time_observer into OveruseFrameDetector.
On 2016/01/21 08:00:05, mflodman wrote:
> I think this CL is touching a lot of files already, shouldn't this be done
right
> away instead to get the same encoding times in this callback as used for
overuse
> detection?

Yep, done. This has the downside that times are not reported if encoding takes
>1 second. Removed EncodingTimeObserver in favor of adding OnEncodeTiming to
EncodedFrameCallback.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:27: #include
"webrtc/modules/video_coding/encoded_frame.h"
On 2016/01/21 08:00:05, mflodman wrote:
> Shouldn't this be webrtc/video_frame.h?

Done.

[pbos-webrtc, 2016-01-21 14:19:03]

Remove EncodingTimeObserver + other feedback

[pbos-webrtc, 2016-01-21 14:19:24]

Description was changed from

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating overload.

BUG=webrtc:5132
R=asapersson@webrtc.org
==========

to

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating overload.

BUG=webrtc:5042, webrtc:5132
R=asapersson@webrtc.org, mflodman@webrtc.org
==========

[pbos-webrtc, 2016-01-21 14:20:56]

Description was changed from

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating overload.

BUG=webrtc:5042, webrtc:5132
R=asapersson@webrtc.org, mflodman@webrtc.org
==========

to

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating overload.

Merges EncodingTimeObserver into EncodedFrameObserver to have one post-encode
callback.

BUG=webrtc:5042, webrtc:5132
R=asapersson@webrtc.org, mflodman@webrtc.org
==========

[pbos-webrtc, 2016-01-21 14:21:12]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 14:21:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1569853002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1569853002/60001

[commit-bot: I haz the power, 2016-01-21 14:26:40]

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

[commit-bot: I haz the power, 2016-01-21 14:26:41]

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

[pbos-webrtc, 2016-01-21 14:37:13]

remove unused variable

[åsapersson, 2016-01-25 15:50:47]

Some comments for overuse_frame_detector-files.

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:242: int64_t diff_ms = now -
last_sample_time_ms_;
Should last_send_time be used instead of last_sample_time_ms_. That would give
the current behavior right?

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:265: current_metrics = metrics_;
Could it be possible that metrics_ is not initialized here (e.g. now when not
updated in constructor)?

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
File webrtc/video/overuse_frame_detector_unittest.cc (right):

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector_unittest.cc:101: 1000);
Is kFrameInterval33ms and 1000 supposed to switch places?

Maybe add a constant for 1000.

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector_unittest.cc:215:
InsertAndSendFramesWithInterval(
Is this line needed?

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector_unittest.cc:283: // >80% encoding time
should trigger overuse.
>85%?

[pbos-webrtc, 2016-01-26 17:09:58]

rebase

[pbos-webrtc, 2016-01-26 17:22:22]

feedback

[pbos-webrtc, 2016-01-26 17:22:25]

PTAL

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:242: int64_t diff_ms = now -
last_sample_time_ms_;
On 2016/01/25 15:50:47, åsapersson wrote:
> Should last_send_time be used instead of last_sample_time_ms_. That would give
> the current behavior right? 

I think that would mean that we could call this in incorrect order, because this
gets called in order of input, but last_send_ms isn't necessarily in the same
order. Do you think there's an issue with this one (over time)?

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:265: current_metrics = metrics_;
On 2016/01/25 15:50:47, åsapersson wrote:
> Could it be possible that metrics_ is not initialized here (e.g. now when not
> updated in constructor)?

Yes, it'll be -1 until we have an estimate. I believe that's a good thing, what
do you think?

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
File webrtc/video/overuse_frame_detector_unittest.cc (right):

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector_unittest.cc:101: 1000);
On 2016/01/25 15:50:47, åsapersson wrote:
> Is kFrameInterval33ms and 1000 supposed to switch places?
> 
> Maybe add a constant for 1000.

Done.

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector_unittest.cc:215:
InsertAndSendFramesWithInterval(
On 2016/01/25 15:50:47, åsapersson wrote:
> Is this line needed?

Done.

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector_unittest.cc:283: // >80% encoding time
should trigger overuse.
On 2016/01/25 15:50:47, åsapersson wrote:
> >85%?

Done.

[pbos-webrtc, 2016-01-26 17:22:35]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-26 17:22:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1569853002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1569853002/120001

[commit-bot: I haz the power, 2016-01-26 17:57:10]

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

[commit-bot: I haz the power, 2016-01-26 17:57:11]

Dry run: Try jobs failed on following builders:
  linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[åsapersson, 2016-01-27 16:47:12]

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:242: int64_t diff_ms = now -
last_sample_time_ms_;
On 2016/01/26 17:22:25, pbos-webrtc wrote:
> On 2016/01/25 15:50:47, åsapersson wrote:
> > Should last_send_time be used instead of last_sample_time_ms_. That would
give
> > the current behavior right? 
> 
> I think that would mean that we could call this in incorrect order, because
this
> gets called in order of input, but last_send_ms isn't necessarily in the same
> order. Do you think there's an issue with this one (over time)?

Ok, maybe we should use capture_ms instead.

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:265: current_metrics = metrics_;
On 2016/01/26 17:22:25, pbos-webrtc wrote:
> On 2016/01/25 15:50:47, åsapersson wrote:
> > Could it be possible that metrics_ is not initialized here (e.g. now when
not
> > updated in constructor)?
> 
> Yes, it'll be -1 until we have an estimate. I believe that's a good thing,
what
> do you think?

Yes ok with -1 until we have an estimate. If -1 maybe we should return here
though and not use metrics for detecting overuse/underuse?

https://codereview.webrtc.org/1569853002/diff/120001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector_unittest.cc (right):

https://codereview.webrtc.org/1569853002/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector_unittest.cc:96: const int kOveruseDurationMs
= 1000;
could perhaps be confused with triggering overuse (maybe just leave out
constant).

[pbos-webrtc, 2016-02-01 16:09:05]

review feedback

[pbos-webrtc, 2016-02-01 16:09:12]

PTAL, I hope this is right.

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:242: int64_t diff_ms = now -
last_sample_time_ms_;
On 2016/01/27 16:47:12, åsapersson wrote:
> On 2016/01/26 17:22:25, pbos-webrtc wrote:
> > On 2016/01/25 15:50:47, åsapersson wrote:
> > > Should last_send_time be used instead of last_sample_time_ms_. That would
> give
> > > the current behavior right? 
> > 
> > I think that would mean that we could call this in incorrect order, because
> this
> > gets called in order of input, but last_send_ms isn't necessarily in the
same
> > order. Do you think there's an issue with this one (over time)?
> 
> Ok, maybe we should use capture_ms instead. 

Done.

https://codereview.webrtc.org/1569853002/diff/80001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:265: current_metrics = metrics_;
On 2016/01/27 16:47:12, åsapersson wrote:
> On 2016/01/26 17:22:25, pbos-webrtc wrote:
> > On 2016/01/25 15:50:47, åsapersson wrote:
> > > Could it be possible that metrics_ is not initialized here (e.g. now when
> not
> > > updated in constructor)?
> > 
> > Yes, it'll be -1 until we have an estimate. I believe that's a good thing,
> what
> > do you think?
> 
> Yes ok with -1 until we have an estimate. If -1 maybe we should return here
> though and not use metrics for detecting overuse/underuse?

Solved through rtc::Optional so we can distinguish between this being set or
not.

https://codereview.webrtc.org/1569853002/diff/120001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector_unittest.cc (right):

https://codereview.webrtc.org/1569853002/diff/120001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector_unittest.cc:96: const int kOveruseDurationMs
= 1000;
On 2016/01/27 16:47:12, åsapersson wrote:
> could perhaps be confused with triggering overuse (maybe just leave out
> constant).

Done.

[pbos-webrtc, 2016-02-01 16:14:11]

rebase

[åsapersson, 2016-02-02 15:19:10]

https://codereview.webrtc.org/1569853002/diff/160001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/1569853002/diff/160001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:195: if (last_capture_ms_ != 0)
!= -1

https://codereview.webrtc.org/1569853002/diff/160001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:266: return 0;
checked above

https://codereview.webrtc.org/1569853002/diff/160001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:275: bool check_for_backoff =
last_rampup_time_ > last_overuse_time_ms_;
also initialize last_rampup_time_ to -1 (and rename to last_rampup_time_ms_)

https://codereview.webrtc.org/1569853002/diff/160001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector.h (right):

https://codereview.webrtc.org/1569853002/diff/160001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.h:20: #include "webrtc/base/exp_filter.h"
nit: order

[pbos-webrtc, 2016-02-02 15:41:34]

feedback

[pbos-webrtc, 2016-02-02 15:41:45]

PTAL, also renamed capture_ms capture_time_ms

https://codereview.webrtc.org/1569853002/diff/160001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/1569853002/diff/160001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:195: if (last_capture_ms_ != 0)
On 2016/02/02 15:19:10, åsapersson wrote:
> != -1

Thanks, done!

https://codereview.webrtc.org/1569853002/diff/160001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:266: return 0;
On 2016/02/02 15:19:10, åsapersson wrote:
> checked above

Done.

https://codereview.webrtc.org/1569853002/diff/160001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:275: bool check_for_backoff =
last_rampup_time_ > last_overuse_time_ms_;
On 2016/02/02 15:19:10, åsapersson wrote:
> also initialize last_rampup_time_ to -1 (and rename to last_rampup_time_ms_) 

Done.

[pbos-webrtc, 2016-02-02 15:47:07]

rebase

[åsapersson, 2016-02-04 11:17:55]

https://codereview.webrtc.org/1569853002/diff/200001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1569853002/diff/200001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:437:
TEST_F(WebRtcVideoEngine2Test, DisablesFullEncoderTimeForExternalEncoders) {
disabled for non-external encoders?

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_captu...
File webrtc/video/video_capture_input.h (right):

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_captu...
webrtc/video/video_capture_input.h:19: #include "webrtc/base/scoped_ptr.h"
can this be removed?

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_captu...
webrtc/video/video_capture_input.h:34: class ProcessThread;
can this be removed?

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_captu...
File webrtc/video/video_capture_input_unittest.cc (right):

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_captu...
webrtc/video/video_capture_input_unittest.cc:18: #include
"webrtc/modules/utility/include/mock/mock_process_thread.h"
remove?

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (left):

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:980:
video_send_config_.encoding_time_observer = &analyzer;
set video_send_config_.post_encode_callback?

[pbos-webrtc, 2016-02-04 12:38:04]

Thanks, PTAL. :)

https://codereview.webrtc.org/1569853002/diff/200001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1569853002/diff/200001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:437:
TEST_F(WebRtcVideoEngine2Test, DisablesFullEncoderTimeForExternalEncoders) {
On 2016/02/04 11:17:55, åsapersson wrote:
> disabled for non-external encoders?

Done.

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_captu...
File webrtc/video/video_capture_input.h (right):

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_captu...
webrtc/video/video_capture_input.h:19: #include "webrtc/base/scoped_ptr.h"
On 2016/02/04 11:17:55, åsapersson wrote:
> can this be removed?

Done.

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_captu...
webrtc/video/video_capture_input.h:34: class ProcessThread;
On 2016/02/04 11:17:55, åsapersson wrote:
> can this be removed?

Done.

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_captu...
File webrtc/video/video_capture_input_unittest.cc (right):

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_captu...
webrtc/video/video_capture_input_unittest.cc:18: #include
"webrtc/modules/utility/include/mock/mock_process_thread.h"
On 2016/02/04 11:17:55, åsapersson wrote:
> remove?

Done.

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (left):

https://codereview.webrtc.org/1569853002/diff/200001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:980:
video_send_config_.encoding_time_observer = &analyzer;
On 2016/02/04 11:17:55, åsapersson wrote:
> set video_send_config_.post_encode_callback?

Done, had to add a proxy class because analyzer is used as an
EncodedFrameObserver on the receive side.

[pbos-webrtc, 2016-02-04 12:38:10]

asapersson@ feedback

[åsapersson, 2016-02-04 13:51:42]

lgtm

[pbos-webrtc, 2016-02-04 13:57:43]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-04 13:57:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1569853002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1569853002/220001

[commit-bot: I haz the power, 2016-02-04 14:57:05]

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

[commit-bot: I haz the power, 2016-02-04 14:57:06]

Dry run: This issue passed the CQ dry run.

[mflodman, 2016-02-05 08:19:03]

One optional comment nit, LGTM.

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
File webrtc/video/overuse_frame_detector.cc (right):

https://codereview.webrtc.org/1569853002/diff/20001/webrtc/video/overuse_fram...
webrtc/video/overuse_frame_detector.cc:201:
frame_timing_.push_back(FrameTiming(frame.timestamp(), now));
On 2016/01/21 14:18:33, pbos-webrtc wrote:
> On 2016/01/21 08:00:04, mflodman wrote:
> > Is it better to use timestamp than capture_time_ms_?
> 
> We don't get capture_time_ms out of the encoder after encoding. If we're lucky
> we get correct RTP timestamps (true except for ChromeOS).

Maybe this is something we should do in the future.

https://codereview.webrtc.org/1569853002/diff/220001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1569853002/diff/220001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:67: // From CpuOveruseMetricsObserver.
nit: I prefer "// Implements CpuOveruseMetricsObserver." since that is what
we've used elsewhere, but not everywhere :/

[pbos-webrtc, 2016-02-05 10:04:24]

updated comment

[pbos-webrtc, 2016-02-05 10:04:59]

https://codereview.webrtc.org/1569853002/diff/220001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1569853002/diff/220001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:67: // From CpuOveruseMetricsObserver.
On 2016/02/05 08:19:03, mflodman wrote:
> nit: I prefer "// Implements CpuOveruseMetricsObserver." since that is what
> we've used elsewhere, but not everywhere :/
> 

Done.

[pbos-webrtc, 2016-02-05 10:13:46]

Description was changed from

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating overload.

Merges EncodingTimeObserver into EncodedFrameObserver to have one post-encode
callback.

BUG=webrtc:5042, webrtc:5132
R=asapersson@webrtc.org, mflodman@webrtc.org
==========

to

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating overload.

Merges EncodingTimeObserver into EncodedFrameObserver to have one post-encode
callback.

BUG=webrtc:5042, webrtc:5132
R=asapersson@webrtc.org, mflodman@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/e4499154554deb3819343f570...
==========

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

Description was changed from

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating overload.

Merges EncodingTimeObserver into EncodedFrameObserver to have one post-encode
callback.

BUG=webrtc:5042, webrtc:5132
R=asapersson@webrtc.org, mflodman@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/e4499154554deb3819343f570...
==========

to

==========
Measure encoding time on encode callbacks.

Permits measuring encoding time even when performed on another thread,
typically for hardware encoding, instead of assuming that encoding is
blocking the calling thread.

Permitted encoding time is increased for hardware encoders since they
can be timed to keep 30fps, for instance, without indicating overload.

Merges EncodingTimeObserver into EncodedFrameObserver to have one post-encode
callback.

BUG=webrtc:5042, webrtc:5132
R=asapersson@webrtc.org, mflodman@webrtc.org

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

[pbos-webrtc, 2016-02-05 10:13:48]

Committed patchset #13 (id:240001) manually as
e4499154554deb3819343f5708e6b3685868c1f1 (presubmit successful).

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

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