1275693 - refactor <canvas> text drawing to re-use gfxTextRun::Draw instead of walking the glyph runs etc itself

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jonathan Kew [:jfkthame]]

In CanvasBidiProcessor::DrawText, we have a bunch of code to walk the glyph runs in a gfxTextRun, handling vertical text, bidi-RTL, etc, which duplicates a lot of functionality also implemented by gfxTextRun::Draw, gfxFont::Draw, etc.

In bug 1274936, I have just landed a patch to make the CanvasBidiProcessor::DrawText delegate the work to gfxTextRun::Draw when filling with a solid color. However, for filling with a gradient or pattern, and for stroke operations, canvas still uses its own version of the code.

To simplify code and ease maintenance, it would be nice to go further here, and route all canvas text drawing through the same gfxTextRun::Draw code path that we use for HTML text. ISTM that given the new mask-based implementation for background-clip:text, we should be able to use the same mechanism when canvas is filling with a pattern or gradient, so that all canvas fillText operations use the gfxTextRun code path.

For strokeText support, I think we will need some extra work in gfx to fully support DrawMode::GLYPH_STROKE, so that may need to be a further followup.

Comment 1

[Jonathan Kew [:jfkthame]]

CJ, is this something you might be able to look into?

Comment 2

[Peter Chang[:pchang]]

Jonathan/CJ, I will find someone to work on this.

Comment 3

[Peter Chang[:pchang]]

Assign kevin to work on this bug.

Comment 4

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Re-use gfxTextRun::Draw in <canvas> text drawing when the style is FILL

Hi Jonathan

Can you help me to review this patch ?

Thank you.

Comment 5

[Jonathan Kew [:jfkthame]]

Sure, I'll try to read through this soon. Does it pass reftests on try already?

(What I'd really like here is to get the stroke code converted as well, so that we can completely remove the big hairy loop in CanvasBidiProcessor::DrawText. But I'm not sure how much extra effort that will need....)

Comment 6

[Kevin Chen[:kechen] (UTC + 8)]

Hi Jonathan

I will push the patch to try to check if the code is correct.

And for the stroke text, I will do some investgations to check if there still need extra works for refactoring.

Thank you.

Comment 7

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8761082 [details] [diff] [review]
Re-use gfxTextRun::Draw in <canvas> text drawing when the style is FILL

Hi Jonathan

I will re-tag r? after finishing the try-run.

Thank you.

Comment 8

[Kevin Chen[:kechen] (UTC + 8)]

Hi Jonathan

After the investigation, I found that gfxTextRun::Draw doesn't supports stroked text with patterns and only has limited parameters for stroke text.[1]

Therefore, to make canvas able to draw stroke text via gfxTextRun, we must find a way to send stroked settings and patterns to it. There are two options which can achieve this:

1. Add additional members, StrokeOptions and gfxPattern, to gfxTextRun::DrawParams, make it be able to support all stroke-text settings.

2. Wrap the patterns and stroke text settings to gfxContext in gfxTextRun::DrawParams, since canvas can only draw filled text and stroked text in one time, the gfxContext won't affect the other's gfxContext. With this way we don't have to add additional data members, however, we might have to add some codes to deal with canvas stroke-text drawing case in gfxFont[2].

Can you give me some advices about which way I should implement?

Thank you.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxTextRun.h#240-241
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#1682-1688

Comment 9

[Jonathan Kew [:jfkthame]]

I think it's probably better to try your option (1). Adding data members to DrawParams should be pretty cheap, provided we don't need to copy them much (StrokeOptions is rather big). As long as we pass DrawParams by const reference, we should be OK. Or maybe it should have a StrokeOptions* pointer, which we can leave as null when not stroking, and callers that do want to stroke can point it at a local (stack) StrokeOptions.

I'm guessing we'll eventually be migrating this code off gfxContext altogether and just using DrawTarget directly, so if we rely on the gfxContext for stroke settings here, we'll probably need to rewrite that later anyway.

