RESOLVED FIXED 226170
Virtualize GraphicsContext
https://bugs.webkit.org/show_bug.cgi?id=226170
Summary Virtualize GraphicsContext
Tim Horton
Reported 2021-05-24 02:22:24 PDT
Virtualize GraphicsContext
Attachments
Patch (348.62 KB, patch)
2021-05-24 02:23 PDT, Tim Horton
no flags
Patch (358.37 KB, patch)
2021-05-24 13:53 PDT, Tim Horton
no flags
Patch (357.72 KB, patch)
2021-05-24 15:06 PDT, Tim Horton
ews-feeder: commit-queue-
Patch (357.21 KB, patch)
2021-05-24 15:47 PDT, Tim Horton
ews-feeder: commit-queue-
Patch (357.21 KB, patch)
2021-05-24 16:31 PDT, Tim Horton
no flags
Patch (361.93 KB, patch)
2021-05-24 17:26 PDT, Tim Horton
no flags
Patch (369.37 KB, patch)
2021-05-24 19:19 PDT, Tim Horton
ews-feeder: commit-queue-
Patch (370.49 KB, patch)
2021-05-24 20:49 PDT, Tim Horton
ews-feeder: commit-queue-
Patch (370.55 KB, patch)
2021-05-24 23:27 PDT, Tim Horton
ews-feeder: commit-queue-
Patch (376.14 KB, patch)
2021-05-25 00:40 PDT, Tim Horton
ews-feeder: commit-queue-
Patch (376.27 KB, patch)
2021-05-25 01:40 PDT, Tim Horton
ews-feeder: commit-queue-
Patch (376.39 KB, patch)
2021-05-25 02:08 PDT, Tim Horton
ews-feeder: commit-queue-
Patch (377.30 KB, patch)
2021-05-25 03:09 PDT, Tim Horton
no flags
Patch (417.63 KB, patch)
2021-05-25 12:41 PDT, Tim Horton
simon.fraser: review+
ews-feeder: commit-queue-
Patch (417.63 KB, patch)
2021-05-25 12:59 PDT, Tim Horton
no flags
Patch (417.64 KB, patch)
2021-05-25 13:03 PDT, Tim Horton
no flags
Patch (422.60 KB, patch)
2021-05-25 15:17 PDT, Tim Horton
no flags
Tim Horton
Comment 1 2021-05-24 02:23:15 PDT
Tim Horton
Comment 2 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).
Tim Horton
Comment 3 2021-05-24 13:53:39 PDT
Tim Horton
Comment 4 2021-05-24 15:06:58 PDT
Tim Horton
Comment 5 2021-05-24 15:47:42 PDT
Tim Horton
Comment 6 2021-05-24 16:31:50 PDT
Tim Horton
Comment 7 2021-05-24 17:26:49 PDT
Tim Horton
Comment 8 2021-05-24 19:19:44 PDT
Tim Horton
Comment 9 2021-05-24 20:49:37 PDT
Tim Horton
Comment 10 2021-05-24 23:27:31 PDT
Tim Horton
Comment 11 2021-05-25 00:40:55 PDT
Tim Horton
Comment 12 2021-05-25 01:40:22 PDT
Tim Horton
Comment 13 2021-05-25 02:08:03 PDT
Tim Horton
Comment 14 2021-05-25 03:09:42 PDT
Tim Horton
Comment 15 2021-05-25 12:41:46 PDT
Tim Horton
Comment 16 2021-05-25 12:59:33 PDT
Tim Horton
Comment 17 2021-05-25 13:03:09 PDT
Simon Fraser (smfr)
Comment 18 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?
Said Abou-Hallawa
Comment 19 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.
Tim Horton
Comment 20 2021-05-25 15:17:59 PDT
Tim Horton
Comment 21 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.
Tim Horton
Comment 22 2021-05-25 16:32:41 PDT
(Windows being the painfully obvious counterpoint to my last point there)
EWS
Comment 23 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].
Radar WebKit Bug Importer
Comment 24 2021-05-25 17:02:32 PDT
Note You need to log in before you can comment on or make changes to this bug.