Last Comment Bug 708154 - Add (hacky) non-GL compositor backend to gonk
: Add (hacky) non-GL compositor backend to gonk
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla12
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-06 17:54 PST by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2011-12-30 04:58 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device (9.56 KB, patch)
2011-12-23 03:38 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device (17.80 KB, patch)
2011-12-23 03:41 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device (9.45 KB, patch)
2011-12-23 21:47 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device, v2 (14.38 KB, patch)
2011-12-23 22:07 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device, v2 (22.76 KB, patch)
2011-12-23 22:09 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
gal: review+
mwu.code: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-06 17:54:10 PST
We want to get tests up and running as soon as possible.  It's likely faster to hack in a CPU compositor to widget/gonk than deal with GL pass-through in the emulator.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-23 03:38:15 PST
Created attachment 584030 [details] [diff] [review]
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device

This gets b2g-gonk working in the emulator.

It's not going to apply on top of trunk; this patch and bug 713168 are racing each other to the finish, and the lose will conflict hard.  So not bothering with rebasing.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-23 03:41:45 PST
Created attachment 584032 [details] [diff] [review]
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device

oops, git fail
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-23 21:47:57 PST
Created attachment 584159 [details] [diff] [review]
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device

Updated per some comments.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-23 22:02:09 PST
Comment on attachment 584159 [details] [diff] [review]
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device

This patch is causing keyboard shortcuts to be dropped in the emulator.  Quick fix coming up.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-23 22:07:22 PST
Created attachment 584160 [details] [diff] [review]
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device, v2
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-23 22:09:18 PST
Created attachment 584161 [details] [diff] [review]
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device, v2

sigh, git fail again
Comment 7 Andreas Gal :gal 2011-12-23 22:40:03 PST
Comment on attachment 584161 [details] [diff] [review]
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device, v2

> #ifndef ABS_MT_TOUCH_MAJOR
> // Taken from include/linux/input.h
> // XXX update the bionic input.h so we don't have to do this!

Github issue would be nice.

>+    // The Linux's input documentation (Documentation/input/input.txt)
>+    // says that we'll always read a multiple of sizeof(input_event) bytes here.
>+    input_event events[16];

Not sure we would ever get that many events at once, and whether more than one ever occurs and its worth the array read here, but who cares. Its just a few bytes stack.

Stealing from mwu. Leaving a feedback? up for him.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-23 23:15:34 PST
Comment on attachment 584161 [details] [diff] [review]
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device, v2

Landed this pending-r=mwu to unblock several projects.  Will update with review comments.

https://hg.mozilla.org/integration/mozilla-inbound/rev/94b5440efa60
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-23 23:21:56 PST
(In reply to Andreas Gal :gal from comment #7)
> Comment on attachment 584161 [details] [diff] [review]
> Add a fallback non-GL rendering patch to the gonk widget backend and add
> support for the qemu touch device, v2
> 
> > #ifndef ABS_MT_TOUCH_MAJOR
> > // Taken from include/linux/input.h
> > // XXX update the bionic input.h so we don't have to do this!
> 
> Github issue would be nice.
> 

https://github.com/andreasgal/B2G/issues/102

> >+    // The Linux's input documentation (Documentation/input/input.txt)
> >+    // says that we'll always read a multiple of sizeof(input_event) bytes here.
> >+    input_event events[16];
> 
> Not sure we would ever get that many events at once, and whether more than
> one ever occurs and its worth the array read here, but who cares. Its just a
> few bytes stack.
> 

This shares the main thread with JS, so we can receive input events during GC, e.g.  In our current setup the number of events we can receive between reads is basically unbounded.  It's important to read as many events as we can at once so that we can do event compression, like collapsing several touchmoves in a row into a single touchmove event.  That's a really nice optimization that we already have in most of the other widget backends.  Using libui should get us that in gonk too.
Comment 10 Phil Ringnalda (:philor, back in August) 2011-12-24 22:14:14 PST
https://hg.mozilla.org/mozilla-central/rev/94b5440efa60
Comment 11 Michael Wu [:mwu] 2011-12-28 13:55:11 PST
What were the changes to the key handling? I couldn't tell what you did besides move some code into a separate function.
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-28 15:54:59 PST
Added support for the old single-touch input protocol in singleTouchHandler(), which qemu's virtual input device uses.
Comment 13 Michael Wu [:mwu] 2011-12-29 08:29:34 PST
Comment on attachment 584161 [details] [diff] [review]
Add a fallback non-GL rendering patch to the gonk widget backend and add support for the qemu touch device, v2

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

Looks fine. I just have comments on a few minor things.

::: widget/src/gonk/Framebuffer.cpp
@@ +63,5 @@
> +namespace mozilla {
> +
> +namespace Framebuffer {
> +
> +static int sFd = -1;

Generally I try to avoid setting static variables to anything but zero.

::: widget/src/gonk/nsAppShell.cpp
@@ +328,5 @@
> +            default:
> +                maybeSendKeyEvent(*event);
> +            }
> +        }
> +        else if (event->type == EV_ABS) {

else is on the same line as } in the rest of the file

::: widget/src/gonk/nsWindow.cpp
@@ +77,5 @@
>          gNativeWindow = new android::FramebufferNativeWindow();
>          sGLContext = GLContextProvider::CreateForWindow(this);
>          // CreateForWindow sets up gScreenBounds
> +        if (!sGLContext) {
> +            LOG("Failed to create GL context for fb, trying /dev/fb0");

Why is it /dev/fb0 here and /dev/graphics/fb0 elsewhere?
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-29 09:35:17 PST
(In reply to Michael Wu [:mwu] from comment #13)
> Comment on attachment 584161 [details] [diff] [review]
> Add a fallback non-GL rendering patch to the gonk widget backend and add
> support for the qemu touch device, v2
> 
> Review of attachment 584161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine. I just have comments on a few minor things.
> 
> ::: widget/src/gonk/Framebuffer.cpp
> @@ +63,5 @@
> > +namespace mozilla {
> > +
> > +namespace Framebuffer {
> > +
> > +static int sFd = -1;
> 
> Generally I try to avoid setting static variables to anything but zero.
> 

0 is a valid fd number, and -1 is used as a sentinel value for sFd.  If you have strong reasons for not initializing to -1, we need another type of sentinel.

> ::: widget/src/gonk/nsAppShell.cpp
> @@ +328,5 @@
> > +            default:
> > +                maybeSendKeyEvent(*event);
> > +            }
> > +        }
> > +        else if (event->type == EV_ABS) {
> 
> else is on the same line as } in the rest of the file
> 

Fixed.

> ::: widget/src/gonk/nsWindow.cpp
> @@ +77,5 @@
> >          gNativeWindow = new android::FramebufferNativeWindow();
> >          sGLContext = GLContextProvider::CreateForWindow(this);
> >          // CreateForWindow sets up gScreenBounds
> > +        if (!sGLContext) {
> > +            LOG("Failed to create GL context for fb, trying /dev/fb0");
> 
> Why is it /dev/fb0 here and /dev/graphics/fb0 elsewhere?

Muscle memory (/dev/fb0 is usually where that lives, outside of android).  Fixed.
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-29 14:48:40 PST
Followup on inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2a16bf3c2d
Comment 16 Michael Wu [:mwu] 2011-12-29 14:49:35 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #14)
> > ::: widget/src/gonk/Framebuffer.cpp
> > @@ +63,5 @@
> > > +namespace mozilla {
> > > +
> > > +namespace Framebuffer {
> > > +
> > > +static int sFd = -1;
> > 
> > Generally I try to avoid setting static variables to anything but zero.
> > 
> 
> 0 is a valid fd number, and -1 is used as a sentinel value for sFd.  If you
> have strong reasons for not initializing to -1, we need another type of
> sentinel.
> 

It's not particularly important, but zero is the most efficient value to initialize global variables since mmap gives zero'd pages. This is why I write code that either expects global variables to be init'd to zero or doesn't care what the variable is initially initialized to.
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-30 04:58:42 PST
https://hg.mozilla.org/mozilla-central/rev/fb2a16bf3c2d

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