Also, this approach seems more useful if CSS adds support for stroke patterns, for example, in the future; unlike canvas text, HTML DOM text can be both filled and stroked at the same time, and this will allow us to support that directly at the gfxTextRun drawing level.

Comment 11

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 2) Re-use gfxTextRun::Draw in <canvas> text drawing when the style is STROKE

Hi Jonathan

This patch is about using gfxTextRun::Draw in canvas stroke case.
I added some parameters to TextRunDrawParams and DrawParams so that canvas can pass the stroke options to GlyphBufferAzure::Flush.

Can you help me to review this patch ?

Thank you.

Comment 12

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 1) Re-use gfxTextRun::Draw in <canvas> text drawing when the style is FILL

Hi Jonathan

This patch use gfxTextRun::Draw in canvas FILL cases.
Can you help me to review this patch ?

Thank you.

Comment 13

[Kevin Chen[:kechen] (UTC + 8)]

The following link is the try run for the two patches :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e3727970e7b&selectedJob=23628651

There is a error at Android 4.3 API15+ opt [rc4], but that error is not related to the patches.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8769977 [details] [diff] [review]
(Part 1) Re-use gfxTextRun::Draw in <canvas> text drawing when the style is FILL

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

This looks great, thanks. r=me, with just a few minor suggested tweaks to the code/style.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3712,5 @@
>  
>      return NSToCoordRound(textRunMetrics.mAdvanceWidth);
>    }
>  
> +  void SetFillGradientContext(RefPtr<gfxContext>& thebes)

I don't think we usually use RefPtr<>& as a parameter type unless it's an outparam. Here, you can just use a plain gfxContext*, for consistency with how we pass gfxContext to methods throughout the gfx/thebes code.

Also, style guide says the parameter should have an `a` prefix: aThebes.

@@ +3715,5 @@
>  
> +  void SetFillGradientContext(RefPtr<gfxContext>& thebes)
> +  {
> +    RefPtr<gfxPattern> pattern;
> +    nsTArray<mozilla::gfx::GradientStop> rawStops;

I think the code will be a little tidier if you start by doing something like

  CanvasGradient* gradient = mState->gradientStyles[Style::FILL];

and then down-cast `gradient` to a `radial` or `linear` variable within the switch cases.

AFAICS we shouldn't need to use RefPtr<> for the local gradient references, they're purely temporaries and the gradient won't be going anywhere during this method.

There'll also be no need for the local copy of rawStops, you can just loop over gradient->mRawStops directly at the end of the method.

@@ +3724,5 @@
> +        static_cast<CanvasRadialGradient*>(
> +          mState->gradientStyles[Style::FILL].get());
> +      pattern = new gfxPattern(gradient->mCenter1.x, gradient->mCenter1.x,
> +                               gradient->mRadius1, gradient->mCenter2.x,
> +                               gradient->mCenter1.y, gradient->mRadius2);

Typo, I guess? I'd expect it to be mCenter2.y here.

@@ +3744,5 @@
> +    }
> +
> +    for (uint32_t i = 0; i < rawStops.Length(); i++) {
> +      pattern->AddColorStop(rawStops[i].offset, rawStops[i].color);
> +    }

The "modern" thing to do here would be

  for (auto stop: gradient->mRawStops) {
    pattern->AddColorStop(stop.offset, stop.color);
  }

@@ +3765,5 @@
> +      return gfx::ExtendMode::CLAMP;
> +    }
> +  }
> +
> +  void SetFillPatternContext(RefPtr<gfxContext>& thebes)

Again, a plain `gfxContext* aThebes` pointer should be fine, AFAICS.

@@ +3814,2 @@
>        RefPtr<gfxContext> thebes =
> +        gfxContext::ForDrawTargetWithTransform(mCtx->mTarget);

I believe ForDrawTargetWithTransform has gone, so this line will need to be reverted when you rebase to latest trunk code.

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 8769977 [details] [diff] [review]
(Part 1) Re-use gfxTextRun::Draw in <canvas> text drawing when the style is FILL

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

