Closed Bug 1337130 Opened 7 years ago Closed 7 years ago

Create a DisplayItemLayer type

Categories

(Core :: Graphics: WebRender, defect)

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54

People

(Reporter: mchang, Assigned: mchang)

References

Details

(Whiteboard: gfx-noted)

Attachments

(3 files, 3 obsolete files)

From our discussion today, we'd like to create a generic layer type that doesn't toss information until we have to feed the display item data into webrender. This is an outline of a DisplayItemLayer type that just contains a pointer to the display item. When we create a layer, we just ask the display item to paint itself. We can evolve this later to become "generate webrender display items for yourself". 

This patch adds basic support for a DisplayItemLayer with basic layers for testing. This also adds support for the nsDisplayCaret item type as an example of how to use it. I'll follow up with the webrender implementation if this looks good.
Whiteboard: gfx-noted
Attachment #8834130 - Flags: review?(jmuizelaar)
Comment on attachment 8834130 [details] [diff] [review]
Add support for a DisplayItemLayer generic type

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

::: gfx/layers/basic/BasicDisplayItemLayer.cpp
@@ +53,5 @@
> +    if (IsHidden()) {
> +      return;
> +    }
> +
> +    printf_stderr("BasicDisplayItemLayer::Paint");

nit: Will delete this.
Comment on attachment 8834130 [details] [diff] [review]
Add support for a DisplayItemLayer generic type

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

This looks reasonable to me. Let's get a mstange to look.
Attachment #8834130 - Flags: review?(mstange)
Attachment #8834130 - Flags: review?(jmuizelaar)
Attachment #8834130 - Flags: review+
Comment on attachment 8834130 [details] [diff] [review]
Add support for a DisplayItemLayer generic type

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

::: gfx/layers/Layers.h
@@ +2313,5 @@
> +
> +  virtual void DumpPacket(layerscope::LayersPacket* aPacket, const void* aParent) override;
> +
> +  // TODO: check how to properly not memory leak this.
> +  nsDisplayItem* mItem;

Display items are allocated into the display list builder arena, which gets destroyed at the end of the painting process.

Layers are retained between paints, so this will point to destroyed memory most of the time (and we can pretty easily trigger case where it'll be read from).

We really need to convert the display item into something we can retain for this to work. One option is to create an SkPicture/Moz2D recording of the Paint command and store that instead.

We can't just deep-copy the display item itself, since internally it references the frame tree and style data, both of which can be modified (or deleted!) any time outside of painting. We don't have any refcounting or copy-on-write semantics for frames/style data.
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Comment on attachment 8834130 [details] [diff] [review]
> Add support for a DisplayItemLayer generic type
> 
> Review of attachment 8834130 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Layers.h
> @@ +2313,5 @@
> > +
> > +  virtual void DumpPacket(layerscope::LayersPacket* aPacket, const void* aParent) override;
> > +
> > +  // TODO: check how to properly not memory leak this.
> > +  nsDisplayItem* mItem;
> 
> Display items are allocated into the display list builder arena, which gets
> destroyed at the end of the painting process.
> 
> Layers are retained between paints, so this will point to destroyed memory
> most of the time (and we can pretty easily trigger case where it'll be read
> from).
>

Yeah. Jeff, Markus, and I discussed this and at the end of a transaction, we'll null out the pointer to the display item so that we can't access the destroyed memory.
 
> We really need to convert the display item into something we can retain for
> this to work. One option is to create an SkPicture/Moz2D recording of the
> Paint command and store that instead.
> 
> We can't just deep-copy the display item itself, since internally it
> references the frame tree and style data, both of which can be modified (or
> deleted!) any time outside of painting. We don't have any refcounting or
> copy-on-write semantics for frames/style data.

The end goal of this isn't to actually use this to paint layers. It's really just to funnel the data so that we can translate it into webrender commands. This seems to work in the sense of we retain the display item long enough such that we can translate it into webrender commands during the layer transaction.
Attachment #8834130 - Attachment is obsolete: true
Attachment #8834130 - Flags: review?(mstange)
Attachment #8834619 - Flags: review?(mstange)
Attachment #8834619 - Flags: review?(matt.woodrow)
Comment on attachment 8834619 [details] [diff] [review]
Add support for a DisplayItemLayer generic type

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

We have a concept called 'empty transactions' where we just composite the existing layer tree without going through display list building and layer tree building. There's a few ways we can trigger these, but the most important is pages with an animated <canvas>. If the only invalidation is just the canvas contents changing, then we'll take the empty transaction path and composite the old layer tree, with the new canvas buffer. Plugins and animated images can hit this too, video won't usually (because we have full async video).

So we either need all DisplayItemLayer implementations to be able to render themselves at any time (not just within a normal full transaction), or we need to disable empty transactions in the presence of DisplayItemLayers.

In the ideal world, we'd be able to guarantee that display list building, layer building and serialization to WR would always take under 16ms (for the set of use cases we care about) and then there wouldn't be much advantage to empty transactions (except reduced CPU time/ battery consumption).

Unfortunately I don't think we're at that point yet, and won't be without some serious display list work, which is likely well outside the scope of WR. Disabling empty transactions will probably be a real regression for canvas/plugin pages, and I'm not convinced that we can get away with that.

I'm happy to have a Skype/Vidyo chat about this if that'd be helpful :)
Attachment #8834619 - Flags: review?(matt.woodrow)
Matt and I discussed this and a couple of things.

