VCMCodecTimer: Change filter from max to 95th percentile

VCMCodecTimer: Change filter from max to 95th percentile

The purpose with this change is to make the filter more robust against anomalies. googMaxDecodeMs is expected to drop a litte by this.

BUG=b/27306053

Committed: https://crrev.com/4bf0c717740d1834e810ea5f32b3c4306c64235f
Cr-Commit-Position: refs/heads/master@{#11952}

[magjed_webrtc, 2016-02-29 16:00:09]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2016-02-29 16:19:03]

Patchset #2 (id:40001) has been deleted

[magjed_webrtc, 2016-02-29 16:19:14]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2016-02-29 16:19:30]

Description was changed from

==========
VCMCodecTimer: Change filter from max to 95th percentile

BUG=b/27306053
==========

to

==========
VCMCodecTimer: Change filter from max to 95th percentile

BUG=b/27306053
==========

[magjed_webrtc, 2016-02-29 16:19:30]

magjed@webrtc.org changed reviewers:
+ stefan@webrtc.org

[magjed_webrtc, 2016-02-29 16:20:26]

stefan - Please take a look. If you approve of this CL, I will add some
unittests of PercentileFilter before landing.

[magjed_webrtc, 2016-03-03 11:30:39]

Ping.

[stefan-webrtc, 2016-03-03 11:32:12]

stefan@webrtc.org changed reviewers:
+ philipel@webrtc.org

[stefan-webrtc, 2016-03-03 11:32:12]

philip, could you take a look at this CL?

[philipel, 2016-03-03 13:34:31]

