Need viewport prediction to reducing unnecessary painting

RESOLVED FIXED in Firefox 14

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 14
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 affected, firefox14 fixed, blocking-fennec1.0 beta+)

Details

(Whiteboard: maple [gfx])

Attachments

(9 attachments, 21 obsolete attachments)

3.10 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
13.99 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
1.97 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
18.04 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
3.01 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
32.29 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
2.25 KB, patch
kats
: review+
Details | Diff | Splinter Review
5.65 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
1.40 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
For cases when paint/compositing takes longer than 15ms (~60fps) we could end up doing useless paints during rapid flinging/zooming. i.e. a paint happens but by the time we have the data from that the viewport has changed enough that it is useless. So, we need some sort of viewport prediction code that allows us to know where the viewport will be at the end of the paint and request that to be painted instead.
Created attachment 599623 [details] [diff] [review]
Add logging to see how much prediction would help
Attachment #599623 - Flags: review?(chrislord.net)
Whiteboard: maple

Comment 5

7 years ago
Comment on attachment 599623 [details] [diff] [review]
Add logging to see how much prediction would help

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

This logging assumes that we could implement perfect prediction, but that's just a case of wording and I'm nitpicking. Looks fine.
Attachment #599623 - Flags: review?(chrislord.net) → review+
You mean perfect prediction is impossible?!? Oh noes! :p

Landed as https://hg.mozilla.org/projects/maple/rev/ba6b3a24d5c6

I ran with this patch on timecube on the Galaxy Nexus and flung the page as hard as I could a few times, just to get an idea of the worst-case scenario. Tallying up the percentages of wasted paints gave me this:

Samples: 68
Average: 8.10294
Stddev: 13.1755
Max: 100
Min: 0

If I pan slowly (as though I were reading) the wasted pixels generally remain less than 1%. At the start of a pan there's usually spike that's in the 3-10% range.
Keywords: fennecnative-betablocker
How is this bug related to bug 718388? Should the bugs be duped?
Duplicate of this bug: 718388
Duped that to here.

Note to future self: bug 718388 has Cwiiis' previous work on viewport prediction along with a patch that at some point in time used to apply somewhere.
blocking-fennec1.0: --- → beta+
Created attachment 600940 [details] [diff] [review]
(1/4) Extract AnimationRunnable to a new file
Attachment #599609 - Attachment is obsolete: true
Attachment #600940 - Flags: review?(chrislord.net)
Created attachment 600941 [details] [diff] [review]
(2/4) Add a getViewportAt to predict animation viewports
Attachment #599610 - Attachment is obsolete: true
Attachment #600941 - Flags: review?(chrislord.net)
Created attachment 600943 [details] [diff] [review]
(3/4) Add a ViewportPredictor class
Attachment #599611 - Attachment is obsolete: true
Attachment #600943 - Flags: review?(chrislord.net)
Created attachment 600945 [details] [diff] [review]
(4/4) Hook up the viewport prediction

This one still needs work. I think we'll need to record the round trip time between adjustViewport() and the corresponding getViewTransform() and use that to calculate the average paint-composite time. Then we can predict the viewport ahead by that much time. Thoughts?
Attachment #600945 - Flags: feedback?(chrislord.net)
Comment on attachment 600940 [details] [diff] [review]
(1/4) Extract AnimationRunnable to a new file

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

As well as the comment below, I think it'd be nice to remove the timer creation/deletion and abstract that in AnimationRunnable too - that would make it a complete class usable outside of PanZoomController without much code duplication.

r+ either way, this can be done later.

