Closed Bug 1341101 Opened 7 years ago Closed 7 years ago

Convert nsDisplayBackgroundImage gradients into WebRender DisplayItems

Categories

(Core :: Graphics: WebRender, defect)

Product:
Core
Component:
Graphics: WebRender
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(6 files)

Bug 1341101 part 1 - Move nsCSSRendering::PaintGradient into its own file
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1341101 part 2 - Create a nsCSSGradientRenderer for collecting gradient parameters
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1341101 part 3 - Support building WR gradients in nsCSSGradientRenderer
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1341101 part 4 - Specify a nsRenderingContext for nsCSSRendering::PaintStyleImageLayer
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1341101 part 5 - Support building WR DisplayItems for BackgroundImageLayers that are gradients
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1341101 part 6 - Build WR gradients for nsDisplayBackgroundImage when supported
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details 
I began to take a look at this last week. It seems like it should be possible for most cases. The code is mostly implemented in nsCSSRendering and can probably be mostly shared between cases but will require some refactoring.

I have a working prototype for just gradients, but it will need some clean up work to share code with nsCSSRendering. I won't be able to work on it this week so leaving unassigned.
Blocks: 1342343
Ryan, may I take this bug if you're not working on this?
Flags: needinfo?(rhunt)
I'm sorry but I just started working on this again after being on PTO!
Flags: needinfo?(rhunt)
Assignee: nobody → rhunt
Summary: Convert nsDisplayBackgroundImage into WebRender DisplayItems → Convert nsDisplayBackgroundImage gradients into WebRender DisplayItems
Feel free to assign review to someone else with more time or who knows the code better.

The overall goal of these patches is to convert nsDisplayBackgroundImage's that are gradients into WR DisplayItems.

To do this I moved the gradient painting code into a new file, nsCSSRenderingGradients.h, and created a nsCSSGradientRenderer class. The purpose of the class is to share the layout-ish painting tasks with the existing paint backend.

The important general values that gradients will need in both backends are generated and stored in the class and then used to paint.

Two changes were made to make that possible. The first was moving ResolveMidpoints() to happen sooner than it did before. This was done because it could and for sharing between backends. The second was moving this code [1] to happen sooner. This was done to simplify the code that happens before and handles this same case. By making them all the same earlier, we shouldn't need to to a delta check again.

[1] http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/painting/nsCSSRendering.cpp#3231

I then created a duplicate of nsCSSRendering::PaintStyleImageLayer that will build display items instead of paint. I stripped out clipping code and background color drawing code, which might need to be added in later. This functions delegates work to nsImageRenderer which is also converted to build display lists. Eventually it should handle style images that are not gradients, like images.

Finally, nsDisplayBackgroundImage is hooked up and the new behavior is put behind a pref.

I've tested this out and it is able to paint gradients. Linear and radial. Not ellipses yet. Tiling also needs to be improved. Clipping also doesn't seem to happen for scroll frames or border corners.
Also the first patch is showing up kind of weird in mozreview.

To make nsCSSRenderingGradients.cpp and keep blame/history, I did a |hg cp| of nsCSSRendering.cpp and removed everything that wasn't gradients. And then removed the gradient stuff from nsCSSRendering.cpp.

Looking at the raw diff might be better?
Comment on attachment 8847480 [details]
Bug 1341101 part 1 - Move nsCSSRendering::PaintGradient into its own file

https://reviewboard.mozilla.org/r/120456/#review122766

The diff is basically unreviewable so I'll assume it basically just does what it claims to do and nothing more. Are there any details that you think I should look at?
Attachment #8847480 - Flags: review?(jmuizelaar) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> Comment on attachment 8847480 [details]
> Bug 1341101 part 1 - Move nsCSSRendering::PaintGradient into its own file
> 
> https://reviewboard.mozilla.org/r/120456/#review122766
> 
> The diff is basically unreviewable so I'll assume it basically just does
> what it claims to do and nothing more. Are there any details that you think
> I should look at?

I meant the mozreview diff. I'll try looking at the actual diff.
It would also be good to get an additional layout reviewer to look at this. Maybe mattwoodrow? If not him, perhaps he can delegate.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> Comment on attachment 8847480 [details]
> Bug 1341101 part 1 - Move nsCSSRendering::PaintGradient into its own file
> 
> https://reviewboard.mozilla.org/r/120456/#review122766
> 
> The diff is basically unreviewable so I'll assume it basically just does
> what it claims to do and nothing more. Are there any details that you think
> I should look at?

