Change reporting of timing frames conditions in GetStats on receive side

Change reporting of timing frames conditions in GetStats on receive side

Instead of the longest frame since the last GetStats call, the longest
frame received during last 10 seconds should be returned in GetStats().

Previous way is not a good idea because there are maybe several
consumers of GetStats calls. If not all of them parse timing frame
reports, some of them may be lost.

Also, streamline reporting of TimingFrames via GetStats (remove separate
methods and use VideoReceiveStream::Stats struct instead).


BUG=webrtc:7594
Review-Url: https://codereview.webrtc.org/3008983002
Cr-Commit-Position: refs/heads/master@{#19650}
Committed: https://chromium.googlesource.com/external/webrtc/+/75204c5ccdda561ccffbfa3a8fa94feac8e19e30

[ilnik, 2017-08-31 09:15:43]

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

[commit-bot: I haz the power, 2017-08-31 09:15:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[ilnik, 2017-08-31 09:34:12]

ilnik@webrtc.org changed reviewers:
+ sprang@webrtc.org, tommi@webrtc.org

[sprang_webrtc, 2017-08-31 13:24:43]

https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.h:169: GUARDED_BY(&crit_);
uhg. mutable :(
If it's needed, can you comment why here?

https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy_unittest.cc:340: const int64_t kShortDelay
= 1000;
nit: kShortDelayMs

[ilnik, 2017-08-31 13:32:59]

https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy.h (right):

https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy.h:169: GUARDED_BY(&crit_);
On 2017/08/31 13:24:43, sprang_webrtc wrote:
> uhg. mutable :(
> If it's needed, can you comment why here?

It's necessary here. Commented on it.

https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_stat...
File webrtc/video/receive_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_stat...
webrtc/video/receive_statistics_proxy_unittest.cc:340: const int64_t kShortDelay
= 1000;
On 2017/08/31 13:24:43, sprang_webrtc wrote:
> nit: kShortDelayMs

Done.

[sprang_webrtc, 2017-09-01 09:15:40]

lgtm

[ilnik, 2017-09-01 09:16:25]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-09-01 09:16:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-01 09:21:54]

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

[commit-bot: I haz the power, 2017-09-01 09:21:55]

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

[ilnik, 2017-09-01 09:41:07]

On 2017/09/01 09:21:55, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   presubmit on master.tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21083)

Tommi, PTAL at:
    webrtc/api/video/video_timing.cc
    webrtc/api/video/video_timing.h
    webrtc/call/video_receive_stream.h
    webrtc/media/engine/fakewebrtccall.cc
    webrtc/media/engine/fakewebrtccall.h
    webrtc/media/engine/webrtcvideoengine.cc

[ilnik, 2017-09-01 15:44:51]

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

[commit-bot: I haz the power, 2017-09-01 15:44:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-01 16:26:29]

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

[commit-bot: I haz the power, 2017-09-01 16:26:30]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
  linux_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/27802)

[ilnik, 2017-09-04 07:50:11]

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

[commit-bot: I haz the power, 2017-09-04 07:50:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-04 09:07:05]

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

[commit-bot: I haz the power, 2017-09-04 09:07:06]

Dry run: This issue passed the CQ dry run.

[tommi, 2017-09-04 09:09:21]

lgtm

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_ti...
File webrtc/api/video/video_timing.cc (right):

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.cc:49: 
nit: fix whitespace

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_ti...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:92: bool operator<(const TimingFrameInfo&
other);
nit: should these be const?

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine.cc:2396: info.timing_frame_info =
stats.timing_frame_info;
std::move?

[ilnik, 2017-09-04 09:18:55]

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_ti...
File webrtc/api/video/video_timing.cc (right):

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.cc:49: 
On 2017/09/04 09:09:21, tommi wrote:
> nit: fix whitespace

Done.

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_ti...
File webrtc/api/video/video_timing.h (right):

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_ti...
webrtc/api/video/video_timing.h:92: bool operator<(const TimingFrameInfo&
other);
On 2017/09/04 09:09:21, tommi wrote:
> nit: should these be const?

Done.

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine.cc (right):

https://codereview.webrtc.org/3008983002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine.cc:2396: info.timing_frame_info =
stats.timing_frame_info;
On 2017/09/04 09:09:21, tommi wrote:
> std::move?

From my understanding it will do nothing.
TimingFrameInfo doesn't have any move constructors and Optional<> doesn't use
pointers, so there'll be no speed-up here too.

[ilnik, 2017-09-04 09:19:55]

The CQ bit was checked by ilnik@webrtc.org

[ilnik, 2017-09-04 09:19:56]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3008983002/#ps60001 (title:
"Implement Tommi@ comments")

[commit-bot: I haz the power, 2017-09-04 09:20:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-04 10:35:40]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1504516795913490,
"parent_rev": "5c0f6c62ea3b1d2c43f8fc152961af27033475f7", "commit_rev":
"75204c5ccdda561ccffbfa3a8fa94feac8e19e30"}

[commit-bot: I haz the power, 2017-09-04 10:35:45]

Description was changed from

==========
Change reporting of timing frames conditions in GetStats on receive side

Instead of the longest frame since the last GetStats call, the longest
frame received during last 10 seconds should be returned in GetStats().

Previous way is not a good idea because there are maybe several
consumers of GetStats calls. If not all of them parse timing frame
reports, some of them may be lost.

Also, streamline reporting of TimingFrames via GetStats (remove separate
methods and use VideoReceiveStream::Stats struct instead).


BUG=webrtc:7594
==========

to

==========
Change reporting of timing frames conditions in GetStats on receive side

Instead of the longest frame since the last GetStats call, the longest
frame received during last 10 seconds should be returned in GetStats().

Previous way is not a good idea because there are maybe several
consumers of GetStats calls. If not all of them parse timing frame
reports, some of them may be lost.

Also, streamline reporting of TimingFrames via GetStats (remove separate
methods and use VideoReceiveStream::Stats struct instead).


BUG=webrtc:7594

Review-Url: https://codereview.webrtc.org/3008983002
Cr-Commit-Position: refs/heads/master@{#19650}
Committed:
https://chromium.googlesource.com/external/webrtc/+/75204c5ccdda561ccffbfa3a8...
==========

[commit-bot: I haz the power, 2017-09-04 10:35:46]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/75204c5ccdda561ccffbfa3a8...