866731 - animate caret with OMTA if webrender is used

Status: NEW

(Core :: Graphics, defect)

Description

[Andreas Gal :gal]

We should have all the code in place to paint the caret into a layer and then use AddAnimation to animate its opacity between 0 and 1.0. We should check that effective opacity 0 actually inhibits drawing the layer (I am pretty sure right now it does not, but that should be trivial to add and its a valid general optimization.

Comment 1

[Andreas Gal :gal]

Attachment: Part 1: Wrap nsDisplayCaret in a nsDisplayCaretBlink which forces a layer and later we will use it to inject the animation onto the layer

Comment 2

[Andreas Gal :gal]

Attachment: Part 2: Remove the caret blink timer

Comment 3

[Andreas Gal :gal]

Attachment: Part 3: Make the caret layer blink using OMTA

Comment 4

[Andreas Gal :gal]

This patch depends on the removal of themed carets in bug 867047 (not really a hard dependency, I just think that code should go, easy to work around).

This patch removes the old caret blink code (easy to change, I don't think we should have this optional, we should land this when we have OMTC everywhere, having two blink paths is nuts, the caret code is already unreadable).

This patch works on Mac OS X with OMTC enabled.

Bonus work:
- We should check that the compositor doesn't animate with the refresh rate once the stop value is hit early in case of a spline animation function (start and stop are both 0.0 and 1.0 here). Thats worth optimizing if its broken.
- It might be worth to introduce an update region to the compositor for trivial stuff like blinking caret. I am not sure how bad it is to blow all the GPU memory bandwidth on screen regions that don't change.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Andreas Gal :gal from comment #4)
> - It might be worth to introduce an update region to the compositor for
> trivial stuff like blinking caret. I am not sure how bad it is to blow all
> the GPU memory bandwidth on screen regions that don't change.

It might make sense for some compositors, especially the CPU compositor. I know that for D3D at least you have no option other than to repaint the entire window. I suppose we could capture the final framebuffer and blit it again, or something like that, but ick.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 743516 [details] [diff] [review]
Part 1: Wrap nsDisplayCaret in a nsDisplayCaretBlink which forces a layer and later we will use it to inject the animation onto the layer

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

I think we should be able to avoid a new display item by having nsDisplayCaret itself override GetLayerState and BuildLayer.

::: layout/base/nsDisplayList.h
@@ +2488,5 @@
> +  virtual LayerState GetLayerState(nsDisplayListBuilder* aBuilder,
> +                                   LayerManager* aManager,
> +                                   const ContainerParameters& aParameters) MOZ_OVERRIDE
> +  {
> +    return mozilla::LAYER_ACTIVE_FORCE;

LAYER_ACTIVE is OK here.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 743518 [details] [diff] [review]
Part 3: Make the caret layer blink using OMTA

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

I think we can use StepFunction here instead of CubicBezier.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Andreas Gal :gal from comment #4)
> This patch removes the old caret blink code (easy to change, I don't think
> we should have this optional, we should land this when we have OMTC
> everywhere, having two blink paths is nuts, the caret code is already
> unreadable).

Yes it is, but unfortunately we can't rely completely on the OMTC compositor for caret drawing even when all platforms support OMTC. There will still be various paths for rendering that don't go through the OMTC compositor --- CSS filters, some SVG masking stuff, esoteric features such as -moz-element(), some uses of drawWindow, printing. So these patches would need to be modified to make sure we only use layer animations when we're using a LayerManager that supports them. But that also means we don't need to wait for OMTC to land everywhere.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Matt, maybe you could take these patches over at a convenient time.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Yes it is, but unfortunately we can't rely completely on the OMTC compositor
> for caret drawing even when all platforms support OMTC. There will still be
> various paths for rendering that don't go through the OMTC compositor ---
> CSS filters, some SVG masking stuff, esoteric features such as
> -moz-element(), some uses of drawWindow, printing.

To make these clearer:

1) Suppose there's a feature like CSS filters or SVG masks that we don't support in the compositor (as is currently the case for those features). Suppose then that we have an element using that feature, say an SVG mask, and it contains the caret. We currently render that element by creating a temporary layer manager, drawing the element's contents to an Azure surface, applying the SVG mask to the surface, and uploading the result to the compositor as a ThebesLayer.

2) -moz-element() is similar but a bit more complicated. -moz-element() lets us draw the contents of an element subtree as the background for another element (useful for live thumbnails etc). Currently we do this by drawing the source element to an Azure surface and copying that surface to a ThebesLayer. To support this directly in the compositor we need to add support for cross-references in the exported layer tree (and solve some other tricky problems).

1 and 2 are fixable over time by extending the layer system to support all relevant rendering features in the compositor, but that's likely to take a long time because some of those features are not high priority.

3) Our canvas.drawWindow API in some cases needs to draw the contents of a document on the main thread without going through the OMTC compositor. For example, one of these cases is when someone requests a drawWindow of the entirety of a very large document (probably with a downscaling transform to make it fit into a reasonable-sized canvas) --- we disable clipping to the document's viewport to allow the entire document to be rendered, and furthermore the OMTC compositor's layer tree could never contain the entire document; that would be impractical for gigantic documents. Another case is when someone requests a drawWindow of a hidden tab; the OMTC compositor's layer tree doesn't contain layers for that tab. (However it's common for drawWindow users to pass the DRAWWINDOW_USE_WIDGET_LAYERS flag, in which case drawWindow actually *does* go through the regular layer compositor for better performance and testing.)

4) Printing. Obviously the regular OMTC compositor cannot be used to print.