I think it would be good if you added some unittests for the percentile filter
class.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codec_timer.cc (right):

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.cc:16: static const int32_t
kIgnoredSampleCount = 5;
Move all static variables into a namespace { ... }.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.cc:31: void
VCMCodecTimer::MaxFilter(int32_t decode_time_ms, int64_t now_ms) {
int64_t

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.cc:50: // Get the maximum observed time
within a time window
Update comment about the 95th percentile stuff.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codec_timer.h (right):

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.h:27: void MaxFilter(int32_t
new_decode_time_ms, int64_t now_ms);
Always use int64_t when storing time intervals or timestamps.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.h:27: void MaxFilter(int32_t
new_decode_time_ms, int64_t now_ms);
Is MaxFilter still the correct term to use now? Maybe AddTiming or UpdateFilter
or something like that instead?

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.h:38: int32_t decode_time_ms;
int64_t decode_time_ms;

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.h:43: int32_t ignoredSampleCount_;
Change int32_t to int.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/percentile_filter.cc (right):

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/percentile_filter.cc:65: const int64_t index =
static_cast<int64_t>(percentile_ * (set_.size() - 1));
Does this cast do anything?

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/percentile_filter.h (right):

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/percentile_filter.h:46:
std::multiset<int32_t>::iterator it_;
Change it_ to something more descriptive, like percentile_it_ or something like
that.

[magjed_webrtc, 2016-03-04 14:09:34]

Patchset #3 (id:100001) has been deleted

[magjed_webrtc, 2016-03-04 14:19:41]

Please take another look.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codec_timer.cc (right):

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.cc:16: static const int32_t
kIgnoredSampleCount = 5;
On 2016/03/03 13:34:31, philipel wrote:
> Move all static variables into a namespace { ... }.

Done.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.cc:31: void
VCMCodecTimer::MaxFilter(int32_t decode_time_ms, int64_t now_ms) {
On 2016/03/03 13:34:31, philipel wrote:
> int64_t

Done.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.cc:50: // Get the maximum observed time
within a time window
On 2016/03/03 13:34:31, philipel wrote:
> Update comment about the 95th percentile stuff.

Done.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codec_timer.h (right):

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.h:27: void MaxFilter(int32_t
new_decode_time_ms, int64_t now_ms);
On 2016/03/03 13:34:31, philipel wrote:
> Is MaxFilter still the correct term to use now? Maybe AddTiming or
UpdateFilter
> or something like that instead?

MaxFilter is a strange name indeed, even before my change. I changed it to
AddTiming.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.h:27: void MaxFilter(int32_t
new_decode_time_ms, int64_t now_ms);
On 2016/03/03 13:34:31, philipel wrote:
> Always use int64_t when storing time intervals or timestamps.

Done.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.h:33: int32_t
RequiredDecodeTimeMs(FrameType frameType) const;
Since I'm changing the interface now, I took the opportunity to nuke this
unnecessary FrameType argument.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.h:38: int32_t decode_time_ms;
On 2016/03/03 13:34:31, philipel wrote:
> int64_t decode_time_ms;

Done.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.h:43: int32_t ignoredSampleCount_;
On 2016/03/03 13:34:31, philipel wrote:
> Change int32_t to int.

Done.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/percentile_filter.cc (right):

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/percentile_filter.cc:65: const int64_t index =
static_cast<int64_t>(percentile_ * (set_.size() - 1));
On 2016/03/03 13:34:31, philipel wrote:
> Does this cast do anything?

It compiles without, but I prefer to do it explicitly when casting from float to
int.

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/percentile_filter.h (right):

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/percentile_filter.h:46:
std::multiset<int32_t>::iterator it_;
On 2016/03/03 13:34:31, philipel wrote:
> Change it_ to something more descriptive, like percentile_it_ or something
like
> that.

Done.

https://codereview.webrtc.org/1742323002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/timing.cc (right):

https://codereview.webrtc.org/1742323002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/timing.cc:219: int32_t VCMTiming::MaxDecodeTimeMs(
Do you want me to change this function name as well?

[philipel, 2016-03-04 15:20:13]

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codec_timer.h (right):

https://codereview.webrtc.org/1742323002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codec_timer.h:33: int32_t
RequiredDecodeTimeMs(FrameType frameType) const;
On 2016/03/04 14:19:41, magjed_webrtc wrote:
> Since I'm changing the interface now, I took the opportunity to nuke this
> unnecessary FrameType argument.

Good.

https://codereview.webrtc.org/1742323002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codec_timer.cc (right):

https://codereview.webrtc.org/1742323002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codec_timer.cc:18: static const int
kIgnoredSampleCount = 5;
Remove static from all constants declared in the anonymous namespace.

https://codereview.webrtc.org/1742323002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/timing.cc (right):

https://codereview.webrtc.org/1742323002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/timing.cc:219: int32_t VCMTiming::MaxDecodeTimeMs(
On 2016/03/04 14:19:41, magjed_webrtc wrote:
> Do you want me to change this function name as well?

Yes, RequiredDecodeTimeMs should work here as well.

[magjed_webrtc, 2016-03-06 13:07:12]

Description was changed from

==========
VCMCodecTimer: Change filter from max to 95th percentile

BUG=b/27306053
==========

to

==========
VCMCodecTimer: Change filter from max to 95th percentile

googMaxDecodeMs is expected to drop a litte by this.

BUG=b/27306053
==========

[magjed_webrtc, 2016-03-06 13:07:56]

Description was changed from

==========
VCMCodecTimer: Change filter from max to 95th percentile

googMaxDecodeMs is expected to drop a litte by this.

BUG=b/27306053
==========

to

==========
VCMCodecTimer: Change filter from max to 95th percentile

The purpose with this change is to make the filter more robust against
anomalies. googMaxDecodeMs is expected to drop a litte by this.

BUG=b/27306053
==========

[magjed_webrtc, 2016-03-07 07:37:31]

Patchset #4 (id:140001) has been deleted

[magjed_webrtc, 2016-03-07 07:38:14]

https://codereview.webrtc.org/1742323002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codec_timer.cc (right):

https://codereview.webrtc.org/1742323002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codec_timer.cc:18: static const int
kIgnoredSampleCount = 5;
On 2016/03/04 15:20:13, philipel wrote:
> Remove static from all constants declared in the anonymous namespace.

Done.

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/timing.cc (right):

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/timing.cc:265:
static_cast<uint32_t>(RequiredDecodeTimeMs()) +
I have to delimit this CL somewhere, therefore the static_cast.

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/timing.cc:278: *max_decode_ms =
static_cast<int>(RequiredDecodeTimeMs());
ditto

[philipel, 2016-03-07 14:09:38]

lgtm

[stefan-webrtc, 2016-03-07 14:48:59]

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codec_timer.h (right):

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codec_timer.h:30: void Reset();
Maybe we should remove this method and instead recreate the object. WDYT?

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codec_timer.h:44: int ignoredSampleCount_;
ignored_sample_count_

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/percentile_filter.cc (right):

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/percentile_filter.cc:29: }
I would prefer not having a Clear() method

[magjed_webrtc, 2016-03-08 15:49:42]

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codec_timer.h (right):

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codec_timer.h:30: void Reset();
On 2016/03/07 14:48:59, stefan-webrtc (holmer) wrote:
> Maybe we should remove this method and instead recreate the object. WDYT?

I agree. Done.

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codec_timer.h:44: int ignoredSampleCount_;
On 2016/03/07 14:48:59, stefan-webrtc (holmer) wrote:
> ignored_sample_count_

Done.

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/percentile_filter.cc (right):

https://codereview.webrtc.org/1742323002/diff/160001/webrtc/modules/video_cod...
webrtc/modules/video_coding/percentile_filter.cc:29: }
On 2016/03/07 14:48:59, stefan-webrtc (holmer) wrote:
> I would prefer not having a Clear() method

