1422466 - Drop a copy from TextDrawTarget::FillGlyphs

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

After bug 1422414 the loop basically just does a memcpy. We can just skip it if we pretend the types are the same.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: PoC

We should try harder to ensure that the types are the same.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1422466. Drop a copy from TextDrawTarget::FillGlyphs.

We try hard to ensure that the glyph buffers are of the same type.

Review commit: https://reviewboard.mozilla.org/r/204840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204840/

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8933919 [details]
Bug 1422466. Drop a copy from TextDrawTarget::FillGlyphs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204840/diff/1-2/

Comment 4

[Aria Beingessner [:Gankra]]

Comment on attachment 8933919 [details]
Bug 1422466. Drop a copy from TextDrawTarget::FillGlyphs.

https://reviewboard.mozilla.org/r/204840/#review210398

::: layout/generic/TextDrawTarget.h:124
(Diff revision 2)
> -
> -    for (size_t i = 0; i < aBuffer.mNumGlyphs; i++) {
> -      wr::GlyphInstance& targetGlyph = glyphs[i];
> -      const gfx::Glyph& sourceGlyph = aBuffer.mGlyphs[i];
> -      targetGlyph.index = sourceGlyph.mIndex;
> -      targetGlyph.point = mSc.ToRelativeLayoutPoint(
> +
> +    // Compare gfx::Glyph and wr::GlyphInstance to make sure that they are
> +    // structurally equivalent to ensure that our cast above was ok
> +    static_assert(std::is_same<decltype(aBuffer.mGlyphs[0].mIndex), decltype(glyphs[0].index)>()
> +                  && std::is_same<decltype(aBuffer.mGlyphs[0].mPosition.x), decltype(glyphs[0].point.x)>()
> +                  && std::is_same<decltype(aBuffer.mGlyphs[0].mPosition.y), decltype(glyphs[0].point.y)>()

This is supposed to move the glyphs from global space to local space by applying the the stacking context's offset. Do you have a reason to believe this must always be 0?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Yeah, see https://bugzilla.mozilla.org/show_bug.cgi?id=1422414. The stacking context offset is always zero now (and has been for a little while).

Comment 6

[Aria Beingessner [:Gankra]]

Ah, great!

Comment 7

[Aria Beingessner [:Gankra]]

Comment on attachment 8933919 [details]
Bug 1422466. Drop a copy from TextDrawTarget::FillGlyphs.

https://reviewboard.mozilla.org/r/204840/#review210426

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/719224b9a2dd
Drop a copy from TextDrawTarget::FillGlyphs. r=Gankro

Comment 9

[Raul Gurzau (:RaulG)]

Backed out 1 changesets (bug 1422466) for build bustage on a CLOSED TREE

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=719224b9a2dd25180b4fe58d039dc91d9c579259

Backout: https://hg.mozilla.org/integration/autoland/rev/5a71560d58db074a628624d99f12d84e0d2d07b7

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=149443565&repo=autoland&lineNumber=17299

03:35:32     INFO -  z:/build/build/src/gfx/layers/wr/WebRenderBridgeChild.cpp(297): error C2039: 'IsEmpty': is not a member of 'mozilla::Range<const mozilla::wr::GlyphInstance>'
03:35:32     INFO -  z:\build\build\src\obj-firefox\dist\include\mozilla/webrender/WebRenderAPI.h(393): note: see declaration of 'mozilla::Range<const mozilla::wr::GlyphInstance>'
03:35:32     INFO -  z:/build/build/src/gfx/layers/wr/WebRenderBridgeChild.cpp(297): error C2955: 'mozilla::detail::AssertionConditionType': use of class template requires template argument list
03:35:32     INFO -  z:\build\build\src\obj-firefox\dist\include\mozilla/Assertions.h(396): note: see declaration of 'mozilla::detail::AssertionConditionType'
03:35:32     INFO -  z:/build/build/src/gfx/layers/wr/WebRenderBridgeChild.cpp(297): error C2057: expected constant expression
03:35:32     INFO -  z:/build/build/src/config/rules.mk:1028: recipe for target 'Unified_cpp_gfx_layers11.obj' failed
03:35:32     INFO -  mozmake.EXE[4]: *** [Unified_cpp_gfx_layers11.obj] Error 2
03:35:32     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/gfx/layers'
03:35:32     INFO -  mozmake.EXE[4]: *** Waiting for unfinished jobs....
03:35:32     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/dom/base'
03:35:32     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/dom/base'

Comment 10

[Cristina Coroiu [:ccoroiu]]

Backed out 1 changesets (bug 1422466) for bustage on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7a2629417a4707f15b8c35c8fccecee530904e73 

Backout: https://hg.mozilla.org/integration/autoland/rev/8295eb601d27dbb4aaa15d879bc9795a200789ee 

Log: https://hg.mozilla.org/integration/autoland/rev/8295eb601d27dbb4aaa15d879bc9795a200789ee

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment 10 is really for bug 1422662, not this bug.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7edd58cc444f
Drop a copy from TextDrawTarget::FillGlyphs. r=Gankro

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7edd58cc444f