Open Bug 866731 Opened 11 years ago Updated 23 days ago

animate caret with OMTA if webrender is used

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86
macOS
Type:
defect

Tracking

()

People

(Reporter: gal, Unassigned)

References

Details

Attachments

(4 files, 1 obsolete file)

Part 1: Wrap nsDisplayCaret in a nsDisplayCaretBlink which forces a layer and later we will use it to inject the animation onto the layer
5.61 KB, patch
Details | Diff | Splinter Review
Part 2: Remove the caret blink timer
5.24 KB, patch
Details | Diff | Splinter Review
Part 3: Make the caret layer blink using OMTA
1.88 KB, patch
Details | Diff | Splinter Review
Part 1 - Layerize the caret when possible
5.85 KB, patch
Details | Diff | Splinter Review 
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.
Depends on: 866729
Depends on: 867047
No longer depends on: 866729
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.
Attachment #743516 - Flags: review?(roc)
Attachment #743517 - Flags: review?(roc)
Attachment #743518 - Flags: review?(roc)
(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 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 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.
(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.
Matt, maybe you could take these patches over at a convenient time.
(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.
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?
We should probably ask UX. I don't know if blinking cursors are a big deal for platform integration or accessibility, for example.
(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.
(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.
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.
(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.
(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.
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.
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?
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.
Ok. I will poke at this. I am out of my depth here, so if someone wants to steal, please do.
Blocks: 990974
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/ )
(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.
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.
Updated with tn's help to fix the transform issue I was seeing.
Attachment #8608326 - Attachment is obsolete: true
(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.
Assignee: gal → nobody
Summary: animate caret with OMTA → animate caret with OMTA if webrender is used
Severity: normal → S3
Component: Graphics: Layers → Graphics: WebRender
Component: Graphics: WebRender → Graphics
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: