MotionMark design subtest doesn't look right with GPUP DOM rendering enabled

• Component: WebKit Process Model
• Status: RESOLVED FIXED

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Created attachment 458659 [details]
MotionMark design subtest with GPUP DOM rendering enabled

Screen shot mid way through the test.

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Created attachment 458662 [details]
Screen shot of slightly reduced test

[Simon Fraser (smfr)]

Oddly I can't reproduce with DOM Rendering enabled on macOS.

[Radar WebKit Bug Importer]

<rdar://problem/92635604>

[Cameron McCormack (:heycam)]

Created attachment 458854 [details]
Patch

[Said Abou-Hallawa]

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) });

[Cameron McCormack (:heycam)]

(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.

[Cameron McCormack (:heycam)]

Created attachment 458936 [details]
Patch

[Antti Koivisto]

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).

[Cameron McCormack (:heycam)]

Created attachment 458980 [details]
Patch

[EWS]

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].