The actual diff is easier to follow. I think mozreview's heuristics for tracking lines that move is getting killed here.

There should be no actual code additions or removals, just moves. So there shouldn't be anything tricky to look at.
Attachment #8847481 - Flags: review?(matt.woodrow)
Attachment #8847483 - Flags: review?(matt.woodrow)
Attachment #8847484 - Flags: review?(matt.woodrow)
Matt, I've tagged you on the patches that I think are significant, but feel free to review them all or delegate. Thanks!
Comment on attachment 8847481 [details]
Bug 1341101 part 2 - Create a nsCSSGradientRenderer for collecting gradient parameters

https://reviewboard.mozilla.org/r/120458/#review123298

::: commit-message-47d6d:1
(Diff revision 1)
> +Bug 1341101 part 2 - Create a nsCSSGradientRenderer r?jrmuizel

Maybe mention that this is refactoring the existing code so that it collects gradient parameters into a renderer object and then does the rendering, it doesn't actually do anything 'new'.
Attachment #8847481 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8847482 [details]
Bug 1341101 part 3 - Support building WR gradients in nsCSSGradientRenderer

https://reviewboard.mozilla.org/r/120460/#review123300

::: layout/painting/nsCSSRenderingGradients.h:59
(Diff revision 1)
>  
>    void Paint(nsRenderingContext& aRenderingContext,
>               const nsRect& aDirtyRect,
>               float aOpacity = 1.0);
>  
> +  void BuildDisplayItems(wr::DisplayListBuilder& aBuilder,

Could you please call this BuildWebRenderDisplayItems (or something along those lines).

Display Item is overload now, so it's worth being explicit.
Comment on attachment 8847483 [details]
Bug 1341101 part 4 - Specify a nsRenderingContext for nsCSSRendering::PaintStyleImageLayer

https://reviewboard.mozilla.org/r/120462/#review123302
Attachment #8847483 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8847484 [details]
Bug 1341101 part 5 - Support building WR DisplayItems for BackgroundImageLayers that are gradients

https://reviewboard.mozilla.org/r/120464/#review123304

Looking really good! I'm excited to see this working.

::: layout/painting/nsCSSRendering.cpp:1974
(Diff revision 3)
> +    }
> +    backgroundSC = aFrame->StyleContext();
> +  }
> +  const nsStyleImageLayers& layers = backgroundSC->StyleBackground()->mImage;
> +
> +  if (aLayer < 0) {

We should just not support aLayer being negative, and only allow this for a specified layer.

::: layout/painting/nsCSSRendering.cpp:1994
(Diff revision 3)
> +nsCSSRendering::BuildDisplayItemsForStyleImageLayer(const PaintBGParams& aParams,
> +                                                    mozilla::wr::DisplayListBuilder& aBuilder,
> +                                                    mozilla::layers::WebRenderDisplayItemLayer* aLayer)
> +{
> +  NS_PRECONDITION(aParams.frame,
> +                  "Frame is expected to be provided to PaintBackground");

Assert CanBuild(WR)DisplayItemForStyleImageLayer?

::: layout/painting/nsCSSRendering.cpp:2776
(Diff revision 3)
> +                                     skipSides, &aBorder);
> +
> +  uint32_t count = drawAllLayers
> +    ? layers.mImageCount                  // iterate all image layers.
> +    : layers.mImageCount - aParams.layer; // iterate from the bottom layer to
> +                                          // the 'aParams.layer-th' layer.

Why do we need to iterate multiple layers here?

We should have created a separate nsDisplayBackgroundImage for each layer containing an image, so I'd have thought we only need to issue WR DL commands for that.

::: layout/painting/nsCSSRendering.cpp:2794
(Diff revision 3)
> +    nsBackgroundLayerState state =
> +      PrepareImageLayer(&aParams.presCtx, aParams.frame,
> +                        aParams.paintFlags, paintBorderArea,
> +                        clipState.mBGClipArea, layer, nullptr);
> +    if (!state.mFillArea.IsEmpty()) {
> +      state.mImageRenderer.DrawLayer(&aParams.presCtx,

Can we include WR specific naming in this function too please :)
Comment on attachment 8847484 [details]
Bug 1341101 part 5 - Support building WR DisplayItems for BackgroundImageLayers that are gradients

https://reviewboard.mozilla.org/r/120464/#review123482

::: layout/painting/nsCSSRendering.cpp:2776
(Diff revision 3)
> +                                     skipSides, &aBorder);
> +
> +  uint32_t count = drawAllLayers
> +    ? layers.mImageCount                  // iterate all image layers.
> +    : layers.mImageCount - aParams.layer; // iterate from the bottom layer to
> +                                          // the 'aParams.layer-th' layer.

You're right, for use by nsDisplayBackgroundImage we will create WR DL commands one layer at a time.

PaintStyleImageLayer is only used to paint all layers for table painting. Is table painting something that will issue WR DL commands in the near future?

If not, then yeah this code isn't needed.
(In reply to Ryan Hunt [:rhunt] from comment #24)
> 
> You're right, for use by nsDisplayBackgroundImage we will create WR DL
> commands one layer at a time.
> 
> PaintStyleImageLayer is only used to paint all layers for table painting. Is
> table painting something that will issue WR DL commands in the near future?
> 
> If not, then yeah this code isn't needed.

We've had a plan for a long time to transition table painting to using separate display items per frame, so the existing background drawing code will 'just work' - see bug 929484.

I'm not aware of anyone actually working on that at the moment, or on converting the 'old style' table painting code to WR.
(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> (In reply to Ryan Hunt [:rhunt] from comment #24)
> > 
> > You're right, for use by nsDisplayBackgroundImage we will create WR DL
> > commands one layer at a time.
> > 
> > PaintStyleImageLayer is only used to paint all layers for table painting. Is
> > table painting something that will issue WR DL commands in the near future?
> > 
> > If not, then yeah this code isn't needed.
> 
> We've had a plan for a long time to transition table painting to using
> separate display items per frame, so the existing background drawing code
> will 'just work' - see bug 929484.
> 
> I'm not aware of anyone actually working on that at the moment, or on
> converting the 'old style' table painting code to WR.

Ah, I'll just simplify the code for now then. It's not hard to go back.
Comment on attachment 8847484 [details]
Bug 1341101 part 5 - Support building WR DisplayItems for BackgroundImageLayers that are gradients

https://reviewboard.mozilla.org/r/120464/#review123304

> We should just not support aLayer being negative, and only allow this for a specified layer.

This has been fixed.

> Assert CanBuild(WR)DisplayItemForStyleImageLayer?

I added this in BuildWebRenderDisplayItemsForStyleImageLayerSC, which this function always calls.

> Can we include WR specific naming in this function too please :)

I've changed DisplayItem to WebRenderDisplayItem for all these cases. It's a bit long, but it's good to be specific.
Comment on attachment 8847482 [details]
Bug 1341101 part 3 - Support building WR gradients in nsCSSGradientRenderer

https://reviewboard.mozilla.org/r/120460/#review124694
Attachment #8847482 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8847485 [details]
Bug 1341101 part 6 - Build WR gradients for nsDisplayBackgroundImage when supported

https://reviewboard.mozilla.org/r/120466/#review125414
Attachment #8847485 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8847484 [details]
Bug 1341101 part 5 - Support building WR DisplayItems for BackgroundImageLayers that are gradients

https://reviewboard.mozilla.org/r/120464/#review126500

::: layout/painting/nsCSSRendering.cpp:1949
(Diff revision 5)
> +nsCSSRendering::CanBuildWebRenderDisplayItemsForStyleImageLayer(nsPresContext& aPresCtx,
> +                                                                nsIFrame *aFrame,
> +                                                                const nsStyleBackground* aBackgroundStyle,
> +                                                                int32_t aLayer)
> +{
> +  MOZ_ASSERT(aFrame && aLayer >= 0);

Might as well assert that aLayer < numlayers too.
Attachment #8847484 - Flags: review?(matt.woodrow) → review+
See Also: → 1347469
Thanks Matt and Jeff for the reviews!

I've rebased the patches and started a try run. The rebased patches are also on mozreview now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78a27533306b4ae11a8e9115362f17e6d76e8208
Blocks: 1351242
Try run looks green. I'm going to review the patch and probably land later tonight.
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d4b0a9b2d35
part 1 - Move nsCSSRendering::PaintGradient into its own file r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d82968c4e292
part 2 - Create a nsCSSGradientRenderer for collecting gradient parameters r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5fef49b82c
part 3 - Support building WR gradients in nsCSSGradientRenderer r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/88a7b38645a0
part 4 - Specify a nsRenderingContext for nsCSSRendering::PaintStyleImageLayer r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/65b02add807c
part 5 - Support building WR DisplayItems for BackgroundImageLayers that are gradients r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ccd13025da4
part 6 - Build WR gradients for nsDisplayBackgroundImage when supported r=jrmuizel
Depends on: 1352234
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: