Last Comment Bug 747528 - Dynamically use draw time measurement to do better viewport prediction
: Dynamically use draw time measurement to do better viewport prediction
Status: RESOLVED FIXED
[inbound]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: Firefox 15
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
Depends on:
Blocks: checkerboarding
  Show dependency treegraph
 
Reported: 2012-04-20 13:51 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-04-26 13:28 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
beta+


Attachments
WIP (1/2) Add PredictionBiasStrategy (6.15 KB, patch)
2012-04-23 19:05 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
WIP (2/2) Hook up the draw timing (17.67 KB, patch)
2012-04-23 19:07 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
CheckViz logging (5.04 KB, patch)
2012-04-23 19:08 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
WIP (3/2) DrawTimingQueue to the rescue (8.78 KB, patch)
2012-04-23 21:46 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
CheckViz logging (4.61 KB, patch)
2012-04-23 21:47 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
(1/2) Add PredictionBiasStrategy (11.44 KB, patch)
2012-04-24 13:15 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Review
(2/2) Hook up the draw timing (22.58 KB, patch)
2012-04-24 13:17 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Review
CheckViz logging (4.61 KB, patch)
2012-04-24 13:19 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
Merged patch (v2) (30.95 KB, patch)
2012-04-25 06:51 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail.mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2012-04-20 13:51:42 PDT
This is a bug to track the work that Andreas started and I'm continuing, wherein we try to record the round-trip draw time at runtime, and then combine that with viewport prediction to try and reduce checkerboarding.

The current patch queue (rebased on top of the tiling and snap-to-tile work) is at https://github.com/staktrace/mozilla-central/commits/tiling_prediction. It's still very rough and will need to be cleaned up a lot before it is ready to land. In particular we should refactor it so that it is a normal strategy like the other strategies, instead of blowing away DisplayPortCalculator and effectively starting from scratch there.

Also the visualization tool being used to improve this is at https://github.com/staktrace/andreasgal-checkviz/commits
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-20 13:57:00 PDT
Also I pushed try builds with slightly older versions of these patches to get tcheckerboard[2] numbers. These are at https://tbpl.mozilla.org/?tree=Try&rev=0df854a00b6d (just the prediction stuff) and https://tbpl.mozilla.org/?tree=Try&rev=2605567556d4 (tiling + prediction). So far both builds show an improvement of 1-2% percent, more so on tcheckerboard2 than tcheckerboard.
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-23 19:05:09 PDT
Created attachment 617742 [details] [diff] [review]
WIP (1/2) Add PredictionBiasStrategy
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-23 19:07:13 PDT
Created attachment 617743 [details] [diff] [review]
WIP (2/2) Hook up the draw timing

I don't want to land this in its current state because the draw timing stuff adds synchronization and slowness on the compositor path (for all strategies). I have some ideas about how to eliminate this, which involve writing a new data structure. I will try that next and that will make me more comfortable with landing this preffed off.
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-23 19:08:14 PDT
Created attachment 617744 [details] [diff] [review]
CheckViz logging

This output is parsed by https://github.com/staktrace/andreasgal-checkviz/
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-23 21:46:35 PDT
Created attachment 617776 [details] [diff] [review]
WIP (3/2) DrawTimingQueue to the rescue

This is more what I had in mind. Compiled but completely untested.
Comment 6 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-23 21:47:01 PDT
Created attachment 617777 [details] [diff] [review]
CheckViz logging

Rebased
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-24 13:15:42 PDT
Created attachment 618009 [details] [diff] [review]
(1/2) Add PredictionBiasStrategy

This strategy is good enough to get no checkerboarding on the red page except in known bad conditions (starting a fling right before we expand to the fixed-margin displayport, and changing directions). It seems ok on CNN, but I only did a basic sanity test there.
Comment 8 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-24 13:17:07 PDT
Created attachment 618010 [details] [diff] [review]
(2/2) Hook up the draw timing

