Minor optimizations for LayerView.Compositor

RESOLVED FIXED in Firefox 51

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

unspecified
mozilla51
All
Android
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

No description provided.
Priority: -- → P3
Now that nsWindow doesn't own LayerViewSupport, we shouldn't be using
WindowEvent for LayerViewSupport calls. This patch converts the calls
that dispatch to proxy to dispatch directly to Gecko. For
SyncResumeResizeCompositor, it used a proxy to call OnResumedCompositor
on the Gecko thread; this patch makes SyncResumeResizeCompositor post an
event to call OnResumedCompositor directly, without going through the
proxy.
Attachment #8784560 - Flags: review?(snorp)
Get rid of LayerView.Compositor.getSurface and just pass in the surface
when creating or resuming the compositor. That also lets us get rid of
some synchronization required for getSurface.
Attachment #8784561 - Flags: review?(snorp)
Attachment #8784560 - Flags: review?(snorp) → review+
Comment on attachment 8784561 [details] [diff] [review]
Get surface when creating or resuming compositor (v1)

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

I think you need to clear mSurface in SyncPauseCompositor(), otherwise we'll end up returning a stale one. Previously, LVS::GetSurface() would've obtained a null one and set it there.

That being said, we should never be trying to consume a null surface, so it's probably not a huge deal in practice.
Attachment #8784561 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #3)
> Comment on attachment 8784561 [details] [diff] [review]
> Get surface when creating or resuming compositor (v1)
> 
> Review of attachment 8784561 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you need to clear mSurface in SyncPauseCompositor(), otherwise we'll
> end up returning a stale one. Previously, LVS::GetSurface() would've
> obtained a null one and set it there.
> 
> That being said, we should never be trying to consume a null surface, so
> it's probably not a huge deal in practice.

Yeah, we hard crash in GLContextProviderEGL if we return a null surface. Other code calls SyncPauseCompositor expecting the surface to stay the same, so I didn't clear the surface in SyncPauseCompositor.
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59fceff95c70
Don't use WindowEvent for LayerViewSupport; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/db1826bc2da5
Get surface when creating or resuming compositor; r=snorp
https://hg.mozilla.org/mozilla-central/rev/59fceff95c70
https://hg.mozilla.org/mozilla-central/rev/db1826bc2da5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.