::: mobile/android/base/ui/PanZoomController.java
@@ +548,5 @@
>              }
>  
>              /* Perform the next frame of the bounce-back animation. */
> +            int frameIndex = getCurrentFrameIndex();
> +            if (frameIndex < EASE_OUT_ANIMATION_FRAMES.length) {

I think it'd be nice to have EASE_OUT_ANIMATION_FRAMES in Animation Runnable too, given that the size of that array depends on a static variable in that class (and you may want to use that elsewhere).
Attachment #600940 - Flags: review?(chrislord.net) → review+
Comment on attachment 600941 [details] [diff] [review]
(2/4) Add a getViewportAt to predict animation viewports

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

This is quite an expensive way of doing this, It'd be nicer if we figured out the maths to work this out for any given frame with a start and end point... That said, I tried to do that when I was writing the animation task patches, and it's a bit tricky, so that shouldn't block this.

I'm really not keen on having getViewportAt in AnimationRunnable though... Can we just make it a public method of the AnimationRunnable in PanZoomController?

::: mobile/android/base/ui/AnimationRunnable.java
@@ +39,5 @@
>      protected final int getCurrentFrameIndex() {
>          return mCurrentFrameIndex;
>      }
>  
> +    protected abstract ViewportMetrics getViewportAt(int frameIndex);

This really doesn't feel like the right place for this function. Viewport metrics don't have much to do with animations (I don't see this being implemented by anything except PanZoomController).
Attachment #600941 - Flags: review?(chrislord.net) → review-
Comment on attachment 600943 [details] [diff] [review]
(3/4) Add a ViewportPredictor class

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

r+ with the synchronized block below removed, or an explanation of what it provides.

::: mobile/android/base/ui/ViewportPredictor.java
@@ +23,5 @@
> +    ViewportMetrics predictViewportIn(long millis) {
> +        // stash a local copy of the animation runnable so we don't need to
> +        // synchronize the entire method and possibly block calls to setAnimation
> +        AnimationRunnable animation = null;
> +        synchronized (this) {

There's no point in this synchronized block is there? I don't see the difference between just straight assigning it above and synchronising here, other than the overhead of some locking.
Attachment #600943 - Flags: review?(chrislord.net) → review+
Comment on attachment 600945 [details] [diff] [review]
(4/4) Hook up the viewport prediction

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

I think it would be nice to have some constantly-adjusted prediction time (just a running average over the last 10 renders or something, multiplied by a fudging constant so that we don't over-predict too often?), but 20ms is probably a reasonable constant to begin with.

What I'm not sure about is using the margins rather than just setting the viewport for this prediction though... Is there a reason you did it like that?

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +342,5 @@
> +                        + prediction.left + " " + prediction.top + " " + prediction.width() + " " + prediction.height());
> +        mDisplayPortMargins.set(DEFAULT_DISPLAY_PORT_MARGIN + viewport.left - prediction.left,
> +                                DEFAULT_DISPLAY_PORT_MARGIN + viewport.top - prediction.top,
> +                                DEFAULT_DISPLAY_PORT_MARGIN + prediction.right - viewport.right,
> +                                DEFAULT_DISPLAY_PORT_MARGIN + prediction.bottom - viewport.bottom);

Is it right to use margins to do this? Shouldn't we just set the viewport to the predicted viewport?

::: mobile/android/base/gfx/LayerController.java
@@ +139,5 @@
>      public Context getContext()                   { return mContext; }
>      public ViewportMetrics getViewportMetrics()   { return mViewportMetrics; }
>  
> +    public ViewportMetrics getPredictedMetricsIn(long millis) {
> +        return mPanZoomController.predictViewportIn(15);

s/15/millis?
Attachment #600945 - Flags: feedback?(chrislord.net) → feedback+

Comment 18

6 years ago
This is r+ and f+ is there a dependency preventing us from landing it right now?
There's an r- on patch 2. But also it depends on bug 732564. I have an idea of how to do this better that should be quick to implement once that bug lands.
Depends on: 732564
Priority: -- → P1
Whiteboard: maple → maple [gfx]
Created attachment 606653 [details] [diff] [review]
(1/5) Add a DisplayPortMetrics class with its own resolution
Attachment #600940 - Attachment is obsolete: true
Attachment #600941 - Attachment is obsolete: true
Attachment #600943 - Attachment is obsolete: true
Attachment #600945 - Attachment is obsolete: true
Attachment #606653 - Flags: feedback?(chrislord.net)
Created attachment 606654 [details] [diff] [review]
(2/5) Draw using the displayport resolution
Created attachment 606656 [details] [diff] [review]
(3/5) Move display port calculations into DisplayPortCalculator and use velocity to improve display port
Created attachment 606658 [details] [diff] [review]
(4/5) Make aboutToCheckerboard() more aggressive while panning
Created attachment 606659 [details] [diff] [review]
(5/5) Delete some things
Posted a build with the current patches at https://people.mozilla.com/~kgupta/tmp/displayport.apk if anybody wants to try it. Still needs some tweaking.
Comment on attachment 606653 [details] [diff] [review]
(1/5) Add a DisplayPortMetrics class with its own resolution

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

Looks fine.

::: mobile/android/base/gfx/DisplayPortMetrics.java
@@ +34,5 @@
> +        mResolution = resolution;
> +    }
> +
> +    public boolean contains(RectF rect) {
> +        return new RectF(mLeft, mTop, mRight, mBottom).contains(rect);

Might be good to just create this on initialisation, rather than keep recreating it? Maybe just use it as the primary storage, forget mLeft/Top/etc.?
Attachment #606653 - Flags: feedback?(chrislord.net) → feedback+
Comment on attachment 606654 [details] [diff] [review]
(2/5) Draw using the displayport resolution

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

Yup, looks fine.
Attachment #606654 - Flags: feedback+
Comment on attachment 606656 [details] [diff] [review]
(3/5) Move display port calculations into DisplayPortCalculator and use velocity to improve display port

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

f- because of the resizing - I'd like to be wrong, but assuming I'm not, it'd be better to just have displayport size be a constant multiplier of the window size.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +17,5 @@
> +    private static final float VELOCITY_MULTIPLIER = 60.0f;
> +
> +    private static final float VELOCITY_FAST_THRESHOLD = 10.0f;
> +    private static final float FAST_SPLIT_FACTOR = 0.9f;
> +    private static final float SLOW_SPLIT_FACTOR = 0.7f;

These constants need commenting.

@@ +32,5 @@
> +                                            Math.abs(velocity.y) / height);
> +            velocityFactor *= VELOCITY_MULTIPLIER;
> +
> +            width += (width * velocityFactor);
> +            height += (height * velocityFactor);

While I like this in principal, changing the size of the display port during panning is a bad idea as it'll cause texture recreation and full invalidations. The texure thrashing can be fixed by getting TiledTextureImage to re-use tiles when resizing, but I'm not sure about the invalidations (maybe I'm wrong about that? This would need to be verified though).
Attachment #606656 - Flags: feedback-
Comment on attachment 606656 [details] [diff] [review]
(3/5) Move display port calculations into DisplayPortCalculator and use velocity to improve display port

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

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +99,5 @@
> +        // compositor-scaling and blurriness. Once we stop panning, the blurriness must be entirely gone.
> +        // Note that usable* could be less than base* if we are pinch-zoomed out into overscroll. In this
> +        // case the display resolution could be higher than zoomFactor.
> +        float displayResolution = metrics.zoomFactor * Math.min(baseWidth / usableWidth,
> +                                                                baseHeight / usableHeight);

Changing resolution will also cause the entire area to be re-rendered, but given that this ought to only happen when we're in danger of checker-boarding and when there's a large displayport area, I'd hope that this doesn't result in too much wasted rendering.
Comment on attachment 606658 [details] [diff] [review]
(4/5) Make aboutToCheckerboard() more aggressive while panning

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

Looks good.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +21,5 @@
>      private static final float FAST_SPLIT_FACTOR = 0.9f;
>      private static final float SLOW_SPLIT_FACTOR = 0.7f;
>  
> +    private static final float PREDICTION_VELOCITY_MULTIPLIER = 30.0f;
> +    private static final float MIN_DANGER_ZONE_MULTIPLIER = 0.10f; // this must be less than (SIZE_MULTIPLIER - 1.0f) / 2

More comments necessary.
Attachment #606658 - Flags: feedback+
Comment on attachment 606659 [details] [diff] [review]
(5/5) Delete some things

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

I'm all for code removal :)
Attachment #606659 - Flags: feedback+
(In reply to Chris Lord [:cwiiis] from comment #28)
> @@ +32,5 @@
> > +                                            Math.abs(velocity.y) / height);
> > +            velocityFactor *= VELOCITY_MULTIPLIER;
> > +
> > +            width += (width * velocityFactor);
> > +            height += (height * velocityFactor);
> 
> While I like this in principal, changing the size of the display port during
> panning is a bad idea as it'll cause texture recreation and full
> invalidations. The texure thrashing can be fixed by getting
> TiledTextureImage to re-use tiles when resizing, but I'm not sure about the
> invalidations (maybe I'm wrong about that? This would need to be verified
> though).

We're not actually changing the size of the display port though, since we adjust the display resolution to counteract the size changes in the display port. The only time the display port size should *actually* change is when our expanded display port exceeds one of the page dimensions, because then we need to crop it and that kills the aspect ratio. If the aspect ratio stays the same then we can adjust the display resolution to make it fit.

(In reply to Chris Lord [:cwiiis] from comment #29)
> Changing resolution will also cause the entire area to be re-rendered, but
> given that this ought to only happen when we're in danger of
> checker-boarding and when there's a large displayport area, I'd hope that
> this doesn't result in too much wasted rendering.

Yes, this approach does cause a lot of re-rendering, but the improvement comes from the rendered content actually being displayed on-screen before we scroll past it completely. With the old code and panning quickly, we would re-render the entire screen anyway because by the time it was time for the next draw we'd be more than screenful away from the last draw AND we wouldn't get the draw in time to show it to the user. IMO having wasted rendering is much more preferable than checkerboarding.
I think it'd be sensible to do this in two runs - the resolution changing is nice, but I can imagine it easily resulting in bad behaviour. Once the displayport resizing is gone, I think it'd be better to just do prediction without resolution change and file a new bug to figure out how best to do the down-scaling after that lands.
(In reply to Kartikaya Gupta (:kats) from comment #32)
> (In reply to Chris Lord [:cwiiis] from comment #28)
> > @@ +32,5 @@
> > > +                                            Math.abs(velocity.y) / height);
> > > +            velocityFactor *= VELOCITY_MULTIPLIER;
> > > +
> > > +            width += (width * velocityFactor);
> > > +            height += (height * velocityFactor);
> > 
> > While I like this in principal, changing the size of the display port during
> > panning is a bad idea as it'll cause texture recreation and full
> > invalidations. The texure thrashing can be fixed by getting
> > TiledTextureImage to re-use tiles when resizing, but I'm not sure about the
> > invalidations (maybe I'm wrong about that? This would need to be verified
> > though).
> 
> We're not actually changing the size of the display port though, since we
> adjust the display resolution to counteract the size changes in the display
> port. The only time the display port size should *actually* change is when
> our expanded display port exceeds one of the page dimensions, because then
> we need to crop it and that kills the aspect ratio. If the aspect ratio
> stays the same then we can adjust the display resolution to make it fit.

Sorry, I missed this (Friday evening...) - in that case, very cool :)

> (In reply to Chris Lord [:cwiiis] from comment #29)
> > Changing resolution will also cause the entire area to be re-rendered, but
> > given that this ought to only happen when we're in danger of
> > checker-boarding and when there's a large displayport area, I'd hope that
> > this doesn't result in too much wasted rendering.
> 
> Yes, this approach does cause a lot of re-rendering, but the improvement
> comes from the rendered content actually being displayed on-screen before we
> scroll past it completely. With the old code and panning quickly, we would
> re-render the entire screen anyway because by the time it was time for the
> next draw we'd be more than screenful away from the last draw AND we
> wouldn't get the draw in time to show it to the user. IMO having wasted
> rendering is much more preferable than checkerboarding.

I'd like to see some testing that shows the gains of rendering at the lower resolution, but I'm broadly positive :) Assuming that everything works and is stable (and given there are no Gecko changes, I can't see why it wouldn't be), I wouldn't mind seeing this polished and landed so we can better see its impact. I think it'd be a good idea to have a pref-controlled on/off switch.
Also regarding the display port size and texture recreation - I'm a little worried that since we're setting the display port in floats rather than ints, even a slight rounding error will trigger texture recreation. I listed this in the Roundings and Conversions section on https://wiki.mozilla.org/Fennec/NativeUI/Viewport but I don't have a good solution to ensuring we don't accidentally trigger these rounding-error-based-recreations. (Main problem in just using ints is that the page size in Java is a float, and if we want to avoid having a one-pixel strip of unpainted area at the borders of the page we need to keep the display port in floats as well).
Comment on attachment 606656 [details] [diff] [review]
(3/5) Move display port calculations into DisplayPortCalculator and use velocity to improve display port

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

Sorry, I didn't read this well enough on Friday and I missed what this is doing. Changing from f- to f+, as I like the idea of it. Trying it on my Flyer though, I end up with much more checkerboarding than before and starting at low-resolution rendering for a significant amount of time. It also never seemed to restore the original resolution. Clearly needs refinement, but I like the idea (I tested on engadget.com).
Attachment #606656 - Flags: feedback- → feedback+
(In reply to Chris Lord [:cwiiis] from comment #36)
> Comment on attachment 606656 [details] [diff] [review]
> (3/5) Move display port calculations into DisplayPortCalculator and use
> velocity to improve display port
> 
> Review of attachment 606656 [details] [diff] [review]:
> -----------------------------------------------------------------
> It
> also never seemed to restore the original resolution.

This part of your comment worries me; I stared at the code for a long time and couldn't see how it could fail to restore the original resolution. Once a fling is done or aborted, we set the velocity back to zero on both axes (by calling stopFling()) and send an updated display port and viewport. This updated display port should have the normal resolution. I also wasn't able to reproduce the behaviour when I tried it on the GN. I'll borrow some other devices and see if I can reproduce i there, but I might need to post some builds with logging and have you run them to figure it out.
Depends on: 737434
Created attachment 608092 [details] [diff] [review]
(1/6) Add a DisplayPortMetrics class with its own resolution

Just adding updated WIPs and debug patches I have so that jrmuizel and anybody else can get at them more easily. We had some interesting findings with respect to how much the display port size affects draw time and how different display port adjusting strategies affect checkerboarding.
Attachment #606653 - Attachment is obsolete: true
Created attachment 608093 [details] [diff] [review]
(2/6) Draw using the displayport resolution
Attachment #606654 - Attachment is obsolete: true
Created attachment 608094 [details] [diff] [review]
(3/6) Move display port calculations into DisplayPortCalculator
Attachment #606656 - Attachment is obsolete: true
Created attachment 608095 [details] [diff] [review]
(4/6) Add the low-resolution display port strategy
Attachment #606658 - Attachment is obsolete: true
Created attachment 608097 [details] [diff] [review]
(5/6) Add the no-margins display port strategy
Created attachment 608098 [details] [diff] [review]
(6/6) Delete stuff
Attachment #606659 - Attachment is obsolete: true
Depends on: 738556
Created attachment 609211 [details] [diff] [review]
(1/7) Add a DisplayPortMetrics class with its own resolution
Attachment #608092 - Attachment is obsolete: true
Attachment #609211 - Flags: review?(chrislord.net)
Created attachment 609212 [details] [diff] [review]
(2/7) Draw using the displayport resolution
Attachment #608093 - Attachment is obsolete: true
Attachment #609212 - Flags: review?(chrislord.net)
Created attachment 609213 [details] [diff] [review]
(3/7) Move display port calculations into DisplayPortCalculator
Attachment #608094 - Attachment is obsolete: true
Attachment #609213 - Flags: review?(chrislord.net)
Created attachment 609214 [details] [diff] [review]
(4/7) Add the low-resolution display port strategy

On the Galaxy Tab 10.1, this gives a completeness score of ~95% on the talos checkerboarding test, as compared to ~53% with the current strategy. Not setting it as the default strategy yet because I want to run the test on other devices as well first.
Attachment #608095 - Attachment is obsolete: true
Attachment #609214 - Flags: review?(chrislord.net)
Created attachment 609215 [details] [diff] [review]
(5/7) Add the no-margins display port strategy

This has a completeness score of ~66% (better than our current strategy, not as good as the low-resolution drawing) on the Galaxy Tab.
Attachment #608097 - Attachment is obsolete: true
Attachment #609215 - Flags: review?(chrislord.net)
Created attachment 609217 [details] [diff] [review]
(6/7) Add the velocity-bias display port strategy
Attachment #609217 - Flags: review?(chrislord.net)
Created attachment 609218 [details] [diff] [review]
(7/7) Delete stuff
Attachment #608098 - Attachment is obsolete: true
Attachment #609218 - Flags: review?(chrislord.net)
(In reply to Kartikaya Gupta (:kats) from comment #49)
> Created attachment 609217 [details] [diff] [review]
> (6/7) Add the velocity-bias display port strategy

This has a completeness score of ~70% (second-best, after the ~95% completeness for low-res) on the Galaxy Tab.
Comment on attachment 609211 [details] [diff] [review]
(1/7) Add a DisplayPortMetrics class with its own resolution

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

Looks fine to me.
Attachment #609211 - Flags: review?(chrislord.net) → review+
Comment on attachment 609212 [details] [diff] [review]
(2/7) Draw using the displayport resolution

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

Looks fine.
Attachment #609212 - Flags: review?(chrislord.net) → review+
Comment on attachment 609213 [details] [diff] [review]
(3/7) Move display port calculations into DisplayPortCalculator

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

Looks good.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +10,5 @@
> +
> +final class DisplayPortCalculator {
> +    private static final String LOGTAG = "GeckoDisplayPortCalculator";
> +
> +    private static final int DEFAULT_DISPLAY_PORT_MARGIN = 300;

I think it's worth considering having different margins for width/height, like xul-fennec, and like we used to have in the Java compositor - Even when there is horizontal scrolling, I think for the overwhelming majority of cases, users will be scrolling vertically. That's nothing to do with this patch though, but something to consider later.
Attachment #609213 - Flags: review?(chrislord.net) → review+
Comment on attachment 609214 [details] [diff] [review]
(4/7) Add the low-resolution display port strategy

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

I'm not keen on having a static class with different code-paths done like this - Ideally, these different strategies would be different sub-classes of a base-class with the shared code. Perhaps it could be a static method on GeckoLayerClient, and the flag could be used on first call to create the right object to return?

It's going to get messy pretty quickly if we need to have calculate* and aboutToCheckerboard* for every strategy, with their support functions/variables littered about the place. r- because of this and the comments below, but r+ with these addressed.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +120,5 @@
>      }
> +
> +    // ------- These functions implement the variation where we draw more of the page at low resolution while panning ---------
> +
> +    private static final float SIZE_MULTIPLIER = 1.5f;

No comment for SIZE_MULTIPLIER.

@@ +123,5 @@
> +
> +    private static final float SIZE_MULTIPLIER = 1.5f;
> +
> +    // The velocity above which we start zooming out the display port to keep up
> +    // with the panning.

A dangerous thing here, the velocity is measured in pixels per frame, right? This is going to be bad for situations like the Galaxy Nexus (small, 720p screen) vs. the HTC Flyer (large, 1024x600 screen) vs. the HTC Wildfire (small 320x240 screen), etc.

It may well be the work of a future patch, but these values need to be relative to actual screen size, not pixels.

@@ +142,5 @@
> +    // amount of pan will decrease each time. If we take into account display port splitting,
> +    // it should be increased as the splitting means some of the display port will be used to
> +    // draw in the opposite direction of the velocity. For now I'm assuming these two cancel
> +    // each other out.
> +    private static final float VELOCITY_MULTIPLIER = 60.0f;

If this is measured in frames, as implied, isn't 60 a bit high?

@@ +260,5 @@
> +            displayResolution);
> +        return dpMetrics;
> +    }
> +
> +    private static float split(float amount, float velocity) {

I'm not sure 'split' is a descriptive enough function name, and although the contents are documented well, there should be a description of the whole function above here.

Maybe splitByVelocity? I'm not too keen on that name either, but it's a bit more descriptive.
Attachment #609214 - Flags: review?(chrislord.net) → review-
Comment on attachment 609215 [details] [diff] [review]
(5/7) Add the no-margins display port strategy

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

Looks fine, but this would need to be done differently taking my previous review into account.
Attachment #609215 - Flags: review?(chrislord.net) → review+
Comment on attachment 609217 [details] [diff] [review]
(6/7) Add the velocity-bias display port strategy

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

r- because of the last question, and the redistribute-and-clamp-margins code is a prime example of what should be shared, I think.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +164,5 @@
> +        float desiredXMargins = metrics.getWidth() * VELOCITY_BIAS_MULTIPLIER;
> +        float desiredYMargins = metrics.getHeight() * VELOCITY_BIAS_MULTIPLIER;
> +
> +        // but if we're panning on one axis, set the margins for the other axis to zero since we are likely
> +        // axis locked and won't be displaying that extra area.

That's a clever optimisation :)

@@ +180,5 @@
> +        float yBufferAmount = Math.min(desiredYMargins, metrics.pageSizeHeight - metrics.getHeight());
> +
> +        // if we're panning above the VELOCITY_BIAS_THRESHOLD on an axis, shift the margin so that it
> +        // is entirely in the direction of panning. Otherwise, split the margin evenly on both sides of
> +        // the display port.

I think biasing so that it's *entirely* in the direction of panning isn't a good idea - I think there ought to be a weight (say, 0.8), in the situation that you do a fast fling, then stop it and make a small adjustment (something I regularly do, at least).

@@ +214,5 @@
> +    }
> +
> +    // Returns true if a checkerboard is about to be visible.
> +    private static boolean aboutToCheckerboardVelocityBias(ImmutableViewportMetrics metrics, DisplayPortMetrics displayPort) {
> +        return true;

Should this just be returning true?
Attachment #609217 - Flags: review?(chrislord.net) → review-
Comment on attachment 609218 [details] [diff] [review]
(7/7) Delete stuff

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

Looks good to me.
Attachment #609218 - Flags: review?(chrislord.net) → review+
Created attachment 609345 [details] [diff] [review]
(4/7) Add the low-resolution display port strategy

Modified to use an interface/subclasses instead of a static switch. Initially I was planning to just keep one of the strategies which is why I went with the static switch, but I guess this makes more sense now. Also add a comment for SIZE_MULTIPLIER, incorporate GeckoAppShell.getDpi() into the calculations, and rename split().

> If this is measured in frames, as implied, isn't 60 a bit high?

It does seem high, yes, but it needs to be high because the draw time goes up as well. The more we draw, the longer it takes, and so the more we need to draw.
Attachment #609214 - Attachment is obsolete: true
Attachment #609345 - Flags: review?(chrislord.net)
Created attachment 609347 [details] [diff] [review]
(5/7) Add the no-margins display port strategy

Rebased into the interface/class implementation. Carrying r+
Attachment #609215 - Attachment is obsolete: true
Attachment #609347 - Flags: review+
Created attachment 609349 [details] [diff] [review]
(6/7) Add the velocity-bias display port strategy

(In reply to Chris Lord [:cwiiis] from comment #57)
> > +        // but if we're panning on one axis, set the margins for the other axis to zero since we are likely
> > +        // axis locked and won't be displaying that extra area.
> 
> That's a clever optimisation :)
> 

Credit should go to jrmuizel for that one :)

> I think biasing so that it's *entirely* in the direction of panning isn't a
> good idea - I think there ought to be a weight (say, 0.8), in the situation
> that you do a fast fling, then stop it and make a small adjustment
> (something I regularly do, at least).

So I tried this locally - the testCheck completeness score dropped slightly on the Galaxy Nexus when I did this (from ~95% to ~93%). I think these results make sense because this strategy has a very small margin and we're mostly scrolling in one direction (as opposed to reversing direction on every scroll). This means if we use any of the margin to draw stuff in the reverse direction it negatively impacts checkerboarding in the forward direction by a significant amount for most of the scrolls. We could increase the margin to compensate but then draw times go up too (and by a surprising amount just based on ad-hoc testing). That is effectively what is happening in the low-res strategy with the split function.

> > +    private static boolean aboutToCheckerboardVelocityBias(ImmutableViewportMetrics metrics, DisplayPortMetrics displayPort) {
> > +        return true;
> 
> Should this just be returning true?

I thought it would be better because we have such a small margin, we want to be drawing more aggressively. At the start of a pan the velocity is going to be large so we're almost certainly going to go into checkerboard on every frame, so drawing all the time seems like the right thing. At the end of the pan we want to re-center the displayport and draw stuff on all sides, so again we don't want to throttle there. When we're not panning we're not drawing anyway so it doesn't make a difference there.
Attachment #609217 - Attachment is obsolete: true
Attachment #609349 - Flags: review?(chrislord.net)
Comment on attachment 609345 [details] [diff] [review]
(4/7) Add the low-resolution display port strategy

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

Better - still not entirely keen on such a monolithic file, but it's well-separated now. Good stuff. r+ regardless of comments.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +38,5 @@
> +     * and/or (b) increasing the buffer on the other axis to compensate for the reduced buffer on
> +     * one axis.
> +     */
> +    private static class FixedMarginStrategy implements DisplayPortStrategy {
> +        private static final int DEFAULT_DISPLAY_PORT_MARGIN = 300;

Maybe this should also now be relative to the screen-size? I'll mention that 300 seemed to work quite well at a resolution of 1024x600, and I assume works well at 800x480, so maybe take one of those resolutions as a guideline for what multiplier to use?

@@ +134,5 @@
> +     * scales this up to the viewport zoom level. This results in a large area of the page drawn but it
> +     * looks blurry. The assumption is that drawing extra that we never display is better than checkerboarding,
> +     * where we draw less but never even show it on the screen.
> +     */
> +    private static class LowResolutionStrategy implements DisplayPortStrategy {

Something I didn't catch in the earlier review is that I think 'DynamicResolutionStrategy' would be a better name for this.
Attachment #609345 - Flags: review?(chrislord.net) → review+
Comment on attachment 609349 [details] [diff] [review]
(6/7) Add the velocity-bias display port strategy

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

Looks good to me.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +212,5 @@
> +                    metrics.zoomFactor);
> +        }
> +
> +        public boolean aboutToCheckerboard(ImmutableViewportMetrics metrics, PointF velocity, DisplayPortMetrics displayPort) {
> +            return true;

Perhaps you could add what you said in comment #61 here?
Attachment #609349 - Flags: review?(chrislord.net) → review+
Landed on inbound with s/LowResolution/DynamicResolution/ and the comment for the velocity-bias aboutToCheckerboard implementation. I left the 300 pixel margin as-is for now and will fix it (and continue working on these strategies) as part of bug 737510 and/or bug 737585 and/or one of the many other checkerboarding bugs.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e5eee67741f
https://hg.mozilla.org/integration/mozilla-inbound/rev/56aab2bfad06
https://hg.mozilla.org/integration/mozilla-inbound/rev/c48996e29c32
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff25df0d0a27
https://hg.mozilla.org/integration/mozilla-inbound/rev/7db6f151bded
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac5aed81d6e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/c69c54b0bb11
status-firefox13: --- → affected
status-firefox14: --- → fixed
Target Milestone: --- → Firefox 14
Created attachment 609477 [details] [diff] [review]
(8/7) Fix error in calculation in part 2

Found this error while debugging my patch for bug 737577.
Attachment #609477 - Flags: review?(chrislord.net)
Comment on attachment 609477 [details] [diff] [review]
(8/7) Fix error in calculation in part 2

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

r+ with comment addressed.

::: mobile/android/chrome/content/browser.js
@@ +1625,2 @@
>                                   (aDisplayPort.right - aDisplayPort.left) / resolution,
>                                   (aDisplayPort.bottom - aDisplayPort.top) / resolution,

I'm not sure I understand this anymore, could you add a comment above this block explaining what zoom/resolution represent and how they're being used here?
Attachment #609477 - Flags: review?(chrislord.net) → review+
Landed follow-up on inbound; added a bunch of comments to explain what's going on.

https://hg.mozilla.org/integration/mozilla-inbound/rev/39e2091a2e20
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 737553
https://hg.mozilla.org/mozilla-central/rev/39e2091a2e20
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 737585
(In reply to Kartikaya Gupta (:kats) from comment #64)
> Landed on inbound with s/LowResolution/DynamicResolution/ and the comment
> for the velocity-bias aboutToCheckerboard implementation. I left the 300
> pixel margin as-is for now and will fix it (and continue working on these
> strategies) as part of bug 737510 and/or bug 737585 and/or one of the many
> other checkerboarding bugs.

For posterity, replacing the 300 pixel margin with a percentage of view size was done in bug 737553.
Depends on: 741988
You need to log in before you can comment on or make changes to this bug.