Add a utility to adjust rendering quality based on rendering performance.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19795

[gman, 2009-07-01 03:37:25]

http://codereview.chromium.org/147258/diff/2001/2003
File samples/beachdemo/beachdemo.js (right):

http://codereview.chromium.org/147258/diff/2001/2003#newcode1548
Line 1548: g_perfMon = new o3djs.util.PerformanceMonitor(
Do we want to follow the standard we've been using so far which is to have a
creation function instead of directly constructing just to be consistent?

http://codereview.chromium.org/147258/diff/2001/2003#newcode1914
Line 1914: function decreaseRenderTargetResolution() {
will this be called continually if the frame rate never gets above 25? Maybe
that doesn't matter, just asking. It feels like I kind of want to be able to say
"Stop bugging me, I can't do anything about it"

Same above. If I'm going faster than 35hz increase will continue to get called. 
Just a suggestion. Maybe Increase and Decrease should be required to return a
bool as in "true = I can increase even more if asked" and "false = I hit my
limit, don't ask anymore"?

http://codereview.chromium.org/147258/diff/2001/2003#newcode2602
Line 2602: var y = 10 + ii * (g_reflectionTexture.height * scale + 10);
see comment below about ...Texture.width

http://codereview.chromium.org/147258/diff/2001/2003#newcode2608
Line 2608: image.transform.scale(renderTargetTexture.width * scale + borderSize
* 2,
I'm guessing you want this to be a fixed size. I probably works now because
renderTargetTexture happens to be 256 when the app starts but it seems like you
should just put 256 here instead of renderTargetTexture.width

http://codereview.chromium.org/147258/diff/2001/2004
File samples/o3djs/util.js (right):

http://codereview.chromium.org/147258/diff/2001/2004#newcode928
Line 928: /**
I don't think this should be in util.  Util is loaded by all apps since
makeClients is in it.

how about "performance.js"?  or "perf.js"?

http://codereview.chromium.org/147258/diff/2001/2004#newcode932
Line 932: * @param {!number} targetFPSMin the minimum acceptable frame rate; if
we're
number does not need a !

http://codereview.chromium.org/147258/diff/2001/2004#newcode935
Line 935: * @param {!Function} increaseQuality a function to increase
quality/lower FPS.
Function should be in like this

{!function(): void}

or if you take the suggestion about returning a bool it should be

{!function(): boolean}

http://codereview.chromium.org/147258/diff/2001/2004#newcode942
Line 942: this.targetFrameTimeMax = 1 / targetFPSMin;
these should end in _ if they are meant to be private.

If they are meant to be public then they each need comments.  See
o3djs.canvas.CanvasInfo in canvas.js for example.

http://codereview.chromium.org/147258/diff/2001/2004#newcode948
Line 948: if (opt_options.opt_minSamples) {
the style guide prefers

this.minSamples = opt_options.opt_minSamples || 60;

http://www.corp.google.com/eng/doc/javascriptguide.html#andor

http://codereview.chromium.org/147258/diff/2001/2004#newcode982
Line 982: *   opt_minSamples: !number,
s/!//

http://codereview.chromium.org/147258/diff/2001/2004#newcode989
Line 989: o3djs.util.PerformanceMonitor.prototype.onRender =
function(renderEvent) {
sadly we probably should not pass renderEvent (I should probably fix fps.js as
well) The reason is renderEvent.elapsedTime is a both a cross process boundry
function call if this js is in Chrome and, if it's instead in the build in V8
the plugin returns a browser Number object and the V8 then has to call into the
browser to get the value which again if it's Chrome is a cross process thing.

So, arguably, best practices is the user's onRender should get elaspedTime in a
variable as in

var elapsedTime = renderEvent.elapsedTime.

That will convert the browser NumberObject to a local number Object or something
like that and then that gets pass around to functions that need it already doing
the best thing.

[Eric U., 2009-07-01 18:34:12]

http://codereview.chromium.org/147258/diff/2001/2003
File samples/beachdemo/beachdemo.js (right):

http://codereview.chromium.org/147258/diff/2001/2003#newcode1548
Line 1548: g_perfMon = new o3djs.util.PerformanceMonitor(
On 2009/07/01 03:37:25, gman wrote:
> Do we want to follow the standard we've been using so far which is to have a
> creation function instead of directly constructing just to be consistent?
> 
> 

Done.

http://codereview.chromium.org/147258/diff/2001/2003#newcode1914
Line 1914: function decreaseRenderTargetResolution() {
On 2009/07/01 03:37:25, gman wrote:
> will this be called continually if the frame rate never gets above 25? Maybe
> that doesn't matter, just asking. It feels like I kind of want to be able to
say
> "Stop bugging me, I can't do anything about it"
> 
> Same above. If I'm going faster than 35hz increase will continue to get
called. 
> Just a suggestion. Maybe Increase and Decrease should be required to return a
> bool as in "true = I can increase even more if asked" and "false = I hit my
> limit, don't ask anymore"?

Yes, it'll get called every delayCycles.
However, as implemented in the beach demo, that won't do anything.  The decision
of whether to do something or not has to get made in one of the two places--the
library or the user's function--and I don't see a big plus to moving it.  If we
push it into the library, it's another check to make each time through.  If we
don't, we make an extra no-op function call each time instead.  I'm inclined to
leave it as is unless you feel strongly about it.

http://codereview.chromium.org/147258/diff/2001/2003#newcode2602
Line 2602: var y = 10 + ii * (g_reflectionTexture.height * scale + 10);
On 2009/07/01 03:37:25, gman wrote:
> see comment below about ...Texture.width

Done.

http://codereview.chromium.org/147258/diff/2001/2003#newcode2608
Line 2608: image.transform.scale(renderTargetTexture.width * scale + borderSize
* 2,
On 2009/07/01 03:37:25, gman wrote:
> I'm guessing you want this to be a fixed size. I probably works now because
> renderTargetTexture happens to be 256 when the app starts but it seems like
you
> should just put 256 here instead of renderTargetTexture.width

Done.

http://codereview.chromium.org/147258/diff/2001/2004
File samples/o3djs/util.js (right):

http://codereview.chromium.org/147258/diff/2001/2004#newcode928
Line 928: /**
On 2009/07/01 03:37:25, gman wrote:
> I don't think this should be in util.  Util is loaded by all apps since
> makeClients is in it.
> 
> how about "performance.js"?  or "perf.js"?

Done.

http://codereview.chromium.org/147258/diff/2001/2004#newcode932
Line 932: * @param {!number} targetFPSMin the minimum acceptable frame rate; if
we're
On 2009/07/01 03:37:25, gman wrote:
> number does not need a !

Done.

http://codereview.chromium.org/147258/diff/2001/2004#newcode935
Line 935: * @param {!Function} increaseQuality a function to increase
quality/lower FPS.
On 2009/07/01 03:37:25, gman wrote:
> Function should be in like this
> 
> {!function(): void}
> 
> or if you take the suggestion about returning a bool it should be
> 
> {!function(): boolean}
> 

Done.

http://codereview.chromium.org/147258/diff/2001/2004#newcode942
Line 942: this.targetFrameTimeMax = 1 / targetFPSMin;
On 2009/07/01 03:37:25, gman wrote:
> these should end in _ if they are meant to be private.
> 
> If they are meant to be public then they each need comments.  See
> o3djs.canvas.CanvasInfo in canvas.js for example.
> 

Done.

http://codereview.chromium.org/147258/diff/2001/2004#newcode948
Line 948: if (opt_options.opt_minSamples) {
On 2009/07/01 03:37:25, gman wrote:
> the style guide prefers
> 
> this.minSamples = opt_options.opt_minSamples || 60;
> 
> http://www.corp.google.com/eng/doc/javascriptguide.html#andor

Done.

http://codereview.chromium.org/147258/diff/2001/2004#newcode982
Line 982: *   opt_minSamples: !number,
On 2009/07/01 03:37:25, gman wrote:
> s/!//

Done.

http://codereview.chromium.org/147258/diff/2001/2004#newcode989
Line 989: o3djs.util.PerformanceMonitor.prototype.onRender =
function(renderEvent) {
On 2009/07/01 03:37:25, gman wrote:
> sadly we probably should not pass renderEvent (I should probably fix fps.js as
> well) The reason is renderEvent.elapsedTime is a both a cross process boundry
> function call if this js is in Chrome and, if it's instead in the build in V8
> the plugin returns a browser Number object and the V8 then has to call into
the
> browser to get the value which again if it's Chrome is a cross process thing.
> 
> So, arguably, best practices is the user's onRender should get elaspedTime in
a
> variable as in
> 
> var elapsedTime = renderEvent.elapsedTime.
> 
> That will convert the browser NumberObject to a local number Object or
something
> like that and then that gets pass around to functions that need it already
doing
> the best thing.

Done; just passing the elapsedTime as a parameter should convert it to a
variable, right?

[gman, 2009-07-01 22:33:45]

Just a couple of comments and a type issue. Otherwise LGTM

Oh, and you need to update samples/js_list.scons and type scons MODE=docs to
make sure it compiles before commiting

http://codereview.chromium.org/147258/diff/2010/2013
File samples/o3djs/performance.js (right):

http://codereview.chromium.org/147258/diff/2010/2013#newcode47
Line 47: * @param {function(): void} increaseQuality a function to increase
should be (!function(): void}

http://codereview.chromium.org/147258/diff/2010/2013#newcode48
Line 48: *     quality/lower FPS.
"/lower FPS." This is confusing I think. Why would I ever want to lower the FPS?

How about "a function to increase quality." or "a function to increase quality
because the frame rate is so high that we can probably afford to render more
details"

http://codereview.chromium.org/147258/diff/2010/2013#newcode50
Line 50: *     quality/raise FPS.
I think this one is less confusing but it also might be worded better like "a
function to decrease quality to hopefully increase the frame rate".

[Eric U., 2009-07-02 00:54:51]

Done and committed.

http://codereview.chromium.org/147258/diff/2010/2013
File samples/o3djs/performance.js (right):

http://codereview.chromium.org/147258/diff/2010/2013#newcode47
Line 47: * @param {function(): void} increaseQuality a function to increase
On 2009/07/01 22:33:45, gman wrote:
> should be (!function(): void}
> 

Done.

http://codereview.chromium.org/147258/diff/2010/2013#newcode48
Line 48: *     quality/lower FPS.
On 2009/07/01 22:33:45, gman wrote:
> "/lower FPS." This is confusing I think. Why would I ever want to lower the
FPS?
> 
> How about "a function to increase quality." or "a function to increase quality
> because the frame rate is so high that we can probably afford to render more
> details"

Done.

http://codereview.chromium.org/147258/diff/2010/2013#newcode50
Line 50: *     quality/raise FPS.
On 2009/07/01 22:33:45, gman wrote:
> I think this one is less confusing but it also might be worded better like "a
> function to decrease quality to hopefully increase the frame rate".
> 

Done.