Status

()

Core
Graphics: Layers
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow, NeedInfo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
WebRender and advanced-layers both want a TextLayer, so we should add support for them.
(Assignee)

Comment 1

a year ago
Created attachment 8811088 [details] [diff] [review]
Part 1: Add a TextLayer class and basic support for them
Attachment #8811088 - Flags: review?(mstange)
(Assignee)

Comment 2

a year ago
Created attachment 8811089 [details] [diff] [review]
Part 2: Add a way to query DT recordings for Glyphs
Attachment #8811089 - Flags: review?(bas)
(Assignee)

Comment 3

a year ago
Created attachment 8811090 [details] [diff] [review]
Part 3: Build TextLayers if enabled
Attachment #8811090 - Flags: review?(mstange)
(Assignee)

Comment 4

a year ago
Created attachment 8811091 [details] [diff] [review]
Part 4: Remove unnecessary clip

This is just clipping to the visible region, so shouldn't be required for correct rendering. It's not obvious that this would actually help us on any backend.
Attachment #8811091 - Flags: review?(mstange)
Comment on attachment 8811088 [details] [diff] [review]
Part 1: Add a TextLayer class and basic support for them

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

::: gfx/layers/basic/BasicTextLayer.cpp
@@ +56,5 @@
> +    Rect snapped(mBounds.x, mBounds.y, mBounds.width, mBounds.height);
> +    MaybeSnapToDevicePixels(snapped, *aDT, true);
> +
> +    // Clip drawing in case we're using (unbounded) operator source.
> +    aDT->PushClipRect(snapped);

I don't think we can have operator source here. We only use it for ColorLayers (for Mac vibrancy).

@@ +57,5 @@
> +    MaybeSnapToDevicePixels(snapped, *aDT, true);
> +
> +    // Clip drawing in case we're using (unbounded) operator source.
> +    aDT->PushClipRect(snapped);
> +    aDT->SetPermitSubpixelAA(false);

Why? Maybe add a comment.

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +283,5 @@
>    // not all edits require an update to the hit testing tree
>    bool updateHitTestingTree = false;
>  
>    for (EditArray::index_type i = 0; i < cset.Length(); ++i) {
> +    Edit& edit = cset[i];

Why can't this stay const? Because you move the glyphs out of it?

::: gfx/layers/ipc/LayersMessages.ipdlh
@@ +263,5 @@
> +  LayerColor color;
> +  Glyph[] glyphs;
> +};
> +
> +// XXX - Bas - Hack warning!

What's the hack? The fact that you have an array of arrays of glyphs instead of just an array of glyphs?
Attachment #8811088 - Flags: review?(mstange) → review+
Comment on attachment 8811089 [details] [diff] [review]
Part 2: Add a way to query DT recordings for Glyphs

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

Is the plan still to explore the other options we've written down https://public.etherpad-mozilla.org/p/wr-plan , and you just want to land what you have now? Or did you start exploring the other options and run into problems?

