Bug 226170 - Virtualize GraphicsContext
Summary: Virtualize GraphicsContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-24 02:22 PDT by Tim Horton
Modified: 2021-07-01 03:25 PDT (History)
26 users (show)

See Also:


Attachments
Patch (348.62 KB, patch)
2021-05-24 02:23 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (358.37 KB, patch)
2021-05-24 13:53 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (357.72 KB, patch)
2021-05-24 15:06 PDT, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (357.21 KB, patch)
2021-05-24 15:47 PDT, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (357.21 KB, patch)
2021-05-24 16:31 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (361.93 KB, patch)
2021-05-24 17:26 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (369.37 KB, patch)
2021-05-24 19:19 PDT, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (370.49 KB, patch)
2021-05-24 20:49 PDT, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (370.55 KB, patch)
2021-05-24 23:27 PDT, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (376.14 KB, patch)
2021-05-25 00:40 PDT, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (376.27 KB, patch)
2021-05-25 01:40 PDT, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (376.39 KB, patch)
2021-05-25 02:08 PDT, Tim Horton
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (377.30 KB, patch)
2021-05-25 03:09 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (417.63 KB, patch)
2021-05-25 12:41 PDT, Tim Horton
simon.fraser: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (417.63 KB, patch)
2021-05-25 12:59 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (417.64 KB, patch)
2021-05-25 13:03 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (422.60 KB, patch)
2021-05-25 15:17 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2021-05-24 02:22:24 PDT
Virtualize GraphicsContext
Comment 1 Tim Horton 2021-05-24 02:23:15 PDT
Created attachment 429514 [details]
Patch
Comment 2 Tim Horton 2021-05-24 02:23:53 PDT
Not even close to ready for review, I just want to fiddle with EWS (and use it to get Windows building).
Comment 3 Tim Horton 2021-05-24 13:53:39 PDT
Created attachment 429560 [details]
Patch
Comment 4 Tim Horton 2021-05-24 15:06:58 PDT
Created attachment 429569 [details]
Patch
Comment 5 Tim Horton 2021-05-24 15:47:42 PDT
Created attachment 429578 [details]
Patch
Comment 6 Tim Horton 2021-05-24 16:31:50 PDT
Created attachment 429587 [details]
Patch
Comment 7 Tim Horton 2021-05-24 17:26:49 PDT
Created attachment 429598 [details]
Patch
Comment 8 Tim Horton 2021-05-24 19:19:44 PDT
Created attachment 429612 [details]
Patch
Comment 9 Tim Horton 2021-05-24 20:49:37 PDT
Created attachment 429623 [details]
Patch
Comment 10 Tim Horton 2021-05-24 23:27:31 PDT
Created attachment 429631 [details]
Patch
Comment 11 Tim Horton 2021-05-25 00:40:55 PDT
Created attachment 429635 [details]
Patch
Comment 12 Tim Horton 2021-05-25 01:40:22 PDT
Created attachment 429638 [details]
Patch
Comment 13 Tim Horton 2021-05-25 02:08:03 PDT
Created attachment 429639 [details]
Patch
Comment 14 Tim Horton 2021-05-25 03:09:42 PDT
Created attachment 429640 [details]
Patch
Comment 15 Tim Horton 2021-05-25 12:41:46 PDT
Created attachment 429680 [details]
Patch
Comment 16 Tim Horton 2021-05-25 12:59:33 PDT
Created attachment 429683 [details]
Patch
Comment 17 Tim Horton 2021-05-25 13:03:09 PDT
Created attachment 429684 [details]
Patch
Comment 18 Simon Fraser (smfr) 2021-05-25 13:35:11 PDT
Comment on attachment 429680 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429680&action=review

> Source/WebCore/platform/graphics/GraphicsContext.h:341
> +    void setDrawLuminanceMask(bool drawLuminanceMask) { m_state.drawLuminanceMask = drawLuminanceMask; updateState(m_state, GraphicsContextState::DrawLuminanceMaskChange); }
> +    bool drawLuminanceMask() const { return m_state.drawLuminanceMask; }

Maybe we could rename these to reduce ambiguity: setIsDrawingLuminanceMask/isDrawingLuminanceMask

> Source/WebCore/platform/graphics/NullGraphicsContext.h:52
> +    bool paintingDisabled() const override { return true; }

Why not final?

> Source/WebCore/platform/graphics/NullGraphicsContext.h:155
> +    const PaintInvalidationReasons m_paintInvalidationReasons { PaintInvalidationReasons::None };

This could be an OptionSet<> in future.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:105
> +    bool fill = mode.contains(TextDrawingMode::Fill);
> +    bool stroke = mode.contains(TextDrawingMode::Stroke);
> +    if (fill && stroke)
> +        return kCGTextFillStroke;
> +    if (fill)
> +        return kCGTextFill;
> +    return kCGTextStroke;

