Closed Bug 719570 Opened 10 years ago Closed 10 years ago

Need option to set checkboard to be flat background color for Eideticker

Categories

(Firefox for Android Graveyard :: General, defect, P3)

x86_64
Linux
defect

Tracking

(fennec11+)

RESOLVED FIXED
Firefox 13
Tracking Status
fennec 11+ ---

People

(Reporter: wlach, Assigned: wlach)

References

Details

Attachments

(1 file, 2 obsolete files)

The way things work now, I need to use a complicated heuristic for actually detecting checkboarding (see: https://github.com/mozilla/eideticker/blob/master/src/videocapture/videocapture/checkerboard.py). Unfortunately, after some testing, I've found this actually doesn't work that well -- it misdetects certain patterns as being checkerboards. I could certainly try to improve the heuristic, but I'm not sure if it would be that easy or worthwhile. 

I think an easier route would be to modify Native Fennec to be able to use an arbitrary color for the checkerboard, dictated by a pref. Bug 710533 already contains most of the code needed for this. Once it lands, it should be a simple matter to modify the code to just return an image filled with the right arbitrary colour instead of a checkerboard.
Actually it would probably be better to just use the page's background color, as that's what XUL Fennec and the Android default browser (at least in 2.2) use.
Summary: Need option to set checkboard background to an arbitrary color for Eideticker → Need option to set checkboard to be flat background color for Eideticker
Depends on: 710533
Here's a simple patch to add the option (disabled by default) to use a flat pattern instead of a checkerboard. It's a profile option, not an Android option, because it would be desirable to set it programmatically on a blank eideticker profile before beginning a test.

Note this patch is based on top of the currently proposed patch for bug 710533 (which adds a tint to the checkerboard)
Assignee: nobody → wlachance
Attachment #590810 - Flags: review?(pwalton)
tracking-fennec: --- → 11+
Priority: -- → P3
Comment on attachment 590810 [details] [diff] [review]
Add option to set checkboard to be page background color

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

::: mobile/android/base/gfx/CheckerboardImage.java
@@ +88,5 @@
> +            Arrays.fill(fillBuffer, color16);
> +
> +            for (int i = 0; i < SIZE; i++) {
> +                shortBuffer.put(fillBuffer);
> +            }

nit: return; here to avoid indentation

::: mobile/android/chrome/content/browser.js
@@ +814,4 @@
>        let computedStyle = contentWindow.getComputedStyle(contentDocument.body);
>        viewport.backgroundColor = computedStyle.backgroundColor;
>      }
> +    viewport.showCheckerboardChecks = Services.prefs.getBoolPref("gfx.show_checkerboard_pattern");

getDrawMetadata() is called all the time, so I'm worried about the performance of calling out to prefs here. Especially since users will almost never modify this, but all users will pay a performance regression for the feature.

I'd prefer to make this a separate event that calls out to Gecko and sets a flag in the GeckoSoftwareLayerClient.
Attachment #590810 - Flags: review?(pwalton) → review-
Update to hopefully address issues. As discussed on irc, I made it so that we read the pref once on startup instead of listening to it change, as that avoids some extra complexity/overhead which isn't worth it for a testing-only feature.
Attachment #590810 - Attachment is obsolete: true
Attachment #591954 - Flags: review?(pwalton)
Comment on attachment 591954 [details] [diff] [review]
Add option to set checkboard to be page background color (take 2)

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

Looks good, thanks!

::: mobile/android/base/gfx/LayerController.java
@@ +90,5 @@
>      private LayerClient mLayerClient;               /* The layer client. */
>  
>      /* The new color for the checkerboard. */
>      private int mCheckerboardColor;
> +    private boolean mCheckerboardShowChecks;

nit: The name mCheckerboardShouldShowChecks seems a little clearer to me.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +354,5 @@
>      }
>  
>      private void updateCheckerboardLayer(GL10 gl, RenderContext renderContext) {
> +        int checkerboardColor = mView.getController().getCheckerboardColor();
> +        boolean showChecks = mView.getController().getCheckerboardShowChecks();

nit: I'd prefer the method name checkerboardShouldShowChecks(), it seems clearer.
Attachment #591954 - Flags: review?(pwalton) → review+
Try run for 565c1a2b49fd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=565c1a2b49fd
Results (out of 52 total builds):
    exception: 2
    success: 45
    warnings: 4
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.com-565c1a2b49fd
This addresses the nits in the review and updates for bitrot. Carrying over r+
Attachment #591954 - Attachment is obsolete: true
Attachment #593236 - Flags: review+
(In reply to Mozilla RelEng Bot from comment #6)
> Try run for 565c1a2b49fd is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=565c1a2b49fd
> Results (out of 52 total builds):
>     exception: 2
>     success: 45
>     warnings: 4
>     failure: 1
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.
> com-565c1a2b49fd

Looks ok except for some timeouts on the crashtest, which seem to be occurring with other builds on try.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6717b2b6a09c
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.