Closed
Bug 1317862
Opened 8 years ago
Closed 7 years ago
Add a TextLayer
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: mattwoodrow, Assigned: mattwoodrow, NeedInfo)
References
Details
Attachments
(4 files, 1 obsolete file)
37.63 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
7.26 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
14.97 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
1.20 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
WebRender and advanced-layers both want a TextLayer, so we should add support for them.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8811088 -
Flags: review?(mstange)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8811089 -
Flags: review?(bas)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8811090 -
Flags: review?(mstange)
Assignee | ||
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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 6•8 years ago
|
||
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 7•8 years ago
|
||
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 8•8 years ago
|
||
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 9•8 years ago
|
||
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•8 years 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.
Updated•8 years ago
|
Attachment #8811089 -
Flags: review?(bas) → review+
Assignee | ||
Comment 11•7 years 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•7 years ago
|
||
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)
Comment 13•7 years ago
|
||
Hmm, I've not touched layout code for a couple of years, so, I think that you should look for another reviewer...
Assignee | ||
Updated•7 years ago
|
Attachment #8813477 -
Flags: review?(masayuki) → review?(jfkthame)
Comment 14•7 years ago
|
||
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...)
Assignee | ||
Updated•7 years ago
|
Attachment #8813477 -
Attachment is obsolete: true
Attachment #8813477 -
Flags: review?(jfkthame)
Comment 15•7 years 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•7 years 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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 17•7 years ago
|
||
It seems like this bloats nsDisplayText. I guess that was intentional?
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 18•7 years 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)
Comment 19•7 years ago
|
||
What ended up happening with the patch in Part 5?
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 20•7 years 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•7 years ago
|
Flags: needinfo?(matt.woodrow)
Comment 21•7 years ago
|
||
Ping?
You need to log in
before you can comment on or make changes to this bug.
Description
•