Closed Bug 1082890 Opened 5 years ago Closed 5 years ago

Make BasicCompositor work with gonk widgetry again

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Not a happy-making patch, but thankfully quite simple.
Attachment #8505173 - Flags: review?(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.
Attachment #8505173 - Flags: review?(mwu)
(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 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
Attachment #8505983 - Flags: review?(mwu) → review+
(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?
(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.
(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! :)
https://hg.mozilla.org/mozilla-central/rev/80a92f907fbd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.