Android: Add API for getting native histograms.

Android: Add API for getting native histograms.

The function getAndReset returns a map which holds the name of a histogram and its samples.

This CL depends on: https://codereview.webrtc.org/1915523002/

BUG=

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

[åsapersson, 2016-05-10 08:02:54]

Patchset #2 (id:20001) has been deleted

[åsapersson, 2016-05-18 08:56:55]

Description was changed from

==========
Android: Add API for getting native histograms.

The function getAndReset returns a map which holds the name of a histogram and
its samples.

BUG=
==========

to

==========
Android: Add API for getting native histograms.

The function getAndReset returns a map which holds the name of a histogram and
its samples.

This CL depends on: https://codereview.webrtc.org/1915523002/

BUG=
==========

[åsapersson, 2016-05-18 09:32:38]

asapersson@webrtc.org changed reviewers:
+ magjed@webrtc.org, perkj@webrtc.org

[magjed_webrtc, 2016-05-18 09:49:22]

lgtm

[perkj_webrtc, 2016-05-20 13:09:10]

Sorry I missed this. Just a few minor comments. 

Can you add a test for this, in PeerConnectionUnittest.java for example.

https://codereview.webrtc.org/1952223007/diff/40001/webrtc/api/java/src/org/w...
File webrtc/api/java/src/org/webrtc/Metrics.java (right):

https://codereview.webrtc.org/1952223007/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/Metrics.java:28: // Most histograms are not
updated frequently (e.g. most video metrics are an
I am not sure I understand this meaning. Should  a user only call getAndReset 
when the stream is removed?

https://codereview.webrtc.org/1952223007/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/Metrics.java:59: public void add(String name,
HistogramInfo info) {
why a public add method? Should you be able to add metrics after calling
getAndReset from java? If this is only called from c++ I think it should be
private.

https://codereview.webrtc.org/1952223007/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/Metrics.java:63: // Enables collection of native
histograms.
Can you elaborate a bit in this description. What is it that happens when this
is called? I guess it means that metrics
are gathered that later can be fetched with getAndReset() ?

[åsapersson, 2016-05-23 09:01:51]

Patchset #3 (id:60001) has been deleted

[åsapersson, 2016-05-23 09:03:00]

Patchset #3 (id:80001) has been deleted

[åsapersson, 2016-05-23 09:08:20]

Patchset #3 (id:100001) has been deleted

[åsapersson, 2016-05-23 09:14:03]

https://codereview.webrtc.org/1952223007/diff/40001/webrtc/api/java/src/org/w...
File webrtc/api/java/src/org/webrtc/Metrics.java (right):

https://codereview.webrtc.org/1952223007/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/Metrics.java:28: // Most histograms are not
updated frequently (e.g. most video metrics are an
On 2016/05/20 13:09:10, perkj_webrtc wrote:
> I am not sure I understand this meaning. Should  a user only call getAndReset 
> when the stream is removed? 

Done.

https://codereview.webrtc.org/1952223007/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/Metrics.java:59: public void add(String name,
HistogramInfo info) {
On 2016/05/20 13:09:10, perkj_webrtc wrote:
> why a public add method? Should you be able to add metrics after calling
> getAndReset from java? If this is only called from c++ I think it should be
> private. 

Done.

https://codereview.webrtc.org/1952223007/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/Metrics.java:63: // Enables collection of native
histograms.
On 2016/05/20 13:09:10, perkj_webrtc wrote:
> Can you elaborate a bit in this description. What is it that happens when this
> is called? I guess it means that metrics
> are gathered that later can be fetched with getAndReset() ? 

Done.

https://codereview.webrtc.org/1952223007/diff/120001/webrtc/api/androidtests/...
File webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java (right):

https://codereview.webrtc.org/1952223007/diff/120001/webrtc/api/androidtests/...
webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java:977:
assertTrue(metrics.map.size() > 0);
A certain time must have elapsed in order for video metrics that are an average
over the call (and logged at the end of the call) to be recorded. Therefore
testing a metric that is recorded when a stream is created.

[perkj_webrtc, 2016-05-23 12:50:27]

lgtm but I think you should consider testing for metrics that depend on that
video is working.

https://codereview.webrtc.org/1952223007/diff/120001/webrtc/api/androidtests/...
File webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java (right):

https://codereview.webrtc.org/1952223007/diff/120001/webrtc/api/androidtests/...
webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java:977:
assertTrue(metrics.map.size() > 0);
On 2016/05/23 09:14:03, åsapersson wrote:
> A certain time must have elapsed in order for video metrics that are an
average
> over the call (and logged at the end of the call) to be recorded. Therefore
> testing a metric that is recorded when a stream is created.

the test waits for 10 frames to be rendered so you should be able to test that
at least 10 frames have been encoded / rendered if there are any such stats.

[åsapersson, 2016-05-23 13:01:24]

https://codereview.webrtc.org/1952223007/diff/120001/webrtc/api/androidtests/...
File webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java (right):

https://codereview.webrtc.org/1952223007/diff/120001/webrtc/api/androidtests/...
webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java:977:
assertTrue(metrics.map.size() > 0);
On 2016/05/23 12:50:27, perkj_webrtc wrote:
> On 2016/05/23 09:14:03, åsapersson wrote:
> > A certain time must have elapsed in order for video metrics that are an
> average
> > over the call (and logged at the end of the call) to be recorded. Therefore
> > testing a metric that is recorded when a stream is created.
> 
> the test waits for 10 frames to be rendered so you should be able to test that
> at least 10 frames have been encoded / rendered if there are any such stats. 

There are no stats for these after 10 frames.

[åsapersson, 2016-05-23 13:39:30]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2016-05-23 13:39:31]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1952223007/#ps120001 (title:
"address comments")

[commit-bot: I haz the power, 2016-05-23 13:39:45]

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

[commit-bot: I haz the power, 2016-05-23 13:49:38]

Description was changed from

==========
Android: Add API for getting native histograms.

The function getAndReset returns a map which holds the name of a histogram and
its samples.

This CL depends on: https://codereview.webrtc.org/1915523002/

BUG=
==========

to

==========
Android: Add API for getting native histograms.

The function getAndReset returns a map which holds the name of a histogram and
its samples.

This CL depends on: https://codereview.webrtc.org/1915523002/

BUG=
==========

[commit-bot: I haz the power, 2016-05-23 13:49:39]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-05-23 13:49:48]

Description was changed from

==========
Android: Add API for getting native histograms.

The function getAndReset returns a map which holds the name of a histogram and
its samples.

This CL depends on: https://codereview.webrtc.org/1915523002/

BUG=
==========

to

==========
Android: Add API for getting native histograms.

The function getAndReset returns a map which holds the name of a histogram and
its samples.

This CL depends on: https://codereview.webrtc.org/1915523002/

BUG=

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

[commit-bot: I haz the power, 2016-05-23 13:49:49]

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