Could be simplified with some OptionSet<> goop.

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:53
> +class Recorder : public GraphicsContext {

I think we should rename this class to RecordingContext

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:95
> +#if USE(CG) || USE(DIRECT2D)
> +    void setIsCALayerContext(bool) override { }
> +    bool isCALayerContext() const override { return false; }
> +    void setIsAcceleratedContext(bool) override { }
> +#endif
> +
> +    void fillRoundedRectImpl(const FloatRoundedRect&, const Color&) override { ASSERT_NOT_REACHED(); }
> +    void drawLineForText(const FloatRect&, bool, bool, StrokeStyle) override { ASSERT_NOT_REACHED(); }

Can everything be final here?
Comment 19 Said Abou-Hallawa 2021-05-25 14:41:29 PDT
Comment on attachment 429684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429684&action=review

> Source/WebCore/ChangeLog:13
> +        In preparation for future patches which introduce new kinds of GraphicsContexts,
> +        virtualize GraphicsContext, moving platform implementations into subclasses
> +        GraphicsContext{CG,Cairo,Direct2D}, NullGraphicsContext, DisplayList::Recorder,
> +        and Nicosia::CairoOperationRecorder.

I think this is great work and it is going to save us from worrying about the paintingDisabled(), m_impl is not null, and the calling the platform function in very function of GraphicsContext. But I do not like the way the superclasses of GraphicsContext are created. I think they should be pointers from now on and the caller should only keep a std::unique_ptr< GraphicsContext > or RefPtr<GraphicsContext> instead. The creation can be the responsibility of GraphicsContext also. For example:

class GraphicsContext {
public:
    static std::unique_ptr< GraphicsContext > create(CGContextRef);

    static std::unique_ptr< GraphicsContext > create(PlatformContextCairo&);
    static std::unique_ptr< GraphicsContext > create(PlatformContextCairo*);
    static std::unique_ptr< GraphicsContext > create(cairo_t*);

    static std::unique_ptr< GraphicsContext > create(DisplayList&, const GraphicsContextState&, const FloatRect& initialClip, const AffineTransform&, Delegate*, DrawGlyphsRecorder::DrawGlyphsDeconstruction);
};

And this case a caller like the one in GraphicsLayerCA.cpp will do just this:

    auto context = GraphicsContext::create(*m_displayList, GraphicsContextState(), initialClip, AffineTransform());
    paintGraphicsLayerContents(*context, FloatRect(FloatPoint(), size()));

The reason for me preferring this approach over something like:

    GraphicsContextCG gc(context.get());

is we have to declare the variable to be GraphicsContextCG just to allocate the memory of the object correctly. But I do not think we will ever need to access the members the superclass directly. So using the the base type GraphicsContext is more appropriate and having it a pointer will solve the allocation issue.
Comment 20 Tim Horton 2021-05-25 15:17:59 PDT
Created attachment 429698 [details]
Patch
Comment 21 Tim Horton 2021-05-25 16:30:09 PDT
(In reply to Simon Fraser (smfr) from comment #18)
> Comment on attachment 429680 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429680&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:341
> > +    void setDrawLuminanceMask(bool drawLuminanceMask) { m_state.drawLuminanceMask = drawLuminanceMask; updateState(m_state, GraphicsContextState::DrawLuminanceMaskChange); }
> > +    bool drawLuminanceMask() const { return m_state.drawLuminanceMask; }
> 
> Maybe we could rename these to reduce ambiguity:
> setIsDrawingLuminanceMask/isDrawingLuminanceMask

Leaving it for now.

> > Source/WebCore/platform/graphics/NullGraphicsContext.h:52
> > +    bool paintingDisabled() const override { return true; }
> 
> Why not final?

Copy paste-o.

> > Source/WebCore/platform/graphics/NullGraphicsContext.h:155
> > +    const PaintInvalidationReasons m_paintInvalidationReasons { PaintInvalidationReasons::None };
> 
> This could be an OptionSet<> in future.
> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:105
> > +    bool fill = mode.contains(TextDrawingMode::Fill);
> > +    bool stroke = mode.contains(TextDrawingMode::Stroke);
> > +    if (fill && stroke)
> > +        return kCGTextFillStroke;
> > +    if (fill)
> > +        return kCGTextFill;
> > +    return kCGTextStroke;
> 
> Could be simplified with some OptionSet<> goop.

Both true.

> > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:53
> > +class Recorder : public GraphicsContext {
> 
> I think we should rename this class to RecordingContext

Strongly Agree. Leaving it for now.

> > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:95
> > +#if USE(CG) || USE(DIRECT2D)
> > +    void setIsCALayerContext(bool) override { }
> > +    bool isCALayerContext() const override { return false; }
> > +    void setIsAcceleratedContext(bool) override { }
> > +#endif
> > +
> > +    void fillRoundedRectImpl(const FloatRoundedRect&, const Color&) override { ASSERT_NOT_REACHED(); }
> > +    void drawLineForText(const FloatRect&, bool, bool, StrokeStyle) override { ASSERT_NOT_REACHED(); }
> 
> Can everything be final here?

Yes, done.

(In reply to Said Abou-Hallawa from comment #19)
> Comment on attachment 429684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429684&action=review
> But I do not think we will ever need to
> access the members the superclass directly. So using the the base type
> GraphicsContext is more appropriate and having it a pointer will solve the
> allocation issue.

I think I agree with the premise (we don't need to call things on the subclass, and it would be nice if we just had a `GraphicsContext`). I don't want to de-stack-allocate all of the GraphicsContexts in this patch, though, that's a separate similarly large change :)

Also I don't think it's SO crazy to say `GraphicsContextCG` when *you are passing it a CGContextRef*. The code is already platform-specific.
Comment 22 Tim Horton 2021-05-25 16:32:41 PDT
(Windows being the painfully obvious counterpoint to my last point there)
Comment 23 EWS 2021-05-25 17:01:37 PDT
Committed r278062 (238144@main): <https://commits.webkit.org/238144@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429698 [details].
Comment 24 Radar WebKit Bug Importer 2021-05-25 17:02:32 PDT
<rdar://problem/78483981>