741837 - Hook up OMTC for Gonk

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Cody Brocious [:Daeken]]

Attachment: Initial patch

Gonk should be able to utilize the OMTC functionality implemented for Mobile and Desktop Firefox.

Attached is an initial patch.  There are currently stability issues and no way to fallback to non-OMTC pathways, so this patch should be used purely for testing.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Cody, we can land this patch very quickly if we
 - respect the omtc pref like fennec does, which we need anyway
 - and obviously, fall back on same-thread compositor when !omtc

Getting code in asap means you have less bitrottage to eat, and allows other folks to test much more easily.

Comment 2

[Cody Brocious [:Daeken]]

Attachment: Patch for OMTC that respects preferences and sets OMTC enabled by default

This patch adds the preference check, but it still doesn't fall back to non-OMTC pathways when it's not available.  In addition, it still has the hard-coded screen size.

Comment 3

[Andreas Gal :gal]

Comment on attachment 612202 [details] [diff] [review]
Patch for OMTC that respects preferences and sets OMTC enabled by default

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

::: widget/gonk/nsWindow.cpp
@@ +89,5 @@
>          // fbs succeeded or failed.
>          gNativeWindow = new android::FramebufferNativeWindow();
> +        
> +        if (sUsingOMTC) {
> +            // XXX: We shouldn't need this as it's set by CreateForWindow

Whats the right fix for this?

@@ +149,5 @@
> +        static unsigned char bits2[1 * 1 * 3];
> +        nsRefPtr<gfxASurface> targetSurface;
> +
> +        if(sUsingOMTC)
> +            targetSurface = new gfxImageSurface(bits2, gfxIntSize(1, 1), 1 * 3,

What do we use this for? looks pretty hacky

Comment 4

[Cody Brocious [:Daeken]]

I'm working on a fix for the size right now.  We have to assign it to something initially as the window is sized based on this, but I have it resizing based on information from the compositor side now.  Should have that rolled into the patch shortly.

As for the other targetSurface, there needs to be some surface for BasicLayers to draw to, but it seems that I can create a gfxImageSurface without actually giving it a backing store, which is probably the better approach.  Going to test that out now.

Comment 5

[Cody Brocious [:Daeken]]

Attachment: Fixed previous issues

Size is no longer hard coded, target surface is created once and there's no provided backing store.

Comment 6

[Andreas Gal :gal]

Comment on attachment 612407 [details] [diff] [review]
Fixed previous issues

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

::: widget/gonk/nsWindow.cpp
@@ +104,5 @@
> +            if (!sGLContext) {
> +                LOG("Failed to create GL context for fb, trying /dev/graphics/fb0");
> +
> +                // We can't delete gNativeWindow.
> +

Extra space not needed here.

@@ +107,5 @@
> +                // We can't delete gNativeWindow.
> +
> +                nsIntSize screenSize;
> +                sFramebufferOpen = Framebuffer::Open(&screenSize);
> +                gScreenBounds = nsIntRect(nsIntPoint(0, 0), screenSize);

All the duplicated code here is a bit ugly and its all slightly different from above. We can fix later I gues.

@@ +154,5 @@
> +
> +        if(sUsingOMTC)
> +            targetSurface = sOMTCSurface;
> +        else
> +            targetSurface = Framebuffer::BackBuffer();

If you move the UsimgOMTC logic into FrameBuffer, you can probably have a simpler interface here

@@ +170,3 @@
>  
> +        if (!sUsingOMTC) {
> +            targetSurface->Flush();

here too

Comment 7

[Andreas Gal :gal]

Land it. That entire fire needs a little cleanup, but we can leave that for a later exercise. Lets turn this on.

Comment 8

[Cody Brocious [:Daeken]]

Attachment: Updated to mozilla-inbound

Comment 9

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5738f35ffb11

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Whoa, um

+pref("layers.offmainthreadcomposition.enabled", true);

That was meant to be "false", right?

Comment 11

[Andreas Gal :gal]

Why not enabled as long it works?

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Cody Brocious [:Daeken] from comment #0)
> Created attachment 611843 [details] [diff] [review]
> Initial patch
> 
> There are currently stability issues

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/5738f35ffb11