Gonk fallback rendering path is broken

RESOLVED FIXED in mozilla16

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: Daeken)

Tracking

unspecified
mozilla16
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The logic in nsWindow::nsWindow() is no longer correct.
(Assignee)

Updated

5 years ago
Assignee: nobody → cbrocious
(Assignee)

Comment 1

5 years ago
Created attachment 615853 [details] [diff] [review]
Implement proper fallback

Implemented OMTC fallback path by moving context construction out of NSWindow's constructor.
Attachment #615853 - Flags: review?(jones.chris.g)
Comment on attachment 615853 [details] [diff] [review]
Implement proper fallback

>diff --git a/widget/gonk/nsWindow.cpp b/widget/gonk/nsWindow.cpp

>+static bool sConstructed;

This name is a little confusing: let's call it sScreenInitialized.

>+    sGLContext = GLContextProvider::CreateForWindow(this);

By this point, we've already set gScreenSize based on
Framebuffer::GetSize().  They /should/ be the same, but let's do the
following

    DebugOnly<nsIntRect> fbBounds = gScreenBounds;
    sGLContext = GLContextProvider::CreateForWindow(this);
    MOZ_ASSERT(fbBounds == gScreenBounds);

to be sure.

>+        } else
>             LOG("Could not create OGL LayerManager");

Nit: braces around this else clause.

>+    nsIntSize screenSize;
>+    sFramebufferOpen = Framebuffer::Open(&screenSize);

If we're always getting the size from GetSize(), we can remove the
param to this function.

Also, please make the change to GetSize() that we discussed on IRC.

r=me with those changes.
Attachment #615853 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 615973 [details] [diff] [review]
Fixed issues raised by cjones

I've implemented fixes for the issues mentioned in the last comment and what we spoke about on IRC.
Attachment #615853 - Attachment is obsolete: true
Attachment #615973 - Flags: review?(jones.chris.g)
Comment on attachment 615973 [details] [diff] [review]
Fixed issues raised by cjones

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> Comment on attachment 615853 [details] [diff] [review]
> Implement proper fallback
>
> >+    sGLContext = GLContextProvider::CreateForWindow(this);
> 
> By this point, we've already set gScreenSize based on
> Framebuffer::GetSize().  They /should/ be the same, but let's do the
> following
> 
>     DebugOnly<nsIntRect> fbBounds = gScreenBounds;
>     sGLContext = GLContextProvider::CreateForWindow(this);
>     MOZ_ASSERT(fbBounds == gScreenBounds);
> 
> to be sure.

Didn't address this.

>diff --git a/widget/gonk/Framebuffer.cpp b/widget/gonk/Framebuffer.cpp

>+static gfxIntSize *sScreenSize = nsnull;

Need to free this in Close().

> bool
> GetSize(nsIntSize *aScreenSize) {
>-    if (0 <= sFd)
>+    // If the framebuffer has been opened, we should always have the size.
>+    if (0 <= sFd || sScreenSize) {
>+        MOZ_ASSERT(sScreenSize);

No need to assert this: if it's null, we'll just crash anyway.

Would like to see the version with those fixed.
Attachment #615973 - Flags: review?(jones.chris.g)
(Assignee)

Comment 5

5 years ago
Created attachment 615984 [details] [diff] [review]
Fixed most recent issues raised by cjones
Attachment #615973 - Attachment is obsolete: true
Attachment #615984 - Flags: review?
Created attachment 636608 [details] [diff] [review]
Fix OMTC on gonk

patch bitrotted.
Attachment #615984 - Attachment is obsolete: true
Attachment #615984 - Flags: review?
Attachment #636608 - Flags: review?(jones.chris.g)
Comment on attachment 636608 [details] [diff] [review]
Fix OMTC on gonk

r+ with free in Close().  Changed locally, will push to inbound.
Attachment #636608 - Flags: review?(jones.chris.g) → review+
Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/a22ca1e05bb8
then backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/074372a19576
then relanded https://hg.mozilla.org/integration/mozilla-inbound/rev/a047da349d59

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a047da349d59
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Duplicate of this bug: 768646
Depends on: 770440
You need to log in before you can comment on or make changes to this bug.