::: gfx/2d/2D.h
@@ +1278,5 @@
>  
>  class DrawTargetCapture : public DrawTarget
>  {
> +public:
> +  virtual bool ContainsOnlyColoredGlyphs(RefPtr<ScaledFont>& aScaledFont, Color& aColor, std::vector<Glyph>& aGlyphs) = 0;

I think this would be a better place to put the IsCaptureDT() override than DrawTargetCaptureImpl.

Also, this line is a little long.
Comment on attachment 8811090 [details] [diff] [review]
Part 3: Build TextLayers if enabled

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

::: layout/generic/nsTextFrame.cpp
@@ +4997,5 @@
> +    RenderToContext(captureCtx, aBuilder);
> +
> +    // TODO: Ideally we'd re-use captureCtx in Paint() if we couldn't build
> +    // a layer here. We have to deal with the problem that the ScreenReferenceDrawTarget
> +    // might not be compatible with the DT used for layer rendering.

What's the problem here, and in what cases can it happen?

@@ +5007,5 @@
> +      mFont = nullptr;
> +      mGlyphs.Clear();
> +    } else {
> +      g->glyphs().SetLength(glyphs.size());
> +      memcpy(g->glyphs().begin(), glyphs.data(), sizeof(gfx::Glyph) * glyphs.size());

PodCopy please

@@ +5061,5 @@
> +          LayoutDeviceIntRect::FromAppUnitsToOutside(GetBounds(aBuilder, &dummy), A2D);
> +  layer->SetBounds(IntRect(destBounds.x, destBounds.y, destBounds.width, destBounds.height));
> +
> +  layer->SetBaseTransform(gfx::Matrix4x4::Translation(aContainerParameters.mOffset.x,
> +                                                      aContainerParameters.mOffset.y, 0));

What about aContainerParameters.mX/YScale? Or is that not needed?
Attachment #8811090 - Flags: review?(mstange) → review+
Comment on attachment 8811091 [details] [diff] [review]
Part 4: Remove unnecessary clip

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

If this change passes reftests then it sounds good to me.
Attachment #8811091 - Flags: review?(mstange) → review+
Comment on attachment 8811088 [details] [diff] [review]
Part 1: Add a TextLayer class and basic support for them

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

::: gfx/layers/Layers.h
@@ +405,5 @@
>    /**
>     * CONSTRUCTION PHASE ONLY
> +   * Create a ColorLayer for this manager's layer tree.
> +   */
> +  virtual already_AddRefed<TextLayer> CreateTextLayer() = 0;

s/ColorLayer/TextLayer/ in comment
(Assignee)

Comment 10

a year ago
(In reply to Markus Stange [:mstange] from comment #6)
> Comment on attachment 8811089 [details] [diff] [review]
> Part 2: Add a way to query DT recordings for Glyphs
> 
> Review of attachment 8811089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is the plan still to explore the other options we've written down
> https://public.etherpad-mozilla.org/p/wr-plan , and you just want to land
> what you have now? Or did you start exploring the other options and run into
> problems?

I had a play with the first option (creating a CheckTextLayerFeasibility function) and it was a nightmare. There's no way we'd ever be able to keep it in sync with the drawing path.

The third option (a wrapper DT that lazily creates the real DT if we hit anything that isn't FillGlyphs) suffers from the same problem that we have with trying to reuse the recording. At the time we do the recording (during DL construction) we don't know what type of DT we would want to actually create for painting.

> 
> ::: gfx/layers/ipc/LayerTransactionParent.cpp
> @@ +283,5 @@
> >    // not all edits require an update to the hit testing tree
> >    bool updateHitTestingTree = false;
> >  
> >    for (EditArray::index_type i = 0; i < cset.Length(); ++i) {
> > +    Edit& edit = cset[i];
> 
> Why can't this stay const? Because you move the glyphs out of it?

Yeah indeed, trying to avoid copying the glyph array (it showed up in profiles a bit).

> 
> ::: gfx/layers/ipc/LayersMessages.ipdlh
> @@ +263,5 @@
> > +  LayerColor color;
> > +  Glyph[] glyphs;
> > +};
> > +
> > +// XXX - Bas - Hack warning!
> 
> What's the hack? The fact that you have an array of arrays of glyphs instead
> of just an array of glyphs?

Passing the raw ScaledFont pointer across IPDL. Not great, and will break horribly if used with e10s.

 
> ::: layout/generic/nsTextFrame.cpp
> @@ +4997,5 @@
> > +    RenderToContext(captureCtx, aBuilder);
> > +
> > +    // TODO: Ideally we'd re-use captureCtx in Paint() if we couldn't build
> > +    // a layer here. We have to deal with the problem that the ScreenReferenceDrawTarget
> > +    // might not be compatible with the DT used for layer rendering.
> 
> What's the problem here, and in what cases can it happen?

When we create the capture (during DL construction), we only have the reference DT, and we don't really know
if this will be the same type as what gets used for a PaintedLayer if we decide not to layerize.

DrawTargetCapture uses the DT to create temporary surfaces, path objects and gradient stop objects, all of which might happen in the cases where we can't layerize (svg glyphs in particular), and those object won't work if the backends are incompatible.

We could probably try guess (using the reference DT, or checking the content backend on gfxPlatform), and then confirm that the backends match during Paint(). That would probably work in most (all?) cases, but it feels pretty ugly.

> 
> @@ +5061,5 @@
> > +          LayoutDeviceIntRect::FromAppUnitsToOutside(GetBounds(aBuilder, &dummy), A2D);
> > +  layer->SetBounds(IntRect(destBounds.x, destBounds.y, destBounds.width, destBounds.height));
> > +
> > +  layer->SetBaseTransform(gfx::Matrix4x4::Translation(aContainerParameters.mOffset.x,
> > +                                                      aContainerParameters.mOffset.y, 0));
> 
> What about aContainerParameters.mX/YScale? Or is that not needed?

It probably is needed, I feel like I copied this from somewhere else though, will have a look.
Attachment #8811089 - Flags: review?(bas) → review+
(Assignee)

Comment 11

a year ago
(In reply to Markus Stange [:mstange] from comment #7)
> 
> @@ +5061,5 @@
> > +          LayoutDeviceIntRect::FromAppUnitsToOutside(GetBounds(aBuilder, &dummy), A2D);
> > +  layer->SetBounds(IntRect(destBounds.x, destBounds.y, destBounds.width, destBounds.height));
> > +
> > +  layer->SetBaseTransform(gfx::Matrix4x4::Translation(aContainerParameters.mOffset.x,
> > +                                                      aContainerParameters.mOffset.y, 0));
> 
> What about aContainerParameters.mX/YScale? Or is that not needed?

FrameLayerBuilder deals with this, just after calling BuildLayer, so we're good.
(Assignee)

Comment 12

a year ago
Created attachment 8813477 [details] [diff] [review]
Part 5: Don't paint text frames that haven't been reflowed

This is the reason that the reftest failed, we just also happened to clip text rendering to the 'visible' region which hid most of the fact that we drew the text in entirely the wrong place.

Part 4 of this patch queue removes the clip (since it shouldn't be necessary) and makes the test fail much more visibly (you can see the entire text content, higher than where it is supposed to be).

Drawing the text in the wrong place breaks invalidation, since we don't know to invalidate the pixels that were drawn in the wrong place.

This just copies the assertion to be early (so the test still asserts), but doesn't paint (so the incorrect pixels are gone).

Would still be nice to fix this properly one day.
Attachment #8813477 - Flags: review?(masayuki)
Hmm, I've not touched layout code for a couple of years, so, I think that you should look for another reviewer...
(Assignee)

Updated

a year ago
Attachment #8813477 - Flags: review?(masayuki) → review?(jfkthame)
Comment on attachment 8813477 [details] [diff] [review]
Part 5: Don't paint text frames that haven't been reflowed

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

::: layout/generic/nsTextFrame.cpp
@@ +6860,5 @@
>  
> +  if ((GetStateBits() & NS_FRAME_FIRST_REFLOW)) {
> +    NS_ERROR("Can only call this on frames that have been reflowed");
> +    return;
> +  }

Could/should we go one better and complain if NS_FRAME_IS_DIRTY? Painting a frame that needs reflow sounds like it's not really a good idea... or are there cases where that makes sense?

(I'm reminded of discussion in bug 1305130, where the original WIP would probably also have caused an assertion here...)
Blocks: 1311790
(Assignee)

Updated

a year ago
Attachment #8813477 - Attachment is obsolete: true
Attachment #8813477 - Flags: review?(jfkthame)

Comment 15

a year ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d4190eaaf8
Part 1: Add TextLayer class and basic support for them. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e55a4cc72d0
Part 2: Add a way to query DT recordings for Glyphs. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e343bcb231
Part 3: Build TextLayers if enabled. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/6888e7fc8a9c
Part 4: Remove unnecessary clip. r=mstange

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f2d4190eaaf8
https://hg.mozilla.org/mozilla-central/rev/7e55a4cc72d0
https://hg.mozilla.org/mozilla-central/rev/92e343bcb231
https://hg.mozilla.org/mozilla-central/rev/6888e7fc8a9c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
It seems like this bloats nsDisplayText. I guess that was intentional?
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 18

a year ago
Not really intentional, but I didn't think it would matter much.

We could move the decision out to the caller, and create a derived class of nsDisplayText so that we only add the extra space when we're going to use it.
Flags: needinfo?(matt.woodrow)
What ended up happening with the patch in Part 5?
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 20

10 months ago
That's a very good question. It looks like I only removed the extra clip when recording so it wasn't a problem for existing tests, but we probably still want to fix this properly.
Flags: needinfo?(matt.woodrow)
(Assignee)

Updated

10 months ago
Flags: needinfo?(matt.woodrow)
Ping?
See Also: → bug 1402533
You need to log in before you can comment on or make changes to this bug.