Bug 311103 – REGRESSION(308458@main): [GTK] white noise images

• Component: WebKitGTK
• Status: NEW

[Fujii Hironori]

After 308458@main, images sometimes have white noise on my laptop. However, no such problem on my desktop.

[Fujii Hironori]

Created attachment 478852 [details]
screenshot

[Fujii Hironori]

Adding sleep can work around the issue. However, it becomes slow.

diff --git a/Source/WebCore/platform/graphics/skia/SkiaReplayCanvas.cpp b/Source/WebCore/platform/graphics/skia/SkiaReplayCanvas.cpp
index 482ac8d7f28e..86dc824bc8c9 100644
--- a/Source/WebCore/platform/graphics/skia/SkiaReplayCanvas.cpp
+++ b/Source/WebCore/platform/graphics/skia/SkiaReplayCanvas.cpp
@@ -47,6 +47,7 @@ SkiaReplayCanvas::SkiaReplayCanvas(const IntSize& size, const RefPtr<SkiaRecordi
 
         m_recording->waitForUploadCondition();
         m_recording->waitForUploadFence();
+        sleep(500_ms);
 
         const auto& gpuAtlases = m_recording->gpuAtlases();
         m_atlases.reserveInitialCapacity(gpuAtlases.size());

[Fujii Hironori]

No. It still happens with the patch. Lower probability.

[Fujii Hironori]

Setting WEBKIT_DISABLE_DMABUF_ATLAS=1 can work around the issue.

[Nikolas Zimmermann]

I guess the problem is related to updating the dma-buf from another thread:

```
    // DMA-buf path: create atlas without uploading, dispatch pixel writes to worker.
    if (!m_uploadWorkQueue)
        m_uploadWorkQueue = WorkQueue::create("AtlasUpload"_s);
    uploadCondition.addPending();
    m_uploadWorkQueue->dispatch([atlas = Ref { *atlas }, condition = Ref { uploadCondition }]() mutable {
        atlas->uploadImages();
        condition->signal();
    });
    return atlas;
```

[Nikolas Zimmermann]

Maybe it is related to sharing the same write scope for multiple disjoint writes -- which should work in theory, but maybe there's a driver dependent issue. Can you create the writeScope inside the inner for loop instead of outside?

```
bool SkiaGPUAtlas::uploadImages()
{
...
#if USE(GBM)
    if (auto* gpuBuffer = m_atlasTexture->memoryMappedGPUBuffer()) {
        if (gpuBuffer->isLinear() || gpuBuffer->isVivanteSuperTiled()) {
            for (const auto& entry : m_layout->entries()) {
                auto writeScope = makeGPUBufferWriteScope(*gpuBuffer);
                if (!writeScope)
                     return false;
                if (auto pixels = pixelDataInSRGB(entry.rasterImage))
                    gpuBuffer->updateContents(*writeScope, pixels->first, entry.atlasRect, pixels->second);
            }

            return true;
        }
    }
#endif
```

[Fujii Hironori]

Unfortunately, it has no effect.

diff --git a/Source/WebCore/platform/graphics/skia/SkiaGPUAtlas.cpp b/Source/WebCore/platform/graphics/skia/SkiaGPUAtlas.cpp
index 4d5a36f96e60..bb8375c96661 100644
--- a/Source/WebCore/platform/graphics/skia/SkiaGPUAtlas.cpp
+++ b/Source/WebCore/platform/graphics/skia/SkiaGPUAtlas.cpp
@@ -101,11 +101,10 @@ bool SkiaGPUAtlas::uploadImages()
 #if USE(GBM)
     if (auto* gpuBuffer = m_atlasTexture->memoryMappedGPUBuffer()) {
         if (gpuBuffer->isLinear() || gpuBuffer->isVivanteSuperTiled()) {
-            auto writeScope = makeGPUBufferWriteScope(*gpuBuffer);
-            if (!writeScope)
-                return false;
-
             for (const auto& entry : m_layout->entries()) {
+                auto writeScope = makeGPUBufferWriteScope(*gpuBuffer);
+                if (!writeScope)
+                    return false;
                 if (auto pixels = pixelDataInSRGB(entry.rasterImage))
                     gpuBuffer->updateContents(*writeScope, pixels->first, entry.atlasRect, pixels->second);
             }

[Nikolas Zimmermann]

Ok, one things less to check.

Please test instead:

Changing:
```
    // DMA-buf path: create atlas without uploading, dispatch pixel writes to worker.
    if (!m_uploadWorkQueue)
        m_uploadWorkQueue = WorkQueue::create("AtlasUpload"_s);
    uploadCondition.addPending();
    m_uploadWorkQueue->dispatch([atlas = Ref { *atlas }, condition = Ref { uploadCondition }]() mutable {
        atlas->uploadImages();
        condition->signal();
    });
    return atlas;
```

To:

```
    // DMA-buf path: create atlas without uploading, dispatch pixel writes to worker.
    atlas->uploadImages();
    uploadCondition->signal();
    return atlas;
```


That does the uploads on the main thread instead of a worker thread.

[Nikolas Zimmermann]

(Not fully correct, you need the addPending()... - but you get the idea, just remove the work queue out of the condition)