Last Comment Bug 743612 - Gonk fallback rendering path is broken
: Gonk fallback rendering path is broken
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla16
Assigned To: Cody Brocious [:Daeken]
:
: Milan Sreckovic [:milan]
Mentors:
: 768646 (view as bug list)
Depends on: 770440
Blocks: 741837
  Show dependency treegraph
 
Reported: 2012-04-08 23:20 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-07-03 01:30 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement proper fallback (3.42 KB, patch)
2012-04-17 13:38 PDT, Cody Brocious [:Daeken]
cjones.bugs: review+
Details | Diff | Splinter Review
Fixed issues raised by cjones (6.53 KB, patch)
2012-04-17 17:52 PDT, Cody Brocious [:Daeken]
no flags Details | Diff | Splinter Review
Fixed most recent issues raised by cjones (6.59 KB, patch)
2012-04-17 19:01 PDT, Cody Brocious [:Daeken]
no flags Details | Diff | Splinter Review
Fix OMTC on gonk (9.88 KB, patch)
2012-06-25 23:39 PDT, Kan-Ru Chen [:kanru] (UTC+8)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-08 23:20:52 PDT
The logic in nsWindow::nsWindow() is no longer correct.
Comment 1 Cody Brocious [:Daeken] 2012-04-17 13:38:36 PDT
Created attachment 615853 [details] [diff] [review]
Implement proper fallback

Implemented OMTC fallback path by moving context construction out of NSWindow's constructor.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-17 17:13:41 PDT
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.
Comment 3 Cody Brocious [:Daeken] 2012-04-17 17:52:04 PDT
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.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-17 18:18:19 PDT
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.
Comment 5 Cody Brocious [:Daeken] 2012-04-17 19:01:27 PDT
Created attachment 615984 [details] [diff] [review]
Fixed most recent issues raised by cjones
Comment 6 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-25 23:39:28 PDT
Created attachment 636608 [details] [diff] [review]
Fix OMTC on gonk

patch bitrotted.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-26 07:36:08 PDT
Comment on attachment 636608 [details] [diff] [review]
Fix OMTC on gonk

r+ with free in Close().  Changed locally, will push to inbound.
Comment 9 Ed Morley [:emorley] 2012-06-27 03:37:10 PDT
https://hg.mozilla.org/mozilla-central/rev/a047da349d59
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-27 07:43:36 PDT
*** Bug 768646 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.