[Webkit-unassigned] [Bug 88268] [chromium] Expose rendering statistics to WebLayerTreeView.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=88268





--- Comment #9 from Dave Tu <dtu at chromium.org>  2012-06-12 17:12:43 PST ---
(From update of attachment 146644)
View in context: https://bugs.webkit.org/attachment.cgi?id=146644&action=review

>> Source/Platform/chromium/public/WebLayerTreeView.h:178
>> +    // information about the state of the compositor.
> 
> You don't need to line wrap so aggressively (there's no 80 col limit in WebKit). This would fit easily on one line.

Done.

>> Source/Platform/chromium/public/WebLayerTreeView.h:179
>> +    WEBKIT_EXPORT void renderingStats(WebRenderingStats&);
> 
> This is a very expensive call relative to other calls on this interface since it blocks on the compositor thread. I think you should move it into the "Debugging / dangerous" section of this header and add some documentation here about how heavy the call is.

Done.

>> Source/WebKit/chromium/public/WebRenderingStats.h:1
>> +/*
> 
> If you want to use this struct in Platform/chromium/public/WebLayerTreeView.h, you need to define this struct in Platform/chromium/public/

Done.

>> Source/WebKit/chromium/public/WebRenderingStats.h:2
>> + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> it's 2012

Done.

>> Source/WebKit/chromium/public/WebRenderingStats.h:31
>> +struct WebRenderingStats {
> 
> please add a constructor that zeros out the fields.  Otherwise if someone writes the very reasonable-looking code:
> 
> {
>   WebRenderingStats stats;
>   myWebView->renderingStats(stats);
> }
> 
> and compositing mode isn't active, the struct will just have garbage stack values sitting in it.

Done.

>> Source/WebKit/chromium/public/WebRenderingStats.h:32
>> +    int frameNumber; // Number of frames that have elapsed on the main thread.
> 
> the names and documentation here are lacking. what does "elapsed" mean?  Is this the number of frames we've rendered? what is this counted from - the "beginning of time", the last call to gather stats, some other time? it looks like this is only implemented in the compositing path - please document this at least. is this a count, or a number?

It's since the beginning of the CCLayerTreeHost. Is that the same as the "beginning of time?" The beginning of the GPU process?

What's the difference between "count" and "number?"

>> Source/WebKit/chromium/public/WebRenderingStats.h:33
>> +    int implFrameNumber; // Number of frames that have elapsed on the impl thread.
> 
> the "impl" bit here doesn't mean much out of context (and the comment doesn't help). perhaps this could be named something like compositorThreadFrameNumber (or frameCount)
> 
> what happens if threaded mode isn't enabled? please document the behavior whatever it is

Isn't the "impl thread" just whatever thread compositing is happening on?

I'm not yet familiar with what terminology is specific and what is vague. If a "frame" is a delimiter between time intervals or an instantaneous event, then the "elapsed frames" would be the number of events that have occurred. In this case, the event for "frameNumber" is the number of commits that have happened (number of calls to CCLayerTreeHost::finishCommitOnImplThread) and the event for "implFrameNumber" is the number of composites that have happened (number of calls to CCLayerTreeHostImpl::drawLayers). Do these terms make sense in this context, or are they only applicable within the compositor code and should be invisible at this layer?

>>> Source/WebKit/chromium/public/WebWidget.h:227
>>> +    virtual void renderingStats(WebRenderingStats&) { }
>> 
>> this doesn't make sense on WebWidget - we only support compositing on WebViews.
>> 
>> renderingStats() is a bit generic for what this is - since it's only for compositing mode, something compositor-specific in the name would be better.
>> 
>> Looking at the implementation, it looks like this call is relatively heavy - it blocks the main thread until the compositor thread can respond. Someone looking at this header in isolation might think it's perfectly fine to do something like call this every frame just in case. I think we should document how expensive this call is or make it cheaper.
> 
> I think this makes sense on webwidget, actually. Fullscreen pepper plugins are webwidgets, remember? :/ And, we have teh composite() method on WebWidget.
> 
> obtainRenderingStats() with a comment saying it is slightly costly is sufficient for now, I think. THis is the place to explain that the call only works in accelerated mode and that calling it  in software mode will assert false.

I intend to extend the method in the future with non-compositing stats. I'm not sure yet whether it will reuse the existing WebRenderingStats struct fields. Maybe a TODO is needed?

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1455
>> +{
> 
> You should ASSERT_NOT_REACHED in the case that this is called in non-accelerated mode.

Won't that just cause Chrome to crash? As written, this method just does nothing in non-accelerated mode. Since I'm adding a constructor to WebRenderingStats, the benchmarking extension can check the fields, and exclude them from Javascript if they haven't been set.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.