1082890 - Make BasicCompositor work with gonk widgetry again

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I added this back as a fallback in case we can't resolve the GL woes on rpi in the remaining timeframe.  Yes, perf takes a ding.  Patch forthcoming.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Make BasicCompositor work for gonk widgets, again

Not a happy-making patch, but thankfully quite simple.

Comment 2

[Michael Wu [:mwu]]

Comment on attachment 8505173 [details] [diff] [review]
Make BasicCompositor work for gonk widgets, again

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

In general, I'm opposed to having the basic drawing path work. If gl doesn't work, it should fail ASAP. But if we have to, this approach seems reasonable. Maybe it can be restricted to ICS so we still fail fast on newer platforms.

::: widget/gonk/nsWindow.cpp
@@ +508,5 @@
> +}
> +
> +TemporaryRef<DrawTarget>
> +nsWindow::StartRemoteDrawing()
> +{

This function is misindented.

@@ +513,5 @@
> +  GonkDisplay* display = GetGonkDisplay();
> +  mFramebuffer = display->DequeueBuffer();
> +  int width = mFramebuffer->width, height = mFramebuffer->height;
> +  void *vaddr;
> +  if (gralloc_module()->lock(gralloc_module(), mFramebuffer->handle,

I don't see any code unlocking this buffer after we're done with it.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Michael Wu [:mwu] from comment #2)
> Comment on attachment 8505173 [details] [diff] [review]
> Make BasicCompositor work for gonk widgets, again
> 
> Review of attachment 8505173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general, I'm opposed to having the basic drawing path work.

Trust me, I hate this much more than you do :/.  This is unfortunately a last-ditch fallback because the GL implementation on the RPi is too buggy to use atm.

> If gl doesn't
> work, it should fail ASAP. But if we have to, this approach seems
> reasonable. Maybe it can be restricted to ICS so we still fail fast on newer
> platforms.

Note, you have to flip several prefs before the drawing path here is even attempted to be used.  It's not something that could ever run by accident as things are implemented currently.  However, I could envision this path being generally useful for debugging or bring-up purposes, much like I'm using it for now.

> 
> ::: widget/gonk/nsWindow.cpp
> @@ +508,5 @@
> > +}
> > +
> > +TemporaryRef<DrawTarget>
> > +nsWindow::StartRemoteDrawing()
> > +{
> 
> This function is misindented.

Fixed.

> 
> @@ +513,5 @@
> > +  GonkDisplay* display = GetGonkDisplay();
> > +  mFramebuffer = display->DequeueBuffer();
> > +  int width = mFramebuffer->width, height = mFramebuffer->height;
> > +  void *vaddr;
> > +  if (gralloc_module()->lock(gralloc_module(), mFramebuffer->handle,
> 
> I don't see any code unlocking this buffer after we're done with it.

Ah, quite right.  (In my gralloc impl, unlock is a no-op.)  Fixed.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Make BasicCompositor work for gonk widgets, again, v2

Comment 5

[Michael Wu [:mwu]]

Comment on attachment 8505983 [details] [diff] [review]
Make BasicCompositor work for gonk widgets, again, v2

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

Sounds like this is sufficiently hard to use that it won't be easily abused. Good enough for me.

::: widget/gonk/nsWindow.cpp
@@ +539,5 @@
> +    }
> +    if (mFramebuffer) {
> +        GetGonkDisplay()->QueueBuffer(mFramebuffer);
> +    }
> +    mFramebuffer = nullptr;   

nit: trailing whitespace

::: widget/gonk/nsWindow.h
@@ +18,5 @@
>  
>  #include "nsBaseWidget.h"
>  #include "nsRegion.h"
>  #include "nsIIdleServiceInternal.h"
> +#include "Units.h"

What's this include for?

@@ +97,5 @@
>  
>      NS_IMETHOD MakeFullScreen(bool aFullScreen) /*MOZ_OVERRIDE*/;
>  
> +    virtual mozilla::TemporaryRef<mozilla::gfx::DrawTarget>
> +    StartRemoteDrawing() MOZ_OVERRIDE;

nit: indent this

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Michael Wu [:mwu] from comment #5)
> ::: widget/gonk/nsWindow.cpp
> @@ +539,5 @@
> > +    }
> > +    if (mFramebuffer) {
> > +        GetGonkDisplay()->QueueBuffer(mFramebuffer);
> > +    }
> > +    mFramebuffer = nullptr;   
> 
> nit: trailing whitespace

Fixed.

> 
> ::: widget/gonk/nsWindow.h
> @@ +18,5 @@
> >  
> >  #include "nsBaseWidget.h"
> >  #include "nsRegion.h"
> >  #include "nsIIdleServiceInternal.h"
> > +#include "Units.h"
> 
> What's this include for?
> 

Oh, that's so that we can use ScreenIntPoint.  I guess it should go in the cursor patch, but it's not really hurting anything and I'm not particularly inclined to juggle patch hunks ;) ... do you mind if we keep it?

> @@ +97,5 @@
> >  
> >      NS_IMETHOD MakeFullScreen(bool aFullScreen) /*MOZ_OVERRIDE*/;
> >  
> > +    virtual mozilla::TemporaryRef<mozilla::gfx::DrawTarget>
> > +    StartRemoteDrawing() MOZ_OVERRIDE;
> 
> nit: indent this

Not sure about the style you're referring to here but I indented it 4 spaces.  Fixed?

Comment 7

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need me from comment #6)
> > ::: widget/gonk/nsWindow.h
> > @@ +18,5 @@
> > >  
> > >  #include "nsBaseWidget.h"
> > >  #include "nsRegion.h"
> > >  #include "nsIIdleServiceInternal.h"
> > > +#include "Units.h"
> > 
> > What's this include for?
> > 
> 
> Oh, that's so that we can use ScreenIntPoint.  I guess it should go in the
> cursor patch, but it's not really hurting anything and I'm not particularly
> inclined to juggle patch hunks ;) ... do you mind if we keep it?
> 

Sure why not.

> > @@ +97,5 @@
> > >  
> > >      NS_IMETHOD MakeFullScreen(bool aFullScreen) /*MOZ_OVERRIDE*/;
> > >  
> > > +    virtual mozilla::TemporaryRef<mozilla::gfx::DrawTarget>
> > > +    StartRemoteDrawing() MOZ_OVERRIDE;
> > 
> > nit: indent this
> 
> Not sure about the style you're referring to here but I indented it 4
> spaces.  Fixed?

I mean something like:

virtual mozilla::TemporaryRef<mozilla::gfx::DrawTarget>
    StartRemoteDrawing() MOZ_OVERRIDE;

So it's easier to see that this is a single definition.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Michael Wu [:mwu] from comment #7)
> (In reply to Chris Jones [:cjones] temporarily active; ni?/f?/r? if you need
> > Not sure about the style you're referring to here but I indented it 4
> > spaces.  Fixed?
> 
> I mean something like:
> 
> virtual mozilla::TemporaryRef<mozilla::gfx::DrawTarget>
>     StartRemoteDrawing() MOZ_OVERRIDE;
> 
> So it's easier to see that this is a single definition.

OK cool, that's the change I made.  Fixed! :)

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Make BasicCompositor work for gonk widgets, again, v3

Carrying r+.  Looks OK on tryserver https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=31b02e393189

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/80a92f907fbd

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80a92f907fbd