Implement framesEncoded stat in video send ssrc stats.

Implement framesEncoded stat in video send ssrc stats.

Implemented as defined by this pull request: https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
Committed: https://crrev.com/43536c3d6a728407adbaabc5b2f2d7a42234234a
Cr-Commit-Position: refs/heads/master@{#14734}

[sakal, 2016-10-17 13:37:59]

Description was changed from

==========
Implement frameEncoded stat in RTCOutboundRTPStreamStats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

to

==========
Implement framesEncoded stat in RTCOutboundRTPStreamStats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

[sakal, 2016-10-17 13:41:30]

sakal@webrtc.org changed reviewers:
+ tommi@webrtc.org

[sakal, 2016-10-17 13:41:30]

PTAL

[hta-webrtc, 2016-10-17 13:47:44]

hta@webrtc.org changed reviewers:
+ hbos@webrtc.org

[hta-webrtc, 2016-10-17 13:48:44]

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

[hta-webrtc, 2016-10-17 13:48:45]

Added hbos as reviewer since he's been doing the implementation of the "new
API", which is what generates RTCOutboundStreamStats.

I see no changes in unit tests; this is a red flag.

[sakal, 2016-10-17 14:04:07]

Right, I didn't realize we had unittests for these components. I will update
them.

[sakal, 2016-10-17 14:43:39]

I've now added a few unittests.

[hta-webrtc, 2016-10-17 16:11:09]

lgtm
(but wait for hbos' review)
Thanks for adding the tests so fast!

[tommi, 2016-10-17 22:54:36]

lgtm

https://codereview.webrtc.org/2421193003/diff/20001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/2421193003/diff/20001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:446: stats_.frames_encoded++;
nit: ++foo

https://codereview.webrtc.org/2421193003/diff/20001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/2421193003/diff/20001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy_unittest.cc:308: for (int i = 1; i <= 3; i++)
{
++i

[sakal, 2016-10-18 07:59:50]

https://codereview.webrtc.org/2421193003/diff/20001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/2421193003/diff/20001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:446: stats_.frames_encoded++;
On 2016/10/17 22:54:36, tommi (webrtc) wrote:
> nit: ++foo

Done.

https://codereview.webrtc.org/2421193003/diff/20001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/2421193003/diff/20001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy_unittest.cc:308: for (int i = 1; i <= 3; i++)
{
On 2016/10/17 22:54:36, tommi (webrtc) wrote:
> ++i

Done.

[sakal, 2016-10-18 08:01:46]

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

[commit-bot: I haz the power, 2016-10-18 08:01:52]

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, 2016-10-18 08:18:39]

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

[commit-bot: I haz the power, 2016-10-18 08:18:40]

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

[hbos, 2016-10-18 18:12:30]

Can you add a statscollector_unittest.cc for this stat?
Is this added to a "Channel-#" report? I realize there is no
"RTCOutboundRTPStreamStats" in statstypes.h/cc. (I'm addressing big
discrepancies like that by rewriting stats collection but that is parallel work,
frames_encoded is needed regardless of which stats collector is used.)

[hbos, 2016-10-19 15:38:43]

On 2016/10/18 18:12:30, hbos wrote:
> Can you add a statscollector_unittest.cc for this stat?
> Is this added to a "Channel-#" report? I realize there is no
> "RTCOutboundRTPStreamStats" in statstypes.h/cc. (I'm addressing big
> discrepancies like that by rewriting stats collection but that is parallel
work,
> frames_encoded is needed regardless of which stats collector is used.)

Nvm, this is not being added to a "Channel-blahblah" report. It is being added
to a non-remote "ssrc" for output of video (this stat should not exist for
audio). I think this is the correct place to add the stat considering we don't
have reports of type RTCOutboundRTPStreamStats.
I'd like to see a unittest in statscollector_unittest.cc, but it already seems
to lack testing of other related stats?

[sakal, 2016-10-20 11:02:45]

Description was changed from

==========
Implement framesEncoded stat in RTCOutboundRTPStreamStats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

to

==========
Implement framesEncoded stat in video send ssrc stats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

[sakal, 2016-10-20 11:05:39]

Patchset #4 (id:60001) has been deleted

[sakal, 2016-10-20 11:07:49]

On 2016/10/19 15:38:43, hbos wrote:
> On 2016/10/18 18:12:30, hbos wrote:
> > Can you add a statscollector_unittest.cc for this stat?
> > Is this added to a "Channel-#" report? I realize there is no
> > "RTCOutboundRTPStreamStats" in statstypes.h/cc. (I'm addressing big
> > discrepancies like that by rewriting stats collection but that is parallel
> work,
> > frames_encoded is needed regardless of which stats collector is used.)
> 
> Nvm, this is not being added to a "Channel-blahblah" report. It is being added
> to a non-remote "ssrc" for output of video (this stat should not exist for
> audio). I think this is the correct place to add the stat considering we don't
> have reports of type RTCOutboundRTPStreamStats.
> I'd like to see a unittest in statscollector_unittest.cc, but it already seems
> to lack testing of other related stats?

Thanks for explaining this. I wasn't aware our implementation is completely out
of sync with the spec and it was confusing me. I added an unittest as requested.
PTAL

[hbos, 2016-10-20 14:45:36]

lgtm

[sakal, 2016-10-20 14:48:44]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2016-10-20 14:48:45]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
hta@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2421193003/#ps80001 (title:
"statscollector unittest")

[commit-bot: I haz the power, 2016-10-20 14:48:55]

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

[hbos, 2016-10-20 16:45:08]

Just noticed one thing.

https://codereview.webrtc.org/2421193003/diff/80001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2421193003/diff/80001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:687: int frames_encoded;
Fix in this or a follow-up: In the linked spec this is "unsigned long", this is
WebIDL for uint32_t, not int.

https://codereview.webrtc.org/2421193003/diff/80001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.webrtc.org/2421193003/diff/80001/webrtc/video_send_stream....
webrtc/video_send_stream.h:59: int frames_encoded = 0;
Same here, uint32_t.

[commit-bot: I haz the power, 2016-10-20 16:49:26]

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

[commit-bot: I haz the power, 2016-10-20 16:49:27]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_tsan2 on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  presubmit on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[hbos, 2016-10-20 16:57:14]

On 2016/10/20 16:45:08, hbos wrote:
> Just noticed one thing.
> 
>
https://codereview.webrtc.org/2421193003/diff/80001/webrtc/media/base/mediach...
> File webrtc/media/base/mediachannel.h (right):
> 
>
https://codereview.webrtc.org/2421193003/diff/80001/webrtc/media/base/mediach...
> webrtc/media/base/mediachannel.h:687: int frames_encoded;
> Fix in this or a follow-up: In the linked spec this is "unsigned long", this
is
> WebIDL for uint32_t, not int.
> 
> https://codereview.webrtc.org/2421193003/diff/80001/webrtc/video_send_stream.h
> File webrtc/video_send_stream.h (right):
> 
>
https://codereview.webrtc.org/2421193003/diff/80001/webrtc/video_send_stream....
> webrtc/video_send_stream.h:59: int frames_encoded = 0;
> Same here, uint32_t.

Oh right, the current stats only support int. But the future stats will be able
to distinguish between int32_t, uint32_t, int64_t and uint64_t. It's better to
do the cast before doing AddInt in statscollector.cc than collecting the wrong
type to begin with.

[sakal, 2016-10-21 08:38:34]

https://codereview.chromium.org/2421193003/diff/80001/webrtc/media/base/media...
File webrtc/media/base/mediachannel.h (right):

https://codereview.chromium.org/2421193003/diff/80001/webrtc/media/base/media...
webrtc/media/base/mediachannel.h:687: int frames_encoded;
On 2016/10/20 16:45:08, hbos wrote:
> Fix in this or a follow-up: In the linked spec this is "unsigned long", this
is
> WebIDL for uint32_t, not int.

Done.

https://codereview.chromium.org/2421193003/diff/80001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.chromium.org/2421193003/diff/80001/webrtc/video_send_strea...
webrtc/video_send_stream.h:59: int frames_encoded = 0;
On 2016/10/20 16:45:08, hbos wrote:
> Same here, uint32_t.

Done.

[sakal, 2016-10-21 08:38:46]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2016-10-21 08:38:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
hta@webrtc.org, hbos@webrtc.org
Link to the patchset: https://codereview.chromium.org/2421193003/#ps100001
(title: "Change type of stat int -> uint32_t.")

[commit-bot: I haz the power, 2016-10-21 08:38:55]

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, 2016-10-21 10:39:36]

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

[commit-bot: I haz the power, 2016-10-21 10:39:37]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_arm on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_asan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_compile_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_tsan2 on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_ubsan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  presubmit on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[sakal, 2016-10-21 13:45:35]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2016-10-21 13:45:48]

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, 2016-10-21 15:46:22]

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

[commit-bot: I haz the power, 2016-10-21 15:46:24]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[sakal, 2016-10-24 07:16:01]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2016-10-24 07:16:08]

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, 2016-10-24 08:46:46]

Description was changed from

==========
Implement framesEncoded stat in video send ssrc stats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

to

==========
Implement framesEncoded stat in video send ssrc stats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

[commit-bot: I haz the power, 2016-10-24 08:46:47]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-10-24 09:26:57]

Description was changed from

==========
Implement framesEncoded stat in video send ssrc stats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541
==========

to

==========
Implement framesEncoded stat in video send ssrc stats.

Implemented as defined by this pull request:
https://github.com/w3c/webrtc-stats/pull/70

BUG=webrtc:6541

Committed: https://crrev.com/43536c3d6a728407adbaabc5b2f2d7a42234234a
Cr-Commit-Position: refs/heads/master@{#14734}
==========

[commit-bot: I haz the power, 2016-10-24 09:26:59]

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