1) We're both ok with a DisplayItemLayer as a medium to transport display list items.
2) We're ok with nulling out the display item pointer at the end of a transaction.
3) In regard to empty transactions, what we came up with was that the DisplayItemLayer, after translating to a WR display item, would keep the WR display item data. When an empty transaction occurs, if the display item is no longer accessible, we just reuse the WR display item. This cached WR data would be thrown out when the layer is thrown away in FrameLayerBuilder at the next transactions.
  - An alternative would be to have WebRender itself retain the textures and we'd try to constrain EmptyTransactions to just update specific textures. Then WebRender would only throw out textures once the next full transaction happened.
Also note, we don't really care about the implementation for BasicLayers. It's just there to test and make sure we're requesting the right thing, but we could maintain a DrawTargetRecording / SkPicture if need be.
Comment on attachment 8834619 [details] [diff] [review]
Add support for a DisplayItemLayer generic type

Matt would like to see an implementation of caching the WR required items. Will ask for review once that is done.
Attachment #8834619 - Flags: review?(mstange)
Requires part 1 and depends on the graphics branch. Essentially replicates nsDisplayCarat::Paint to create WebRenderCommands. Also includes the bare bones necessary to cache the WebRenderCommands assuming we retained the layer.

I'm still not quite sure how we're going to do an empty transaction, but I guess that's the work in WebRenderLayerManager.
Attachment #8835166 - Flags: review?(matt.woodrow)
Attachment #8834619 - Flags: review?(matt.woodrow)
Comment on attachment 8834619 [details] [diff] [review]
Add support for a DisplayItemLayer generic type

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +737,4 @@
>        return MakeUnique<BorderLayerProperties>(static_cast<BorderLayer*>(aRoot));
>      case Layer::TYPE_TEXT:
>        return MakeUnique<TextLayerProperties>(static_cast<TextLayer*>(aRoot));
> +    case Layer::TYPE_DISPLAYITEM:

We might need to figure out a solution for this. It's used for computing the changed region passed to MozAfterPaint, which the reftest harness uses.

The 'normal' way of comparing display items is DLBI, maybe we can make FrameLayerBuilder run that for DisplayItemLayers and store it on the 'invalid region' for the layer.

