Bug 239952 - MotionMark design subtest doesn't look right with GPUP DOM rendering enabled
Summary: MotionMark design subtest doesn't look right with GPUP DOM rendering enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Process Model (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cameron McCormack (:heycam)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-05-01 22:24 PDT by Cameron McCormack (:heycam)
Modified: 2022-05-07 16:35 PDT (History)
11 users (show)

See Also:


Attachments
MotionMark design subtest with GPUP DOM rendering enabled (582.20 KB, image/png)
2022-05-01 22:26 PDT, Cameron McCormack (:heycam)
no flags Details
Snapshotted page (666.65 KB, image/png)
2022-05-01 22:27 PDT, Cameron McCormack (:heycam)
no flags Details
slightly reduced test (21.66 KB, text/html)
2022-05-01 22:28 PDT, Cameron McCormack (:heycam)
no flags Details
Screen shot of slightly reduced test (196.57 KB, image/png)
2022-05-01 22:30 PDT, Cameron McCormack (:heycam)
no flags Details
Patch (6.05 KB, patch)
2022-05-05 00:21 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (16.30 KB, patch)
2022-05-05 23:35 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (17.31 KB, patch)
2022-05-06 16:21 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2022-05-01 22:24:53 PDT
See the attached screen shots.  The colors of the text trails isn't right.

I'll attach a smaller test case too, which demonstrates a similar problem.
Comment 1 Cameron McCormack (:heycam) 2022-05-01 22:26:30 PDT
Created attachment 458659 [details]
MotionMark design subtest with GPUP DOM rendering enabled

Screen shot mid way through the test.
Comment 2 Cameron McCormack (:heycam) 2022-05-01 22:27:28 PDT
Created attachment 458660 [details]
Snapshotted page

I took a snapshot of the design.html DOM then reloaded it as a new document.  It looked as expected.
Comment 3 Cameron McCormack (:heycam) 2022-05-01 22:28:22 PDT
Created attachment 458661 [details]
slightly reduced test

Compare this with GPUP DOM rendering enabled and disabled.  When enabled, it looks like some of the text runs do not get repainted with their updated colors.
Comment 4 Cameron McCormack (:heycam) 2022-05-01 22:30:11 PDT
Created attachment 458662 [details]
Screen shot of slightly reduced test
Comment 5 Simon Fraser (smfr) 2022-05-02 14:52:13 PDT
Oddly I can't reproduce with DOM Rendering enabled on macOS.
Comment 6 Radar WebKit Bug Importer 2022-05-02 15:10:28 PDT
<rdar://problem/92635604>
Comment 7 Cameron McCormack (:heycam) 2022-05-05 00:21:52 PDT
Created attachment 458854 [details]
Patch
Comment 8 Said Abou-Hallawa 2022-05-05 10:28:46 PDT
Comment on attachment 458854 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

Can't we write an API test for this patch? Maybe one similar to BifurcatedGraphicsContextTests.Text

> Source/WebCore/platform/graphics/GraphicsContext.cpp:596
> +GraphicsContextState GraphicsContext::stateWithChangesCleared() const

I think this function should be in GraphicsContextState.

Can't we name this function such that it reveals its purpose? Can't it be named something like GraphicsContextState::cloneForRecording()?

> Source/WebCore/platform/graphics/GraphicsContext.cpp:599
> +    state.didApplyChanges();

This call is misleading. We did not apply the changes of the state to call its didApplyChanges().

If this function is moved to GraphicsContextState then you can set m_changeFlags directly there. But if this function stays in GraphicsContext, then I would recommend adding a new function like clearChangeFlags() which can replace didApplyChanges() or coexist with it.

> Source/WebCore/platform/graphics/GraphicsContext.h:160
> +    GraphicsContextState stateWithChangesCleared() const;

I think it's better to move this function to GraphicsContextState. But if you prefer it to be here, I think a better name would be 'stateForRecording()'? And I think it can return 'const GraphicsContextState'

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:55
> +    ASSERT(!state.changes());
>      m_stateStack.append({ state, initialCTM, initialCTM.mapRect(initialClip) });

Another approach is to clone the state here:

m_stateStack.append({ state.cloneForRecording(), initialCTM, initialCTM.mapRect(initialClip) });
Comment 9 Cameron McCormack (:heycam) 2022-05-05 14:12:16 PDT
(In reply to Said Abou-Hallawa from comment #8)
> Can't we write an API test for this patch? Maybe one similar to
> BifurcatedGraphicsContextTests.Text

I will add a test to the patch.

> > Source/WebCore/platform/graphics/GraphicsContext.cpp:596
> > +GraphicsContextState GraphicsContext::stateWithChangesCleared() const
> 
> I think this function should be in GraphicsContextState.
> 
> Can't we name this function such that it reveals its purpose? Can't it be
> named something like GraphicsContextState::cloneForRecording()?

That name sounds fine.

> > Source/WebCore/platform/graphics/GraphicsContext.cpp:599
> > +    state.didApplyChanges();
> 
> This call is misleading. We did not apply the changes of the state to call
> its didApplyChanges().
> 
> If this function is moved to GraphicsContextState then you can set
> m_changeFlags directly there. But if this function stays in GraphicsContext,
> then I would recommend adding a new function like clearChangeFlags() which
> can replace didApplyChanges() or coexist with it.

Yes, clearing m_changeFlags directly in cloneForRecording sounds better.

> > Source/WebCore/platform/graphics/GraphicsContext.h:160
> > +    GraphicsContextState stateWithChangesCleared() const;
> 
> I think it's better to move this function to GraphicsContextState. But if
> you prefer it to be here, I think a better name would be
> 'stateForRecording()'? And I think it can return 'const GraphicsContextState'

Curious what purpose the const would serve.  To prevent the caller from accidentally modifying the returned object (or require them to copy the object to do so)?

> > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:55
> > +    ASSERT(!state.changes());
> >      m_stateStack.append({ state, initialCTM, initialCTM.mapRect(initialClip) });
> 
> Another approach is to clone the state here:
> 
> m_stateStack.append({ state.cloneForRecording(), initialCTM,
> initialCTM.mapRect(initialClip) });

Most DisplayListRecorders (the ones for recording the drawing on a layer) are created with a default constructed GraphicsContextState, so I figured we should avoid making a clone in that case by making it the caller's responsibility.
Comment 10 Cameron McCormack (:heycam) 2022-05-05 23:35:12 PDT
Created attachment 458936 [details]
Patch
Comment 11 Antti Koivisto 2022-05-05 23:54:43 PDT
Comment on attachment 458936 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:3287
> +void Internals::setForceUseGlyphDisplayListForTesting(bool enabled)
> +{
> +    TextPainter::setForceUseGlyphDisplayListForTesting(enabled);
> +}

Internals::resetToConsistentState should probably reset this (so it doesn't get stuck in the process if the test somehow fails).
Comment 12 Cameron McCormack (:heycam) 2022-05-06 16:21:24 PDT
Created attachment 458980 [details]
Patch
Comment 13 EWS 2022-05-07 16:35:21 PDT
Committed r293951 (250397@main): <https://commits.webkit.org/250397@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458980 [details].