Bug 239952

Summary: MotionMark design subtest doesn't look right with GPUP DOM rendering enabled
Product: WebKit Reporter: Cameron McCormack (:heycam) <heycam>
Component: WebKit Process ModelAssignee: Cameron McCormack (:heycam) <heycam>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, esprehn+autocc, ews-watchlist, glenn, koivisto, kondapallykalyan, mmaxfield, pdr, sabouhallawa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
MotionMark design subtest with GPUP DOM rendering enabled
none
Snapshotted page
none
slightly reduced test
none
Screen shot of slightly reduced test
none
Patch
none
Patch
none
Patch none

Cameron McCormack (:heycam)
Reported 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.
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
Snapshotted page (666.65 KB, image/png)
2022-05-01 22:27 PDT, Cameron McCormack (:heycam)
no flags
slightly reduced test (21.66 KB, text/html)
2022-05-01 22:28 PDT, Cameron McCormack (:heycam)
no flags
Screen shot of slightly reduced test (196.57 KB, image/png)
2022-05-01 22:30 PDT, Cameron McCormack (:heycam)
no flags
Patch (6.05 KB, patch)
2022-05-05 00:21 PDT, Cameron McCormack (:heycam)
no flags
Patch (16.30 KB, patch)
2022-05-05 23:35 PDT, Cameron McCormack (:heycam)
no flags
Patch (17.31 KB, patch)
2022-05-06 16:21 PDT, Cameron McCormack (:heycam)
no flags
Cameron McCormack (:heycam)
Comment 1 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.
Cameron McCormack (:heycam)
Comment 2 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.
Cameron McCormack (:heycam)
Comment 3 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.
Cameron McCormack (:heycam)
Comment 4 2022-05-01 22:30:11 PDT
Created attachment 458662 [details] Screen shot of slightly reduced test
Simon Fraser (smfr)
Comment 5 2022-05-02 14:52:13 PDT
Oddly I can't reproduce with DOM Rendering enabled on macOS.
Radar WebKit Bug Importer
Comment 6 2022-05-02 15:10:28 PDT
Cameron McCormack (:heycam)
Comment 7 2022-05-05 00:21:52 PDT
Said Abou-Hallawa
Comment 8 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) });
Cameron McCormack (:heycam)
Comment 9 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.
Cameron McCormack (:heycam)
Comment 10 2022-05-05 23:35:12 PDT
Antti Koivisto
Comment 11 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).
Cameron McCormack (:heycam)
Comment 12 2022-05-06 16:21:24 PDT
EWS
Comment 13 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].
Note You need to log in before you can comment on or make changes to this bug.