One more possible cleanup...

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3768,5 @@
> +
> +  void SetFillPatternContext(RefPtr<gfxContext>& thebes)
> +  {
> +    RefPtr<SourceSurface> targetSurf =
> +      (mState->patternStyles[Style::FILL].get())->mSurface;

Hmm, I don't think RefPtr<> is necessary here for targetSurf; it's just creating extra AddRef/Release overhead, but we don't need a strong reference - unless I'm overlooking something.

And the explicit .get() (here and below) shouldn't be needed. Altogether, I think this method can be simplified to something like:

  void SetFillPatternContext(gfxContext* aThebes)
  {
    const CanvasPattern* pat = mState->patternStyles[Style::FILL];
    RefPtr<gfxPattern> pattern = new gfxPattern(pat->mSurface, Matrix());
    pattern->SetExtend(CvtCanvasRepeatToGfxRepeat(pat->mRepeat));
    aThebes->SetPattern(pattern);
  }

which should be functionally equivalent.

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8769976 [details] [diff] [review]
(Part 2) Re-use gfxTextRun::Draw in <canvas> text drawing when the style is STROKE

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

Looks great - nice to see that big hairy loop in CanvasBidiProcessor finally getting removed!

r=me, with some minor comments that I think could be tidied up a bit.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3712,5 @@
>  
>      return NSToCoordRound(textRunMetrics.mAdvanceWidth);
>    }
>  
> +  already_AddRefed<gfxPattern> GetGradientContext(Style aStyle)

Ah, I see this replaces some of the stuff I commented on in the previous patch. OK, so some of the notes there may be irrelevant after the further changes anyhow.

I think the "Context" on the name here is a bit odd now, with the change to the method signature. Maybe just drop it? Or call this something like "GetGradientFor(Style aStyle)"? Similar for GetPatternContext below.

@@ +3717,4 @@
>    {
>      RefPtr<gfxPattern> pattern;
>      nsTArray<mozilla::gfx::GradientStop> rawStops;
> +    CanvasGradient::Type type = mState->gradientStyles[aStyle]->GetType();

I'd still be in favor of doing

  const CanvasGradient* gradient = mState->gradientStyles[aStyle];

first, and avoiding the local rawStops copy, as previously suggested.

@@ +3810,5 @@
>  
> +    // Defer the tasks to gfxTextRun which will handle color/svg-in-ot fonts
> +    // appropriately.
> +    if (mOp == CanvasRenderingContext2D::TextDrawOperation::FILL ||
> +        mOp == CanvasRenderingContext2D::TextDrawOperation::STROKE) {

There's no need for this if() at all, now, as there is no else codepath left. If you like, you could include an assertion that mOp must be one of FILL or STROKE; then simply get rid of this conditional, which will always be true.

::: gfx/thebes/gfxFont.cpp
@@ +1680,5 @@
>              }
>          }
>          if ((mRunParams.drawMode &
>               (DrawMode::GLYPH_STROKE | DrawMode::GLYPH_STROKE_UNDERNEATH)) ==
> +              DrawMode::GLYPH_STROKE &&

Actually, the indent was correct previously: the LHS of the == is the expression that begins at "(mRunParams", and this is the RHS, which should line up with it.

But it's really awkward to read, anyway. How about adding a helper such as

static DrawMode
GetStrokeMode(DrawMode aMode)
{
    return aMode & (DrawMode::GLYPH_STROKE | DrawMode::GLYPH_STROKE_UNDERNEATH);
}

so that you can rewrite the condition here as

    if (GetStrokeMode(mRunParams.drawMode) == DrawMode::GLYPH_STROKE &&
        mRunParams.strokeOpts) {
        ....
    }

which I think would be a lot clearer?

::: gfx/thebes/gfxFont.h
@@ +2192,2 @@
>      nscolor                  textStrokeColor;
> +    RefPtr<gfxPattern>       textStrokePattern;

Both here and in gfxTextRun::DrawParams, does this need to be a RefPtr? It seems likely a (const?) raw pointer would be fine, though I haven't checked carefully.

::: layout/generic/nsTextFrame.cpp
@@ +6654,5 @@
>              gfxTextRun::Range aRange,
>              const nsTextFrame::DrawTextRunParams& aParams)
>  {
>    gfxTextRun::DrawParams params(aParams.context);
> +  StrokeOptions strokeOpts;

It's a bit unfortunate that this means we'll (default-)construct a StrokeOptions in every DrawTextRun call, even though in the vast majority of cases we don't need it. (And I'm doubtful whether compilers will be smart enough to optimize away the construction when not needed, though I suppose we could look at the generated code.)

