1059583 - Track whether a layer needs to be synchronized with other layer updates or not

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Updates coming from JS need to be synchronized, in that any changes done before JS returned control to the browser must be displayed simultaneously. This allows composing canvas2d/dom/webgl/css3d and having them be consistent.

If the browser fails to synchronize these, you might get an overlay lagging behind an underlay, like markers on a map not keeping their location as the map is dragged.

One clear case where a layer is alone in its 'synchronization group' is under fullscreen mode for the layer's element. In this case, we are free to inject a frame of latency to get better throughput, and any other tricks that usually are blocked by synchronization.

In order to identify optimization opportunities, we should have a query that tells us if a layer needs to stay synchronized or not.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Unfortunately it appears that WebGL canvases are always transparent, so we need to be careful of changes underneath the canvas as well as ones on top of it.

Comment 2

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> Unfortunately it appears that WebGL canvases are always transparent, so we
> need to be careful of changes underneath the canvas as well as ones on top
> of it.

They are not always transparent. I believe they default to having alpha, but it's optional.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: is-layer-sync

The name can probably be much better, so suggestions welcome.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8480329 [details] [diff] [review]
is-layer-sync

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

::: dom/canvas/WebGLContext.h
@@ +195,5 @@
>                                nsIInputStream **aStream) MOZ_OVERRIDE;
>      mozilla::TemporaryRef<mozilla::gfx::SourceSurface> GetSurfaceSnapshot(bool* aPremultAlpha) MOZ_OVERRIDE;
>  
>      NS_IMETHOD SetIsOpaque(bool b) MOZ_OVERRIDE { return NS_OK; };
> +    bool GetIsOpaque() MOZ_OVERRIDE { return false; };

Were you going to change this to return !HasAlpha()?

::: gfx/layers/Layers.cpp
@@ +1921,5 @@
>  
>  PRLogModuleInfo* LayerManager::sLog;
>  
> +static bool
> +DoesIntersect(const nsIntRegion& a, const nsIntRegion& b)

nsIntRegion::Intersects

@@ +1927,5 @@
> +    return !a.Intersect(b).IsEmpty();
> +}
> +
> +bool
> +Layer::DoesIntersect_RecurseSiblingsAndChildren(const nsIntRegion& ref) const

Siblings aren't necessarily in the same coordinate space as us.

I think what we need to do is:

Take our GetVisibleRegion() and transform it by our transform.

Loop over GetNextSibling doing the same with their visible region and transform checking for intersections.

If that doesn't find anything, convert the visible region into the coordinate space of our parent using its transform.

Then repeat the GetNextSibling loop for siblings of our parent. We don't need to go back down here because the visible region of our parent's siblings includes the visible region of our parent's sibling's children.

Repeat going up ancestors if necessary.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8480329 [details] [diff] [review]
is-layer-sync

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

Alright, sounds like an r-.

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Comment on attachment 8480329 [details] [diff] [review]
> is-layer-sync
> 
> Review of attachment 8480329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContext.h
> @@ +195,5 @@
> >                                nsIInputStream **aStream) MOZ_OVERRIDE;
> >      mozilla::TemporaryRef<mozilla::gfx::SourceSurface> GetSurfaceSnapshot(bool* aPremultAlpha) MOZ_OVERRIDE;
> >  
> >      NS_IMETHOD SetIsOpaque(bool b) MOZ_OVERRIDE { return NS_OK; };
> > +    bool GetIsOpaque() MOZ_OVERRIDE { return false; };
> 
> Were you going to change this to return !HasAlpha()?
I was, but I decided not to mess with it. I'd rather have correct information coming from only one source, rather than try to keep two sources of information correct, one of which is ignored.
> 
> ::: gfx/layers/Layers.cpp
> @@ +1921,5 @@
> >  
> >  PRLogModuleInfo* LayerManager::sLog;
> >  
> > +static bool
> > +DoesIntersect(const nsIntRegion& a, const nsIntRegion& b)
> 
> nsIntRegion::Intersects
I didn't see an overload for nsIntRegion::Intersects(nsIntRegion), but I can try it.
> 
> @@ +1927,5 @@
> > +    return !a.Intersect(b).IsEmpty();
> > +}
> > +
> > +bool
> > +Layer::DoesIntersect_RecurseSiblingsAndChildren(const nsIntRegion& ref) const
> 
> Siblings aren't necessarily in the same coordinate space as us.
> 
> I think what we need to do is:
> 
> Take our GetVisibleRegion() and transform it by our transform.
> 
> Loop over GetNextSibling doing the same with their visible region and
> transform checking for intersections.
> 
> If that doesn't find anything, convert the visible region into the
> coordinate space of our parent using its transform.
> 
> Then repeat the GetNextSibling loop for siblings of our parent. We don't
> need to go back down here because the visible region of our parent's
> siblings includes the visible region of our parent's sibling's children.
> 
> Repeat going up ancestors if necessary.

