Add (hacky) non-GL compositor backend to gonk

RESOLVED FIXED in mozilla12

Status

()

Core
Widget
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
mozilla12
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
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.
Assignee: nobody → jones.chris.g
Attachment #584030 - Flags: review?(mwu)
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
Attachment #584030 - Attachment is obsolete: true
Attachment #584030 - Flags: review?(mwu)
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.
Attachment #584032 - Attachment is obsolete: true
Attachment #584159 - 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)
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
Attachment #584160 - Flags: review?(mwu)
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
Attachment #584160 - Attachment is obsolete: true
Attachment #584160 - Flags: review?(mwu)
Attachment #584161 - Flags: review?(mwu)

Comment 7

6 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/94b5440efa60
Target Milestone: --- → mozilla12

Comment 11

6 years ago
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 13

6 years ago
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.
Followup on inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2a16bf3c2d

Comment 16

6 years ago
(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
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.