Closed Bug 708154 Opened 13 years ago Closed 13 years ago

Add (hacky) non-GL compositor backend to gonk

Categories

(Core :: Widget, defect)

Product:
Core
Component:
Widget
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla12

People

(Reporter: cjones, Assigned: cjones)

Details

Attachments

(1 file, 4 obsolete files)

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
gal
: review+
mwu
: review+
Details | Diff | Splinter Review 
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.
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.
Assignee: nobody → jones.chris.g
Attachment #584030 - Flags: review?(mwu)
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.
Attachment #584159 - Attachment is obsolete: true
Attachment #584159 - Flags: review?(mwu)
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.
Attachment #584161 - Flags: review?(mwu)
Attachment #584161 - Flags: review+
Attachment #584161 - Flags: feedback?(mwu)
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
Attachment #584161 - Flags: feedback?(mwu) → review?(mwu)
(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.
What were the changes to the key handling? I couldn't tell what you did besides move some code into a separate function.
Added support for the old single-touch input protocol in singleTouchHandler(), which qemu's virtual input device uses.
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?
Attachment #584161 - Flags: review?(mwu) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/fb2a16bf3c2d
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: