1231550 - Avoid exposing gfxContext to lots of text- and font-related functions

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Nicholas Nethercote [inactive]]

SetupCairoFont() is a leaf function that gets passed a |gfxContext*|. In all
implementations it only calls gfxContext::GetCairo(). We can instead pass in a
|cairo_t*| obtained with GetCairo() higher up in the call graph (in some cases,
quite a lot higher up).

This helps with gfxContext-to-DrawTarget conversions, because it reduces the
number of functions that are exposed to a gfxContext.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Pass a cairo_t* instead of a gfxContext* to various font functions

The only arguable downside I can see with this patch is that, because we call
GetCairo() earlier in most places, it might cause us to call GetCairo() in
cases where previously we didn't. But I don't think it's expensive and so this
shouldn't be a problem?

Comment 2

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cedf1d0aba94

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Pass a cairo_t* instead of a gfxContext* to various font functions

SetupCairoFont() is a leaf function that gets passed a |gfxContext*|. In all
implementations it only calls gfxContext::GetCairo(). We can instead pass in a
|cairo_t*| obtained with GetCairo() higher up in the call graph (in some cases,
quite a lot higher up).

This helps with gfxContext-to-DrawTarget conversions, because it reduces the
number of functions that are exposed to a gfxContext.

This patch also removes the |gfxContextMatrixAutoSaveRestore| in
gfxFont::SetupGlyphExtents() because it was unnecessary.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Pass a cairo_t* instead of a gfxContext* to various font functions

SetupCairoFont() is a leaf function that gets passed a |gfxContext*|. In all
implementations it only calls gfxContext::GetCairo(). We can instead pass in a
|cairo_t*| obtained with GetCairo() higher up in the call graph (in some cases,
quite a lot higher up).

This helps with gfxContext-to-DrawTarget conversions, because it reduces the
number of functions that are exposed to a gfxContext.

This patch also removes the |gfxContextMatrixAutoSaveRestore| in
gfxFont::SetupGlyphExtents() because it was unnecessary.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Moz2Dify gfxFont::CalcXScale()

This boils down to these two lines being equivalent:

  Size t = aContext->UserToDevice(Size(1.0, 0.0));
  Size t = aDrawTarget->GetTransform() * Size(1.0, 0.0);

The rest is just plumbing.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Introduce ShapeTextParams

A lot of text-related functions that take a |gfxContext*| only actually use a
DrawTarget and a reference |cairo_t*|. This patch introduces ShapeTextParams, a
new type that encapsulates those two things. This requires moving
GetRoundOffsetsToPixel() out of |gfxContext| and passing it a
|ShapeTextParams|.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove unused argument from SetPotentialLineBreaks()

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 5, INCOMPLETE) - Use ShapeTextParams instead of gfxContext wherever possible

This is already a huge patch and it doesn't even compile yet. There are plenty
more functions that can be converted.

I'm on the fence about this whole approach. It's good that it significantly
reduces the amount of code exposed to gfxContext, which seems like it can only
help converting gfxContext uses to DrawTarget. But I also wonder if it's just
rearranging furniture. Opinions one way or the other would be welcome :)

One thing worth noting is that ShapeTextParams needs a DrawTarget member only
for a single case: gfxGDIFont::GetGlyphWidth(). If it wasn't for that we could
instead pass a |cairo_t*| for the reference cairo instead (like in part 1
above).

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> This helps with gfxContext-to-DrawTarget conversions, because it reduces the
> number of functions that are exposed to a gfxContext.

You might want to confirm that this is actually actually the case.  It sounds to me as though this might hurt the conversion since I suspect we want to convert the things that use cairo_t into using APIs on DrawTarget.  I'm not an expert in this area, though.

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> Created attachment 8697114 [details] [diff] [review]
> Pass a cairo_t* instead of a gfxContext* to various font functions
> 
> The only arguable downside I can see with this patch is that, because we call
> GetCairo() earlier in most places, it might cause us to call GetCairo() in
> cases where previously we didn't. But I don't think it's expensive and so
> this
> shouldn't be a problem?

Hmm. I'd like to think about this a bit... GetCairo() might be somewhat expensive in the case where the context's backend type isn't cairo. And depending on the font backend, we may or may not really need the cairo at all -- I don't think it should be necessary for Core Text, or for DirectWrite using natural widths.

I wonder if we could pass a DrawTarget into the font functions instead, and then let them extract the cairo from that only if needed.

Comment 12

[Nicholas Nethercote [inactive]]

> Hmm. I'd like to think about this a bit... GetCairo() might be somewhat
> expensive in the case where the context's backend type isn't cairo.

Another option would be to create a wrapper type for gfxContext that restricts you so you can only look at its mDT and its mRefCairo. Then the GetCairo() calls could occur at the same time as they currently do but it would also be clear which parts of the code don't need a full gfxContext. A bit of a hack, though.


> And depending on the font backend, we may or may not really need the cairo at
> all -- I don't think it should be necessary for Core Text, or for
> DirectWrite using natural widths.

But as long as any backend needs the cairo, and all backends are using the same interface, we're stuck with the problem, correct?


> I wonder if we could pass a DrawTarget into the font functions instead, and
> then let them extract the cairo from that only if needed.

I would love it if we could do that. It would make things a lot simpler to pass a DrawTarget everywhere instead of the new ShapeTextParams, which contains a DrawTarget and a cairo_t.

Here's what GetCairo() looks like:

> cairo_t *
> gfxContext::GetCairo()
> {
>   if (mDT->GetBackendType() == BackendType::CAIRO) {
>     cairo_t *ctx =
>       (cairo_t*)mDT->GetNativeSurface(NativeSurfaceType::CAIRO_CONTEXT);
>     if (ctx) {
>       return ctx;
>     }
>   }
> 
>   if (mRefCairo) {
>     // Set transform!
>     return mRefCairo;
>   }
> 
>   mRefCairo = cairo_create(gfxPlatform::GetPlatform()->ScreenReferenceSurface()->CairoSurface()); 
> 
>   return mRefCairo;
> }

If we hit the first |return| statement then the DrawTarget alone is enough. It's the second and third |return| statements, involving mRefCairo, that complicate things. Is there some way we could get rid of mRefCairo? Or could we store a pointer to it in the DrawTarget's user data?

I need a better understanding of this stuff: what operations are still done by Cairo, what operations are done by Moz2D but with a Cairo backend, and what operations are done by Moz2D without any Cairo involvement.

Comment 13

[Nicholas Nethercote [inactive]]

At this point I think parts 2 and 4 are definitely worth taking, and parts 1, 3 and 5 are questionable.

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to Nicholas Nethercote [:njn] from comment #12)
> If we hit the first |return| statement then the DrawTarget alone is enough.
> It's the second and third |return| statements, involving mRefCairo, that
> complicate things. Is there some way we could get rid of mRefCairo? Or could
> we store a pointer to it in the DrawTarget's user data?

(Huh.... I thought I'd commented on this already, but must have failed to post it, sorry.)

Yes, I believe mRefCairo exists only to cache the result of cairo_create(gfxPlatform::GetPlatform()->ScreenReferenceSurface()->CairoSurface()), so ISTM we could have something like a GetCairoFromDrawTarget() helper function that encapsulates the same idea... get the cairo directly from the DT if it's cairo-backed; if not, check for one cached in the DT's userdata; if that's not present, then create it from ScreenReferenceSurface() and cache it there for next time.

Then the font backends can just call GetCairoFromDrawTarget() if they need a cairo context, where they currently use gfxContext::GetCairo. And I think we can probably eliminate this entirely from some of the backends, but that can be a separate followup.

Comment 15

[Nicholas Nethercote [inactive]]

> so ISTM we could have something like a
> GetCairoFromDrawTarget() helper function that encapsulates the same idea...
> get the cairo directly from the DT if it's cairo-backed; if not, check for
> one cached in the DT's userdata; if that's not present, then create it from
> ScreenReferenceSurface() and cache it there for next time.

I created bug 1232576 to do this part.

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: Use DrawTarget instead of gfxContext and/or nsRenderingContext in many places in font/text code

Apologies for the monster patch. Take your time for the review :)

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: Use DrawTarget instead of gfxContext and/or nsRenderingContext in many places in font/text code

(Updated gfxWordCacheTest.cpp, which is NPOTB.)

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8699696 [details] [diff] [review]
Use DrawTarget instead of gfxContext and/or nsRenderingContext in many places in font/text code

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

Looks fine, afaics.

::: layout/generic/MathMLTextRunFactory.cpp
@@ +528,5 @@
>  #define TT_DTLS TRUETYPE_TAG('d', 't', 'l', 's')
>  
>  void
>  MathMLTextRunFactory::RebuildTextRun(nsTransformedTextRun* aTextRun,
> +                                     mozilla::gfx::DrawTarget* aRefDrawTarget,

No need for the "mozilla::" here, there's a "using namespace..." at the top of the file.

::: layout/generic/TextOverflow.cpp
@@ +33,2 @@
>  public:
> +  typedef mozilla::gfx::DrawTarget DrawTarget;

"mozilla::" shouldn't be needed.

::: layout/generic/nsFrame.h
@@ +272,5 @@
>                ComputeSizeFlags aFlags) override;
>  
>    // Compute tight bounds assuming this frame honours its border, background
>    // and outline, its children's tight bounds, and nothing else.
> +  nsRect ComputeSimpleTightBounds(mozilla::gfx::DrawTarget* aDrawTarget) const;

Shouldn't need the "mozilla::gfx::" prefix here, I think, as you're adding a typedef in nsIFrame.

Comment 19

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/290f666471a0177f984f05d5c9933914d2d11443
Bug 1231550 - Use DrawTarget instead of gfxContext and/or nsRenderingContext in many places in font/text code. r=jfkthame.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/290f666471a0