Webkit2 Dev talk

Development OS
1

I figured for more “development” talk, we could have ourselves a little forum topic. If you develop on haikuwebkit’s webkit2 port feel free to ask questions, or provide answers.

But please keep any user supports questions out of this thread.

So I’ll start:

I’m investigating the function
WebViewBase::Draw()

Currently this is the most “critical” flaw, Drawing still looks quite bad in MiniBrowser, with frequent flashes of the background (panel) color. I figure this somehow means the background view is beeing redrawn, and then webkit above it. (?)

void WebViewBase::Draw(BRect update)
{
#if USE(COORDINATED_GRAPHICS) || USE(TEXTURE_MAPPER)
auto drawingArea = static_cast<DrawingAreaProxyCoordinatedGraphics*>(page()->drawingArea());
if (!drawingArea)
return;

IntRect updateArea(update);

callOnMainRunLoop([this, drawingArea, updateArea](){
    LockLooper();
    WebCore::Region unpainted;

    // TODO: Is it possible to make this not require being called from the
    // main thread?
    drawingArea->paint(this, updateArea, unpainted);

    UnlockLooper();
});

#endif
}

Interestingly enough, moving the drawingArea- > Paint() out of the callOnMainLoop and into the function body makes drawing a bit faster. I’m not sure what the rationale is for the TODO, why it didn’t work then. Or is this related to locking perhaps? It feels “faster” in the function body directly.

The actual drawing, that is the paint() goes through a proxy, and at the end ends up in our backing store, pretty straightforward

void BackingStore::paint(BView* into, const WebCore::IntRect& rect)
{
// Paint the contents of our bitmap into the BView.
into->PushState();
into->SetDrawingMode(B_OP_COPY);
into->DrawBitmap(&m_bitmap, rect, rect);
into->PopState();
// TODO: Would SetViewBitmap work instead? We probably would only need
// to call it once from the WebView. Is it faster?
}

Now my question is, what is the original reason it was called from the runloop, is this architectural in nature?

4 Likes
2

Hmm, I think it was a memory safety issue. Let me see…

So, if I remember correctly, almost everything in WebKit runs on the main thread. WebViewBase::Draw, however, is, IIRC, being called on the BWindow thread. We are violating an assumption that WebKit makes and should carefully check that we aren’t going to cause any race conditions. Let me look some more…

I think my primary concern was DrawingAreaProxyCoordinatedGraphics::forceUpdateIfNeeded(). There’s a lot of code there to review. But it should be very much possible to get most/all of it to work outside the main thread.

1 Like
3

If you need to draw BView contents in thread other that BWindow looper thread, you have 2 options:

  1. Draw to BBitmap or BPicture in other thread and then call DrawBitmap or DrawPicture in BView::Draw implementation.

  2. Send message to other thread, unlock window looper and block current thread in BView::Draw implementation. In other thread handle that sent message: lock window looper, do drawing on BView, unlock window looper, unblock BWindow thread.

In general you must draw view contents in BView::Draw implementation, otherwise various troubles and artifacts will appear.

ForceUpdateIfNeeded method is unreliable.

2 Likes
4

Naive question, is the Haiku view skipping the background paint completely since the Draw() function overwrites a full pixmap.

This line of code in the BView constructor eliminates background painting before the derived Draw() is called.

SetViewColor(B_TRANSPARENT_COLOR);

I expect this to be set, any Haiku app which redraws entire window frame has this line of code.

1 Like
5

Funnily enough, no, it wasn’t set. I thought of this yesterday but checked in the BView flags and could not find it. It’s a bit non-obvious (to me) that this has to be set as a color. I guess I would have preffered a View flag!

In any case, thanks for the hint: I’ve uploaded the change now. #13 - Haiku: WebViewBase: Use transparent view color - haiku/haikuwebkit - Codeberg.org

Do we have a plan for what needs to be reviewed? Just not drawing in the Webkit thread does improve performance somewhat, it seems atleast without the Transparent flag, but introduces some artifacts aswell. With the transparent flag the code as is works better with no artifacts, apart from when the window gets resized and leaves a white background… Not sure if this could be adjusted to draw the Document color there “quickly” before webkit has a chance to draw it’s stuff, so it is atleast consistent.

6

Drawing on BView attached to BWindow (not BBitmap) outside of BView::Draw method implementation is significantly slower because back to front buffer copy will be performed on each drawing method call.

7

Here are my typical view flags to catch resize events and force Draw() call

: BView(frame, nullptr, B_FOLLOW_ALL, B_WILL_DRAW | B_FRAME_EVENTS | B_FULL_UPDATE_ON_RESIZE)

8

I’m not sure. Pretty much every memory read and write is suspect because this code was designed to run from the main thread (I’m pretty sure) and we’re violating that assumption.

In DrawingAreaProxyCoordinatedGraphics::forceUpdateIfNeeded(), it sends out a message. There are many, many lines of code dedicated to messaging sending if you dig into that. If we send the ForceUpdate message from the main thread but do everything else in the BWindow thread, then we would have much less code to review.

Outside of forceUpdateIfNeeded, most of the code in DrawingAreaProxyCoordinatedGraphics::paint() seems to consist of memory reads. At least there is less risk of things going catastrophically wrong if we read memory while the main thread is writing to it compare to a race condition involving two memory writes. Still, if we wanted to be certain, we would need to check all of them carefully.

Yes, that lets us sidestep most of the memory safety issues directly. And if we succeed in getting DrawingAreaProxyCoordinatedGraphics::paint to run on the BWindow thread, this is essentially what we would be doing. In BackingStore::paint we see into->DrawBitmap(&m_bitmap, rect, rect);. m_bitmap is updated on the main thread as updates become available and we would then like to draw m_bitmap (or call BackingStore::paint) directly on the BWindow thread.

Hmm, we are almost entirely doing this at present. We are sending a message to the other thread with callOnMainRunLoop and then the main thread handles the BView drawing. But we do not block BView::Draw currently. Would that solve the flashing? It seems like the most memory-safe way to do things but somewhat slower at drawing updates. Anyway, I think all we need to do to do this method is to replace callOnMainRunLoop with callOnMainRunLoopAndWait

9

Yes it will. Drawing inside BView::Draw is performed inside single draw transaction that is committed at once when calling of all Draw methods completed. If drawing is performed outside of Draw method implementation, each drawing call will be executed in separate transaction and perform copy from backbuffer to screen frontbuffer.

1 Like