744241 - Make displayport heuristic constants configurable

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[JP Rosevear [:jpr]]

Make viewport-bias displayport heuristic constants configurable.

At minimum we should allow tweaking of the the threshold at which we extend the displayport in both directions and the factor which we extend it by when moving "fast" and the factor which we extend it by when moving "slow".

This will ease a/b testing.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Make constants configurable

This makes the constants in FixedMargin and VelocityBias configurable. I chose this approach rather than adding prefs for each individual constant because the number of constants will probably change and this is more flexible. I didn't bother doing it for the dynamic resolution strategy yet since that's got a lot more knobs.

Comment 2

[Chris Lord [:cwiiis]]

Comment on attachment 614053 [details] [diff] [review]
Make constants configurable

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

I don't think it's a good idea for all the different strategies to share the same pref if this is how we do it. Either the options should be a comma-separated key/value pairs (or something similar), or there should be a pref per strategy.

I don't like the idea that you might change the strategy and end up causing exceptions or really bad behaviour because you've set some parameters that mean different things, or behave differently on a different strategy. I'm willing to be convinced otherwise though, but at least initially, r-'ing.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Lord [:cwiiis] from comment #2)
> I don't like the idea that you might change the strategy and end up causing
> exceptions or really bad behaviour because you've set some parameters that
> mean different things, or behave differently on a different strategy.

Yeah, that's a good point. I'll redo the patch.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Make constants configurable (v2)

Split out the options into separate prefs for each strategy.

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 614144 [details] [diff] [review]
Make constants configurable (v2)

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

r+ with the last comment addressed.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +42,5 @@
> +            StringTokenizer st = new StringTokenizer(strategyOptions, ",");
> +            options = new float[st.countTokens()];
> +            for (int i = 0; i < options.length; i++) {
> +                try {
> +                    options[i] = Float.parseFloat(st.nextToken());

I'd rather have it as a free-form string that gets decoded by the strategy itself, rather than assuming that it's a comma-separated list of floats, but that doesn't block this landing and perhaps I'm being too pedantic...

@@ +54,4 @@
>          switch (strategy) {
>              case 0:
>              default:
> +                sStrategy = new FixedMarginStrategy(options);

Might be nice to have an enum so that we can refer to the strategies by name, rather than using numbers.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +277,4 @@
>                      }
>                  }
> +                if (strategy != -1) {
> +                    DisplayPortCalculator.setStrategy(strategy, options[strategy]);

strategy should be bounds checked here, or setting a strategy >= NUM_STRATEGIES will end up with an ArrayIndexOutOfBoundsException (I think?)

Comment 6

[JP Rosevear [:jpr]]

Is this manageable for mere mortals to change?  Are individual prefs not easier to change by humans?

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 614144 [details] [diff] [review]
Make constants configurable (v2)

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

::: mobile/android/app/mobile.js
@@ +365,5 @@
>  #ifdef ANDROID
>  // 0=fixed margin, 1=velocity bias, 2=dynamic resolution, 3=no margins
>  pref("gfx.displayport.strategy", 1);
> +// options for the various displayport strategies
> +pref("gfx.displayport.options.0", "1.5,0.1,0.2");

I think I'd rather have these set as individual int prefs rather than strings that need to be parsed. So:

pref("gfx.displayport.fixed_strategey.multiplier", 15);
pref("gfx.displayport.fixed_strategey.xdanger", 1);
pref("gfx.displayport.fixed_strategey.ydanger", 2);

@@ +368,5 @@
> +// options for the various displayport strategies
> +pref("gfx.displayport.options.0", "1.5,0.1,0.2");
> +pref("gfx.displayport.options.1", "1.5,32,0.2");
> +pref("gfx.displayport.options.2", "");
> +pref("gfx.displayport.options.3", "");

since dynamic and no margins don't have options, don't have empty prefs

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Make constants configurable (v3)

Now with more 50% more prefs!

Comment 9

[Chris Lord [:cwiiis]]

Comment on attachment 614387 [details] [diff] [review]
Make constants configurable (v3)

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

r+, but a few comments. I don't think any of the quibbles are things that should stop it landing.

::: mobile/android/app/mobile.js
@@ +365,3 @@
>  #ifdef ANDROID
>  // 0=fixed margin, 1=velocity bias, 2=dynamic resolution, 3=no margins
>  pref("gfx.displayport.strategy", 1);

If we're using two-letter prefixes below, dare I suggest that the same prefix be used here? That way the strategy prefs will be a bit more obvious when looking at them in about:config.

@@ +370,5 @@
> +// stated otherwise, the value will be multiplied by the size of the viewport.
> +// fixed margin strategy options
> +pref("gfx.displayport.strategy_fm.multiplier", 1500); // displayport dimension multiplier
> +pref("gfx.displayport.strategy_fm.danger_x", 100); // danger zone on x-axis
> +pref("gfx.displayport.strategy_fm.danger_y", 200); // danger zone on y-axis

You mention what the units are further down for threshold and buffer, but not for these two.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +70,5 @@
> +    }
> +
> +    private static float getFloatPref(Map<String, Integer> prefs, String prefName, int defaultValue) {
> +        Integer value = (prefs == null ? null : prefs.get(prefName));
> +        return (float)(value == null ? defaultValue : value) / 1000f;

What was wrong with float prefs?

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +127,5 @@
> +        prefs.put(DisplayPortCalculator.PREF_DISPLAYPORT_FM_DANGER_X);
> +        prefs.put(DisplayPortCalculator.PREF_DISPLAYPORT_FM_DANGER_Y);
> +        prefs.put(DisplayPortCalculator.PREF_DISPLAYPORT_VB_MULTIPLIER);
> +        prefs.put(DisplayPortCalculator.PREF_DISPLAYPORT_VB_VELOCITY_THRESHOLD);
> +        prefs.put(DisplayPortCalculator.PREF_DISPLAYPORT_VB_REVERSE_BUFFER);

I think it'd be nicer to have a function in DisplayPortCalculator that takes a JSONArray and dumps all its prefs in it, rather than this way round - at least then, when adding a pref, you only need to keep one file in sync.

@@ +267,5 @@
>                      JSONObject pref = jsonPrefs.getJSONObject(i);
> +                    prefValues.put(pref.getString("name"), pref.getInt("value"));
> +                }
> +                if (DisplayPortCalculator.setStrategy(prefValues)) {
> +                    GeckoAppShell.unregisterGeckoEventListener("Preferences:Data", this);

This method of prefs retrieval seems delicate/inefficient, but I guess that's another bug :(

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Make constants configurable (v4)

Updating with latest patch, carrying r+

(In reply to Chris Lord [:cwiiis] from comment #9)
> If we're using two-letter prefixes below, dare I suggest that the same
> prefix be used here? That way the strategy prefs will be a bit more obvious
> when looking at them in about:config.

I left this as-is, changing it is a little messy right now.

> You mention what the units are further down for threshold and buffer, but
> not for these two.

Fixed

> What was wrong with float prefs?

As discussed on IRC, prefs can only be int/string/bool. Left these as ints rather doing string/Float.parseFloat.

> I think it'd be nicer to have a function in DisplayPortCalculator that takes
> a JSONArray and dumps all its prefs in it, rather than this way round - at
> least then, when adding a pref, you only need to keep one file in sync.

Fixed

Comment 11

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17e8b7678990

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/17e8b7678990