Add stats overlay to iOS AppRTCDemo.

Add stats overlay to iOS AppRTCDemo.

BUG=
R=jiayl@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/d33258098b99068064e0d4a8cfc6566dec8e881e

[tkchin_webrtc, 2015-08-13 21:24:14]

Patchset #1 (id:1) has been deleted

[tkchin_webrtc, 2015-08-13 21:32:07]

Patchset #1 (id:20001) has been deleted

[tkchin_webrtc, 2015-08-13 21:33:05]

tkchin@webrtc.org changed reviewers:
+ haysc@webrtc.org, jiayl@webrtc.org

[jiayl2, 2015-08-13 22:21:50]

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right):

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:171: -
(void)setShouldGetStats:(BOOL)shouldGetStats {
I didn't find who calls this method...

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:180: ARDAppClient *strongSelf =
weakSelf;
OOC: what's the magic with stringSelf/weakSelf?

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:181: [strongSelf.peerConnection
getStatsWithDelegate:strongSelf
Is peerConnection guaranteed to exist?

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:186: [_statsTimer invalidate];
Is this guaranteed to be called?
Do we need to release in dealloc if not?

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCDemo/ARDBitrateTracker.m (right):

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDBitrateTracker.m:41: if (byteCount >
_prevByteCount) {
I think we should update prev byte count and time in the else case too, e.g.
webrtc counter got reset after ICE restart. We don't want to be stuck in that
case.

The whole block can be simplified to:

if (_prevTime && byteCount > _prevByteCount) {
  _bitrate = ...
}
_prevByteCount = ...
_prevTime = ...

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCDemo/ARDStatsBuilder.m (right):

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDStatsBuilder.m:87: // TODO(tkchin): CPU
utilization.
nit: make the TODO more clear about what's needed to do.

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCDemo/common/ARDUtilities.m (right):

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/common/ARDUtilities.m:100: // Create an array of
thread ports for the current task
nit: '.' at the end of a comment. here and other places.

[tkchin_webrtc, 2015-08-13 23:55:31]

PTAL

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCDemo/ARDAppClient.m (right):

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:171: -
(void)setShouldGetStats:(BOOL)shouldGetStats {
On 2015/08/13 22:21:50, jiayl2 wrote:
> I didn't find who calls this method...

.shouldGetStats = is a property setter, it calls setShouldGetStats

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:180: ARDAppClient *strongSelf =
weakSelf;
On 2015/08/13 22:21:50, jiayl2 wrote:
> OOC: what's the magic with stringSelf/weakSelf?

We don't want to add a refcount to this object in the timerHandler, if we
referenced self directly we would've done so. So we create a weak pointer
instead and access that within the block. However, within the block we need to
convert the weak pointer to a strong pointer first, otherwise it's possible that
earlier calls will succeed, but later calls will fail (if the weak pointer
disappears in the middle of the block).

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:181: [strongSelf.peerConnection
getStatsWithDelegate:strongSelf
On 2015/08/13 22:21:49, jiayl2 wrote:
> Is peerConnection guaranteed to exist?

No. Messages sent to nil are no-ops so this is safe.

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDAppClient.m:186: [_statsTimer invalidate];
On 2015/08/13 22:21:50, jiayl2 wrote:
> Is this guaranteed to be called?
> Do we need to release in dealloc if not?

This is being released in dealloc by setting to NO.

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCDemo/ARDBitrateTracker.m (right):

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDBitrateTracker.m:41: if (byteCount >
_prevByteCount) {
On 2015/08/13 22:21:50, jiayl2 wrote:
> I think we should update prev byte count and time in the else case too, e.g.
> webrtc counter got reset after ICE restart. We don't want to be stuck in that
> case.
> 
> The whole block can be simplified to:
> 
> if (_prevTime && byteCount > _prevByteCount) {
>   _bitrate = ...
> }
> _prevByteCount = ...
> _prevTime = ...

Done. Needed to add the active connection check. I was getting bad values
because of that.

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCDemo/ARDStatsBuilder.m (right):

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/ARDStatsBuilder.m:87: // TODO(tkchin): CPU
utilization.
On 2015/08/13 22:21:50, jiayl2 wrote:
> nit: make the TODO more clear about what's needed to do.

Nothing left, that was a TODO for myself in this CL that I forgot to remove :)

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCDemo/common/ARDUtilities.m (right):

https://codereview.webrtc.org/1289623005/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCDemo/common/ARDUtilities.m:100: // Create an array of
thread ports for the current task
On 2015/08/13 22:21:50, jiayl2 wrote:
> nit: '.' at the end of a comment. here and other places.

Done.

[jiayl2, 2015-08-14 16:25:31]

lgtm

[tkchin_webrtc, 2015-08-14 18:00:16]

Committed patchset #2 (id:50001) manually as
d33258098b99068064e0d4a8cfc6566dec8e881e (presubmit successful).