Could you possibly write this? I don't really know what I'm doing with layer coord spaces.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Layer overlap check

Something like this should work. Builds, haven't tested it with anything.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Let's start off simple with just the is-element-fullscreen check.

Comment 9

[u480271]

Comment on attachment 8483981 [details] [diff] [review]
patch

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

::: dom/canvas/WebGLContext.cpp
@@ +1164,5 @@
>  private:
>      nsRefPtr<HTMLCanvasElement> mContent;
>  };
>  } // end namespace mozilla
> +bool

WS above

@@ +1230,3 @@
>      canvasLayer->Initialize(data);
> +    if (!data.mHasAlpha) {
> +        auto flags = canvasLayer->GetContentFlags();

Are we cool to use the auto keyword? (Save all the typing!)

::: dom/canvas/WebGLContext.h
@@ +194,5 @@
>                                const char16_t* aEncoderOptions,
>                                nsIInputStream **aStream) MOZ_OVERRIDE;
>      mozilla::TemporaryRef<mozilla::gfx::SourceSurface> GetSurfaceSnapshot(bool* aPremultAlpha) MOZ_OVERRIDE;
>      NS_IMETHOD SetIsOpaque(bool b) MOZ_OVERRIDE { return NS_OK; };
> +    bool GetIsOpaque() MOZ_OVERRIDE { return false; };

What does this do? Should it return !HasAlpha()?

::: gfx/layers/Layers.cpp
@@ +1982,5 @@
> +{
> +  if (mIsElemFullscreen)
> +    return false;
> +
> +  return true;

return (!mIsElemFullscreen), no? You may notice I prefer simple boolean logic. If the function is extended later, then do the extra work. (YAGNI, blah blah blah)

::: gfx/layers/Layers.h
@@ +2016,5 @@
>      bool mHasAlpha;
>      // Whether mGLContext contains data that is alpha-premultiplied.
>      bool mIsGLAlphaPremult;
> +
> +    bool mIsElemFullscreen;

Do layers need to carry this state around? Do layers not have a pointer to their elem so that:

   return elem->State().HasState(NS_EVENT_STATE_FULL_SCREEN);

Can be called at the point you return mIsElemFullscreen?

I profess to knowing nothing about layers, so this may be a question directed at :mattwoodrow.

Comment 10

[Matt Woodrow (:mattwoodrow)]

(In reply to Dan Glastonbury :djg :kamidphish from comment #9)

> Do layers need to carry this state around? Do layers not have a pointer to
> their elem so that:
> 
>    return elem->State().HasState(NS_EVENT_STATE_FULL_SCREEN);
> 
> Can be called at the point you return mIsElemFullscreen?
> 
> I profess to knowing nothing about layers, so this may be a question
> directed at :mattwoodrow.

Nope, layers have no visibility into layout/content at all.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8483981 [details] [diff] [review]
patch

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

r+ with the rest of Dan's comments fixed (I'm not a huge fan of auto apart from stupidly verbose iterators).

Comment 12

[Kelsey Gilbert [:jgilbert]]

Ugh, I think I keep hitting this bug:
http://bz.selenic.com/show_bug.cgi?id=3879

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: basic patch

Comment 14

[Kelsey Gilbert [:jgilbert]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/96a507248c53

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e776483c4213

https://tbpl.mozilla.org/php/getParsedLog.php?id=47728163&tree=Mozilla-Inbound

Comment 16

[Kelsey Gilbert [:jgilbert]]

We have a better solution for this in the form of delayed transactions. (from :benwa)