Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(24)

Issue 8310001: Show framerate in chromoting debug log (Closed)

Created:
9 years, 2 months ago by Alpha Left Google
Modified:
9 years, 2 months ago
Reviewers:
Jamie, Wez, garykac
CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org
Visibility:
Public.

Description

Show framerate in chromoting debug log Add frame rate as a statistic that we collect. This can be used to determine the size of video frames that we're sending. Frame rate recorded is the number of frames received per second but not the frame rate displayed. BUG=None TEST=chromoting runs just fine Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107121

Patch Set 1 #

Total comments: 2

Patch Set 2 : fixed nits #

Patch Set 3 : fixed comments #

Patch Set 4 : oops.. compile error. #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -3 lines) Patch
M remoting/client/chromoting_client.cc View 1 chunk +3 lines, -0 lines 1 comment Download
M remoting/client/chromoting_stats.h View 2 chunks +2 lines, -0 lines 0 comments Download
M remoting/client/chromoting_stats.cc View 1 2 3 2 chunks +10 lines, -3 lines 2 comments Download
M remoting/client/plugin/chromoting_scriptable_object.h View 1 chunk +2 lines, -0 lines 1 comment Download
M remoting/client/plugin/chromoting_scriptable_object.cc View 3 chunks +4 lines, -0 lines 1 comment Download
M remoting/webapp/me2mom/client_session.js View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M remoting/webapp/me2mom/remoting.js View 1 2 1 chunk +3 lines, -0 lines 1 comment Download

Messages

Total messages: 10 (0 generated)
Alpha Left Google
9 years, 2 months ago (2011-10-15 16:29:25 UTC) #1
Wez
http://codereview.chromium.org/8310001/diff/1/remoting/client/chromoting_stats.cc File remoting/client/chromoting_stats.cc (right): http://codereview.chromium.org/8310001/diff/1/remoting/client/chromoting_stats.cc#newcode14 remoting/client/chromoting_stats.cc:14: // time window. nit: Specify the units.
9 years, 2 months ago (2011-10-15 21:21:39 UTC) #2
Jamie
http://codereview.chromium.org/8310001/diff/1/remoting/client/chromoting_stats.cc File remoting/client/chromoting_stats.cc (right): http://codereview.chromium.org/8310001/diff/1/remoting/client/chromoting_stats.cc#newcode14 remoting/client/chromoting_stats.cc:14: // time window. On 2011/10/15 21:21:39, Wez wrote: > ...
9 years, 2 months ago (2011-10-15 22:38:27 UTC) #3
Alpha Left Google
On 2011/10/15 22:38:27, Jamie wrote: > http://codereview.chromium.org/8310001/diff/1/remoting/client/chromoting_stats.cc > File remoting/client/chromoting_stats.cc (right): > > http://codereview.chromium.org/8310001/diff/1/remoting/client/chromoting_stats.cc#newcode14 > ...
9 years, 2 months ago (2011-10-19 16:11:30 UTC) #4
Wez
The fps statistic should make it clear that this is the number of frames we're ...
9 years, 2 months ago (2011-10-19 21:53:54 UTC) #5
Jamie
What do you mean by "make it clear"? I don't think we should make the ...
9 years, 2 months ago (2011-10-19 21:57:24 UTC) #6
Wez
On 2011/10/19 21:57:24, Jamie wrote: > What do you mean by "make it clear"? I ...
9 years, 2 months ago (2011-10-20 00:14:17 UTC) #7
Alpha Left Google
On 2011/10/20 00:14:17, Wez wrote: > On 2011/10/19 21:57:24, Jamie wrote: > > What do ...
9 years, 2 months ago (2011-10-20 10:25:39 UTC) #8
Alpha Left Google
ping.
9 years, 2 months ago (2011-10-21 16:28:55 UTC) #9
Wez
9 years, 2 months ago (2011-10-21 18:01:43 UTC) #10
LGTM but please address issues below.

http://codereview.chromium.org/8310001/diff/7010/remoting/client/chromoting_c...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/8310001/diff/7010/remoting/client/chromoting_c...
remoting/client/chromoting_client.cc:106: // Add one frame to the counter.
nit: Could fold this text into the previous comment and put the two lines of
code together, since they both relate to the video frame.

http://codereview.chromium.org/8310001/diff/7010/remoting/client/chromoting_s...
File remoting/client/chromoting_stats.cc (right):

http://codereview.chromium.org/8310001/diff/7010/remoting/client/chromoting_s...
remoting/client/chromoting_stats.cc:12: base::TimeDelta::FromSeconds(3);
I think these still need to be const int, and be converted to TimeDelta only
where necessary, otherwise you're adding static initializers.

http://codereview.chromium.org/8310001/diff/7010/remoting/client/chromoting_s...
remoting/client/chromoting_stats.cc:17: base::TimeDelta::FromSeconds(3);
Why do we need separate frame-rate and bandwidth windows?  Seems logical to use
the same one.

http://codereview.chromium.org/8310001/diff/7010/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_scriptable_object.cc (right):

http://codereview.chromium.org/8310001/diff/7010/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_scriptable_object.cc:36: const char
kVideoFramerateAttribute[] = "videoFramerate";
videoFramerate -> videoFrameRate

http://codereview.chromium.org/8310001/diff/7010/remoting/client/plugin/chrom...
File remoting/client/plugin/chromoting_scriptable_object.h (right):

http://codereview.chromium.org/8310001/diff/7010/remoting/client/plugin/chrom...
remoting/client/plugin/chromoting_scriptable_object.h:72: //   readonly
attribute float videoFramerate;
videoFramerate -> videoFrameRate

http://codereview.chromium.org/8310001/diff/7010/remoting/webapp/me2mom/remot...
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/8310001/diff/7010/remoting/webapp/me2mom/remot...
remoting/webapp/me2mom/remoting.js:471: ', Framerate: ' +
Framerate -> Frame Rate

Powered by Google App Engine
This is Rietveld 408576698