@@ +6679,3 @@
>        params.textStrokeColor = aParams.textStrokeColor;
> +      strokeOpts.mLineWidth = aParams.textStrokeWidth;
> +      params.strokeOpts = &strokeOpts;

To avoid the issue noted above, I wonder if it'd be worth moving the StrokeOptions local inside the if()-statement body here. Then you'd have to call aTextRun->Draw() here as well, before it goes out of scope, and put the non-stroke call to Draw() into an else clause.

Maybe it's not worth it, but something to consider if there seems to be any perf impact on general text drawing.

Comment 17

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 1) Re-use gfxTextRun::Draw in <canvas> text drawing when the style is FILL

Carry r+ from comment 14

Comment 18

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 2) Re-use gfxTextRun::Draw in <canvas> text drawing when the style is STROKE

Carry r+ from comment 16

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 8770493 [details] [diff] [review]
(Part 1) Re-use gfxTextRun::Draw in <canvas> text drawing when the style is FILL

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3717,5 @@
> +  void SetFillGradientContext(gfxContext* aThebes)
> +  {
> +    RefPtr<gfxPattern> pattern;
> +    CanvasGradient::Type type = mState->gradientStyles[Style::FILL]->GetType();
> +    CanvasGradient* gradient = mState->gradientStyles[Style::FILL];

If you reverse these two lines, you can simply write

  CanvasGradient::Type type = gradient->GetType();

rather than repeating the whole mState->gradientStyles[Style::FILL] expression.

@@ +3721,5 @@
> +    CanvasGradient* gradient = mState->gradientStyles[Style::FILL];
> +
> +    switch (type) {
> +    case CanvasGradient::Type::RADIAL: {
> +      CanvasRadialGradient* radial = (CanvasRadialGradient*) gradient;

Please use C++ static_cast<...> rather than C-style casts.

Comment 20

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 1) Re-use gfxTextRun::Draw in <canvas> text drawing when the style is FILL

Carry r+ from comment 14

Comment 21

[Carsten Book [:Tomcat]]

has problems to apply:

renamed 1275693 -> 0002-Bug-1275693-Refactor-canvas-strokeText-drawing-to-re.patch
applying 0002-Bug-1275693-Refactor-canvas-strokeText-drawing-to-re.patch
patching file dom/canvas/CanvasRenderingContext2D.cpp
Hunk #1 FAILED at 3708
Hunk #2 FAILED at 3730
Hunk #4 FAILED at 3795
3 out of 4 hunks FAILED -- saving rejects to file dom/canvas/CanvasRenderingContext2D.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0002-Bug-1275693-Refactor-canvas-strokeText-drawing-to-re.patch

can you take a look, thanks!

Comment 22

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 2) Re-use gfxTextRun::Draw in <canvas> text drawing when the style is STROKE

Carry r+ from comment 16

I've updated the patch, please help me to checkin again, thank you.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dda1fd43e2aa
Refactor <canvas> fillText drawing to re-use gfxTextRun::Draw. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0c3966c1109
Refactor <canvas> strokeText drawing to re-use gfxTextRun::Draw. r=jfkthame

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dda1fd43e2aa
https://hg.mozilla.org/mozilla-central/rev/c0c3966c1109