::: gfx/layers/Layers.cpp
@@ +2130,5 @@
> +void
> +DisplayItemLayer::PrintInfo(std::stringstream& aStream, const char* aPrefix)
> +{
> +  Layer::PrintInfo(aStream, aPrefix);
> +  AppendToString(aStream, mItem->GetType(), " [item type=", "]");

Null check mItem? Or cache the type (DebugOnly<>)?

::: gfx/layers/Layers.h
@@ +2294,5 @@
> + * It's ok to recycle a DisplayItemLayer for a different display item since its just a pointer.
> + * Instead, when a layer transaction is completed, it is up to the layer manager to tell
> + * DisplayItemLayers that the display item pointer is no longer valid.
> + *
> + * Backstory:

As much as I enjoy this, maybe it doesn't need to be checked in.

::: gfx/layers/basic/BasicDisplayItemLayer.cpp
@@ +53,5 @@
> +    if (IsHidden()) {
> +      return;
> +    }
> +
> +    mItem->Paint(aDT, aDeviceOffset, aMaskLayer);

Null check here too.

::: layout/painting/nsDisplayList.h
@@ +1889,5 @@
>  
> +  /**
> +   * Paint the item using the raw target.
> +   */
> +  virtual void Paint(DrawTarget* aDT,

Can we avoid adding a new Paint() override that items need to implement?

For WRLayerManager, I assume we want to serialize the mask separately, and for BasicLayerManager, take a look at BasicPaintedLayer::PaintThebes (the !IsRetained() path) for an example of how to handle the mask outside of this call.

We should just instantiate a new nsRenderingContext around the DrawTarget in the caller, and then use the old paint method.

Feel free to add a non-virtual Paint function that does the DrawTarget->nsRenderingContext wrapping and then calls the old one.

We really need to just get rid of nsRenderingContext so that this stops being a problem.
Attachment #8834619 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8835166 [details] [diff] [review]
Translate nsDisplayCaret via a DisplayItemLayer to WebRenderCommands

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

::: layout/painting/nsDisplayList.cpp
@@ +4334,5 @@
> +  if (!frame) {
> +    return;
> +  }
> +  // Not sure how we do this now
> +  //NS_ASSERTION(frame == aForFrame, "We're referring different frame");

frame == mFrame

@@ +4347,5 @@
> +
> +  DrawTarget* dt = gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget();
> +  MOZ_ASSERT(dt);
> +  Rect devCaretRect =
> +    NSRectToSnappedRect(caretRect + ToReferenceFrame(), appUnitsPerDevPixel, *dt);

When snapping, we're using the transform for the DT to figure out what untransformed rect will map to a pixel snapped rect in device space.

The reference draw target doesn't have a useful transform, so we're not gaining anything by snapping here.

I think we can't reasonably know what the transform will be when this is actually rasterized into pixels, so the only solution would be to pass a 'please snap me' flag as part of the WR command, and have WR figure it out at draw time.

I'd just use NSRectToRect for now, add a TODO, and maybe file a bug for getting WR to understand snapping.
Attachment #8835166 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8834619 [details] [diff] [review]
Add support for a DisplayItemLayer generic type

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

::: gfx/layers/Layers.h
@@ +2279,4 @@
>  };
>  
>  /**
> + * A generic layer that references back to it's display item.

it's -> its

@@ +2288,5 @@
> + * The problem with this is that the whole pipeline would have to deal with more
> + * display items, which is slower.
> + *
> + * An alternative is to create a DisplayItemLayer, but the wrinkle with this is that
> + * it has a pointer to it's nsDisplayItem. Managing the lifetime is key as display items

it's -> its

@@ +2289,5 @@
> + * display items, which is slower.
> + *
> + * An alternative is to create a DisplayItemLayer, but the wrinkle with this is that
> + * it has a pointer to it's nsDisplayItem. Managing the lifetime is key as display items
> + * only live as long as a LayerTransaction. Layers are also recycled at each transaction.

I'd say "only live as long as their display list builder, which goes away at the end of the paint. Layers however are retained between paints."

@@ +2329,5 @@
> +  virtual void PrintInfo(std::stringstream& aStream, const char* aPrefix) override;
> +
> +  virtual void DumpPacket(layerscope::LayersPacket* aPacket, const void* aParent) override;
> +
> +  // READ COMMENT ABOVE TO ENSURE WE DON'T LEAK THIS

"to ensure this doesn't become a dangling pointer". We're not worried about leaks here, we're worried about use-after-free.
> We might need to figure out a solution for this. It's used for computing the changed region passed to MozAfterPaint, which the reftest harness uses.
> The 'normal' way of comparing display items is DLBI, maybe we can make FrameLayerBuilder run that for DisplayItemLayers and store it on the 'invalid region' for the layer.

We discussed this a bit in Toronto. Layer Invalidation happens in two cases:

1) Content side invalidation for inactive layers.
2) Compositor side for partial composition.

For (1), in WebRender, we shouldn't need inactive layers anymore. All WR DisplayItems should be an active layer and anything else should be assigned to a painted layer. (True except for filters - those will use existing layer types which already have inactive layer invalidation).

For (2), partial composition will need to be handled by webrender. Webrender should retain some of the command stream and figure out what changed to only paint the changed portion of the screen.

In regards to reftests, we should be able to tell reftests to snapshot when there are no pending paints. IIUC, this should happen if nothing will cause a paint. We should be able to check if another paint has been scheduled after a MozAfterPaint instead of detecting which region has been invalidated. With webrender, we don't need partial reftest paint support since we plan to paint the whole screen every frame and don't need invalidation. Since invalidation bugs would result in artifacts for invalidation reftests, and with webrender we'll paint the whole screen, the artifacts should not appear and the reftests should pass.

Does this work for you?
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Comment on attachment 8834619 [details] [diff] [review]
> ::: layout/painting/nsDisplayList.h
> @@ +1889,5 @@
> >  
> > +  /**
> > +   * Paint the item using the raw target.
> > +   */
> > +  virtual void Paint(DrawTarget* aDT,
> 
> Can we avoid adding a new Paint() override that items need to implement?
> 
> For WRLayerManager, I assume we want to serialize the mask separately, and
> for BasicLayerManager, take a look at BasicPaintedLayer::PaintThebes (the
> !IsRetained() path) for an example of how to handle the mask outside of this
> call.
> 
> We should just instantiate a new nsRenderingContext around the DrawTarget in
> the caller, and then use the old paint method.
> 
> Feel free to add a non-virtual Paint function that does the
> DrawTarget->nsRenderingContext wrapping and then calls the old one.
> 
> We really need to just get rid of nsRenderingContext so that this stops
> being a problem.


Here's something that does that. What I had to rework though was that the paint methods on nsDisplayItems require an nsDisplayListbuilder. We only have this information when we call EndTransaction from FrameLayerBuilder, so I had to pipe that through all the LayerManagers to take a nsDisplayListBuilder. Then in DisplayItemLayer, we can call the nsDisplayItem::Paint with the builder. Other than that, everything else is the same plus Markus' comments.
Attachment #8835817 - Flags: review?(matt.woodrow)
Fixed up to have a generic CreateDisplayItemLayer method. WR will pixel snap as long as the input rects are pixel whole. Thus we round to int for the rects now. Finally, also creates the rects relative to the parent layer since we had some positioning problems with carets before.
Attachment #8835166 - Attachment is obsolete: true
Attachment #8836373 - Flags: review+
Comment on attachment 8835817 [details] [diff] [review]
Add support for a DisplayItemLayer generic type v2

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

Can we make the nsDisplayListBuilder* a property of DisplayItemLayer and pass it in as part of SetDisplayItem?

Having display item classes defined in layers code is a bit sad, I'd really like to try contain it to the smallest area possible.

::: gfx/layers/basic/BasicDisplayItemLayer.cpp
@@ +57,5 @@
> +    }
> +
> +    AutoRestoreTransform autoRestoreTransform(aDT);
> +    Matrix transform = aDT->GetTransform();
> +    RefPtr<gfxContext> context = gfxContext::CreateOrNull(aDT, aDeviceOffset);

CreatePreservingTransformOrNull?
(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> Comment on attachment 8835817 [details] [diff] [review]
> Add support for a DisplayItemLayer generic type v2
> 
> Review of attachment 8835817 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we make the nsDisplayListBuilder* a property of DisplayItemLayer and
> pass it in as part of SetDisplayItem?
> 
> Having display item classes defined in layers code is a bit sad, I'd really
> like to try contain it to the smallest area possible.

Fixed.

> ::: gfx/layers/basic/BasicDisplayItemLayer.cpp
> @@ +57,5 @@
> > +    }
> > +
> > +    AutoRestoreTransform autoRestoreTransform(aDT);
> > +    Matrix transform = aDT->GetTransform();
> > +    RefPtr<gfxContext> context = gfxContext::CreateOrNull(aDT, aDeviceOffset);
> 
> CreatePreservingTransformOrNull?

I couldn't find a way to keep both aDeviceOffset & the transform, hence the CreateOrNull then setting the transform.
Attachment #8835817 - Attachment is obsolete: true
Attachment #8835817 - Flags: review?(matt.woodrow)
Attachment #8836785 - Flags: review?(matt.woodrow)
Attachment #8836785 - Flags: review?(matt.woodrow) → review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/48b29a84e4e5
Create a DisplayItemLayer type. r=mattwoodrow
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/b242c7944db3
Fixup. Make sure nsDisplayCaret doesn't return LAYER_INACTIVE. r=me
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/669d6b1f3031
Add a WebRenderDisplayItemLayer. r=mattwoodrow
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/886a61447287
Add WebRenderDisplayItemLayer build bustage. r=me
Flags: needinfo?(matt.woodrow)
Comment on attachment 8836785 [details] [diff] [review]
Add support for DisplayItemLayer generic type v3

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

::: gfx/layers/wr/WebRenderImageLayer.cpp
@@ +6,2 @@
>  #include "WebRenderLayersLogging.h"
> +#include "WebRenderImageLayer.h"

Why did you change the include order here? The .h file for a given .cpp should always be the first include (per style guide, and to make sure the .h file explicitly includes all the things it needs and doesn't accidentally pick them up from the .cpp's other includes).

I'm going to be undoing this change in the merge from m-c that I'll be landing soon, as part of resolving a conflict. Hopefully it doesn't break anything.
Target Milestone: --- → mozilla54
See Also: → 1402533
You need to log in before you can comment on or make changes to this bug.