This patch tries to have a minimal impact on other strategies and the compositor code path, hence the DrawTimingQueue business.
Comment 9 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-24 13:19:01 PDT
Created attachment 618012 [details] [diff] [review]
CheckViz logging

Rebased checkviz logging. This shouldn't land so not requesting review, but putting it here for safekeeping.
Comment 10 Chris Lord [:cwiiis] 2012-04-24 16:03:58 PDT
Comment on attachment 618009 [details] [diff] [review]
(1/2) Add PredictionBiasStrategy

Review of attachment 618009 [details] [diff] [review]:
-----------------------------------------------------------------

I think most of my problems here are me not understanding how this works, but if that's the case, maybe the comments need amendment. r- for now, but an r+ with the below addressed.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +630,5 @@
> +     * This class implements the variation where we use the draw time to predict where we will be when
> +     * a draw completes, and draw that instead of where we are now. In this variation, when our panning
> +     * speed drops below a certain threshold, we draw 9 viewports' worth of content so that the user can
> +     * pan in any direction without encountering checkerboarding. Once they start panning, we shrink
> +     * the displayport down to the size of the viewport, but use the draw time and panning velocity to

This comment isn't quite right, is it? It looks like the code is meant to decide some kind of confidence range of how many frames it may take to update, then size the displayport to encompass both extremes?

@@ +638,5 @@
> +        private static float VELOCITY_THRESHOLD;
> +
> +        private int mPixelArea;         // area of the viewport, used in draw time calculations
> +        private int mMinFramesToDraw;   // minimum number of frames we take to draw
> +        private int mMaxFramesToDraw;   // maximum number of frames we take to draw

These are never changed, how does this work...?

@@ +664,5 @@
> +            float maxDx = velocity.x * mMaxFramesToDraw;
> +            float maxDy = velocity.y * mMaxFramesToDraw;
> +
> +            // if we draw that entire area range, how many pixels will we draw?
> +            float pixelsToDraw = (width + Math.abs(maxDx - minDx)) * (height + Math.abs(maxDy - minDy));

I'm really not sure what's going on here, I think these comments could explain a bit better.

@@ +671,5 @@
> +            maxDx = maxDx * pixelsToDraw / mPixelArea;
> +            maxDy = maxDy * pixelsToDraw / mPixelArea;
> +
> +            // and finally generate the displayport. the min/max stuff takes care of
> +            // negative velocities as well positive.

as well *as* positive

@@ +681,5 @@
> +            return getTileAlignedDisplayPortMetrics(margins, metrics.zoomFactor, metrics);
> +        }
> +
> +        public boolean aboutToCheckerboard(ImmutableViewportMetrics metrics, PointF velocity, DisplayPortMetrics displayPort) {
> +            // the code below is the same as in calculate()

So can't it be shared?
Comment 11 Chris Lord [:cwiiis] 2012-04-24 16:56:45 PDT
Comment on attachment 618010 [details] [diff] [review]
(2/2) Hook up the draw timing

Review of attachment 618010 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, the first patch makes a lot more sense now. r+ with comments addressed.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +716,5 @@
>          }
>  
>          @Override
> +        public boolean drawTimeUpdate(long millis, int pixels) {
> +            // normalize the time to be per-viewport-area, and in number of frames

I know what this does, but only from looking at the code - this comment just confuses me...

@@ +725,5 @@
> +            // be tweaked into more of a floating window average or something.
> +            if (normalizedFrames <= mMinFramesToDraw) {
> +                mMinFramesToDraw--;
> +            } else if (normalizedFrames > mMaxFramesToDraw) {
> +                mMaxFramesToDraw++;

It seems this heuristic would be very negatively affected if something starts happening in the background (mMaxFramesToDraw would likely be over-inflated)

::: mobile/android/base/gfx/DrawTimingQueue.java
@@ +7,5 @@
> +
> +import android.os.SystemClock;
> +
> +/**
> + * A custom-built data structure to assist with measuring draw times.

This is pretty clever :)

@@ +14,5 @@
> + * objects and associated timestamps. It provides only three operations, which
> + * is all we require for our purposes of measuring draw times. Note
> + * in particular that the class is designed so that even though it is
> + * accessed from multiple threads, it does not require synchronization;
> + * any concurrency errors that result from this handled gracefully.

from this *are* handled

@@ +17,5 @@
> + * accessed from multiple threads, it does not require synchronization;
> + * any concurrency errors that result from this handled gracefully.
> + *
> + * Assuming an unrolled buffer so that mTail is greater than mHead, the data
> + * stored in the buffer at entries [mHead, mTail) will never be modified, and

Opening/closing bracket styles don't match (maybe purposeful?)

@@ +19,5 @@
> + *
> + * Assuming an unrolled buffer so that mTail is greater than mHead, the data
> + * stored in the buffer at entries [mHead, mTail) will never be modified, and
> + * so are "safe" to read. If this reading is done on the same thread that
> + * owns mHead, then reading the range [mHead, mTail) is guaranteed to be safe

and again.

@@ +34,5 @@
> +    private int mTail;
> +
> +    DrawTimingQueue() {
> +        mMetrics = new DisplayPortMetrics[ BUFFER_SIZE ];
> +        mTimestamps = new long[ BUFFER_SIZE ];

Style, no spaces around BUFFER_SIZE.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +400,5 @@
>          mCurrentViewTransform.scale = mFrameMetrics.zoomFactor;
>  
>          mRootLayer.setPositionAndResolution(x, y, x + width, y + height, resolution);
>  
> +        if (layersUpdated && mRecordDrawTimes) {

Let's have some comments on what this is doing.
Comment 12 Chris Lord [:cwiiis] 2012-04-24 16:57:58 PDT
Comment on attachment 618009 [details] [diff] [review]
(1/2) Add PredictionBiasStrategy

Review of attachment 618009 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, after reading the second patch, this makes sense. That said, I think they should land in the reverse order to that which they're listed here, and I think it shouldn't land without sorting out the comment clarity.
Comment 13 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-25 06:51:53 PDT
Created attachment 618239 [details] [diff] [review]
Merged patch (v2)

I squashed the two patches into one since it seemed simpler than splitting them out and having each one potentially not make sense on its own. Also updated for most of the review comments; I just couldn't find an easy way to refactor the duplicated code in calculate() and aboutToCheckerboard() so I added a comment about that instead.

Carrying r+ to the updated patch.
Comment 14 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-25 06:54:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/be42a3881ada
Comment 15 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-25 06:55:53 PDT
(In reply to Chris Lord [:cwiiis] from comment #11)
> > + * Assuming an unrolled buffer so that mTail is greater than mHead, the data
> > + * stored in the buffer at entries [mHead, mTail) will never be modified, and
> 
> Opening/closing bracket styles don't match (maybe purposeful?)
> 

Also, yes, this was on purpose - I'm using the perhaps-unfamiliar mathematical notation of open/closed ranges. [mHead, mTail) means "from mHead to mTail, including mHead but not including mTail"
Comment 16 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-25 13:57:05 PDT
Comment on attachment 618239 [details] [diff] [review]
Merged patch (v2)

[Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: none by default; they won't have the option to turn on prediction bias
Testing completed (on m-c, etc.): not much at this point; local testing prior to landing and some basic sanity checks
Risk to taking this patch (and alternatives if risky): medium risk - adds a bunch of code that is preffed off by default, but ALSO adds some code on a fairly critical path of the compositor, and bugs there would be deadly (crash-inducing).
String changes made by this patch: none
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-25 20:38:12 PDT
https://hg.mozilla.org/mozilla-central/rev/be42a3881ada
Comment 18 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-26 13:28:42 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/5fdf583b6268

Note You need to log in before you can comment on or make changes to this bug.