Done.

[stefan-webrtc, 2016-03-08 16:07:48]

https://codereview.webrtc.org/1742323002/diff/180001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codec_timer.h (right):

https://codereview.webrtc.org/1742323002/diff/180001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codec_timer.h:43: std::queue<Sample> history_;
I haven't looked at the details of PercentileFilter, but do we really need both
a history_ here and the set in PercentileFilter? Maybe it would be simpler to
have the PercentileFilter remove elements from itself when it has too many?

[magjed_webrtc, 2016-03-10 09:14:09]

https://codereview.webrtc.org/1742323002/diff/180001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codec_timer.h (right):

https://codereview.webrtc.org/1742323002/diff/180001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codec_timer.h:43: std::queue<Sample> history_;
On 2016/03/08 16:07:48, stefan-webrtc (holmer) wrote:
> I haven't looked at the details of PercentileFilter, but do we really need
both
> a history_ here and the set in PercentileFilter? Maybe it would be simpler to
> have the PercentileFilter remove elements from itself when it has too many?

Yes, we need both the queue and the multiset in PercentileFilter, if we want it
to be fast. The queue is for fast retrieval of the oldest element, and the
multiset is for fast retrieval of the percentile value. The logic in
VCMCodecTimer adds/removes elements to PercentileFilter to maintain a sliding
time window of observations, and then fetches the percentile value in
RequiredDecodeTimeMs().

If you think this approach is overengineering, a simpler but less efficient
approach is to just have a std::deque with the history, and in
RequiredDecodeTimeMs() copy the history to a std::vector, sort it, and return
the percentile value. I think we can have many hundreds of observations in the
history, so I don't like this inefficient approach. Also, I have already
implemented the efficient approach with tests and everything now...

WDYT?

[stefan-webrtc, 2016-03-10 14:26:02]

lgtm

[magjed_webrtc, 2016-03-10 14:45:00]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2016-03-10 14:45:01]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1742323002/#ps180001 (title:
"Addressing stefan@s comments")

[commit-bot: I haz the power, 2016-03-10 14:45:13]

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

[commit-bot: I haz the power, 2016-03-10 15:22:47]

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

[commit-bot: I haz the power, 2016-03-10 15:22:48]

Try jobs failed on following builders:
  mac_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/9717)

[magjed_webrtc, 2016-03-11 09:15:30]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2016-03-11 09:15:38]

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

[commit-bot: I haz the power, 2016-03-11 10:15:11]

Description was changed from

==========
VCMCodecTimer: Change filter from max to 95th percentile

The purpose with this change is to make the filter more robust against
anomalies. googMaxDecodeMs is expected to drop a litte by this.

BUG=b/27306053
==========

to

==========
VCMCodecTimer: Change filter from max to 95th percentile

The purpose with this change is to make the filter more robust against
anomalies. googMaxDecodeMs is expected to drop a litte by this.

BUG=b/27306053
==========

[commit-bot: I haz the power, 2016-03-11 10:15:13]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2016-03-11 10:15:16]

Description was changed from

==========
VCMCodecTimer: Change filter from max to 95th percentile

The purpose with this change is to make the filter more robust against
anomalies. googMaxDecodeMs is expected to drop a litte by this.

BUG=b/27306053
==========

to

==========
VCMCodecTimer: Change filter from max to 95th percentile

The purpose with this change is to make the filter more robust against
anomalies. googMaxDecodeMs is expected to drop a litte by this.

BUG=b/27306053

Committed: https://crrev.com/4bf0c717740d1834e810ea5f32b3c4306c64235f
Cr-Commit-Position: refs/heads/master@{#11952}
==========

[commit-bot: I haz the power, 2016-03-11 10:15:18]

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

[magjed_webrtc, 2016-03-16 12:38:06]

A revert of this CL (patchset #5 id:180001) has been created in
https://codereview.webrtc.org/1808693002/ by magjed@webrtc.org.

The reason for reverting is: Caused unexpected perf stats changes, see
http://crbug/594575..