For caret specifically, we could avoid 4 by simply never printing the caret, and we could avoid 3 by not drawing the caret if DRAWWINDOW_USE_WIDGET_LAYERS is not specified. So eventually I think we could eliminate the non-compositor caret drawing path, but for other kinds of drawing we can't eliminate problems 3 and 4.

Comment 11

[Andreas Gal :gal]

Hey roc, actually I think we can land this as is. Keep in mind that we only hide/blink the cursor with the compositor. We always draw it on the main thread. So if we take this today in every case where OMTA isn't working we are getting a solid cursor, which is not awesome, but also not that bad. What do you think?

Comment 12

[Joe Drew (not getting mail)]

We should probably ask UX. I don't know if blinking cursors are a big deal for platform integration or accessibility, for example.

Comment 13

[Stephen Horlander [:shorlander]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #12)
> We should probably ask UX. I don't know if blinking cursors are a big deal
> for platform integration or accessibility, for example.

Yes, it is important for integration and so people can easily locate their cursor position.

Comment 14

[(no longer active)]

(In reply to comment #11)
> Hey roc, actually I think we can land this as is. Keep in mind that we only
> hide/blink the cursor with the compositor. We always draw it on the main
> thread. So if we take this today in every case where OMTA isn't working we are
> getting a solid cursor, which is not awesome, but also not that bad. What do
> you think?

Not blinking the cursor would be a deviation from platform conventions, and I don't think it would be acceptable.

Comment 15

[Andreas Gal :gal]

Of course. I am not saying we would take a blinking regression on any common path, but the cursor not blinking in printed documents (not sure what that means anyway) or with -moz-element is something we might be willing to accept.

Comment 16

[(no longer active)]

(In reply to comment #15)
> Of course. I am not saying we would take a blinking regression on any common
> path, but the cursor not blinking in printed documents (not sure what that
> means anyway) or with -moz-element is something we might be willing to accept.

Hmm, good point.  AFAIK right now we don't render the caret in printed documents.  Not sure what we do for -moz-element.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Andreas Gal :gal from comment #11)
> So if we take this today in every case where OMTA isn't working we
> are getting a solid cursor, which is not awesome, but also not that bad.

I think it's pretty bad, since that's every desktop platform currently. And this is not that important.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Well, it's probably worth having for the next B2G release from mozilla-central. I think someone should add back the main-thread blinking code and land this.

Comment 19

[Andreas Gal :gal]

roc, the reason I introduced the extra display list item is because it derives from WrapList so I can do:

+  nsRefPtr<Layer> layer = aManager->GetLayerBuilder()->
+    BuildContainerLayerFor(aBuilder, aManager, mFrame, this, mList,
+                           aContainerParameters, nullptr);

Can I pass this for mList here if you want me to not have the extra DL item?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it's probably best to have nsDisplayCaret override BuildLayer and explicitly build a ColorLayer there. There is no need to build a container layer just for the caret.

Comment 21

[Andreas Gal :gal]

Ok. I will poke at this. I am out of my depth here, so if someone wants to steal, please do.

Comment 22

[Markus Stange [:mstange]]

Forcing an active layer for the caret is risky. It'll cause any inactive ancestor container layers of the textbox to become active (which changes layerization of the sibling content of those inactive layers), and it'll cause any painted display items that overlap the caret to be pushed into a new PaintedLayer on top of the caret. These things can cause lots of invalidation.

I don't have a good solution in mind, but I can construct testcases if anybody is interested. (Here's one: http://output.jsbin.com/todonicayi/1/ )

Comment 23

[Markus Stange [:mstange]]

(In reply to Andreas Gal :gal from comment #4)
> Bonus work:
> - We should check that the compositor doesn't animate with the refresh rate
> once the stop value is hit early in case of a spline animation function
> (start and stop are both 0.0 and 1.0 here). Thats worth optimizing if its
> broken.

It is broken and we have bug 1103207 about it.

> - It might be worth to introduce an update region to the compositor for
> trivial stuff like blinking caret. I am not sure how bad it is to blow all
> the GPU memory bandwidth on screen regions that don't change.

This has since been implemented for D3D11 in bug 1107297, but not for any other backends. It was tried for CompositorOGL in bug 1063776 but we decided not to pursue it there.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Layerize the caret when possible

Here's a rebased version of the old part 1 taking into account the above comments. It seems to sort of work, but on OS X desktop when I put focus in the browser URL bar it doesn't draw the caret. With mstange's help I traced this down to the |snapped| rect in BasicColorLayer::Paint. It seems to already take into account aDT->GetTransform so when it is used for clipping and fillrecting it doesn't work as intended. Untransforming it by aDT->GetTransform fixes it, but breaks other things. Not sure what's going on there.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Layerize the caret when possible

Updated with tn's help to fix the transform issue I was seeing.

Comment 26

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #22)
> Forcing an active layer for the caret is risky. It'll cause any inactive
> ancestor container layers of the textbox to become active (which changes
> layerization of the sibling content of those inactive layers), and it'll
> cause any painted display items that overlap the caret to be pushed into a
> new PaintedLayer on top of the caret. These things can cause lots of
> invalidation.

This is not an issue with WebRender. I think we should do this now, if WebRender is used.