Manage DrawingBuffer lifetime in GraphicsContext3D

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Chris Marrin]

This is part of https://bugs.webkit.org/show_bug.cgi?id=47379. Move creation of DrawingBuffer to GraphicsContext3D, keep a list of DrawingBuffers and manage their lifetime to avoid dangling pointers. DrawingBuffer for Mac is still not completely implemented and GraphicsContext3D still creates its own "drawing buffer" for use by WebGL.

[Chris Marrin]

Created attachment 70462 [details]
Patch

[WebKit Review Bot]

Attachment 70462 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4324027

[Chris Marrin]

Created attachment 70464 [details]
Patch fixing Chromium build

[WebKit Review Bot]

Attachment 70464 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4355019

[Chris Marrin]

Created attachment 70466 [details]
Patch with more Chromium fixes and added m_context null checks to DrawingBuffer functions

[WebKit Review Bot]

Attachment 70466 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4343025

[Chris Marrin]

Created attachment 70468 [details]
Patch

[Chris Marrin]

Created attachment 70469 [details]
Patch adding removeDrawingBuffer calls and fixing more Chromium build problems

[Chris Marrin]

Created attachment 70473 [details]
Patch adding protection against null m_context in DrawingBuffer ctors

[WebKit Review Bot]

Attachment 70469 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4369015

[WebKit Review Bot]

Attachment 70473 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4336025

[Chris Marrin]

Created attachment 70480 [details]
Patch fixing another Chromium build issue

[Chris Marrin]

Created attachment 70483 [details]
Patch fixing merge conflicts

[WebKit Review Bot]

Attachment 70483 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4304034

[Chris Marrin]

Created attachment 70537 [details]
Patch

[Eric Seidel (no email)]

Attachment 70537 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4286037

[WebKit Review Bot]

Attachment 70537 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4289034

[Chris Marrin]

Created attachment 70542 [details]
Patch gets rid of const on platformLayer() so Chromium builds

[Chris Marrin]

Created attachment 70571 [details]
Patch using ref counting rather than weak pointers (please hold review)

[Darin Adler]

Comment on attachment 70571 [details]
Patch using ref counting rather than weak pointers (please hold review)

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

> WebCore/platform/graphics/GraphicsContext3D.h:442
> +    static PassRefPtr<GraphicsContext3D> create(Attributes attrs, HostWindow* hostWindow, RenderStyle renderStyle = RenderOffscreen);

The argument names here are not helpful and should be omitted.

> WebCore/platform/graphics/gpu/DrawingBuffer.cpp:42
> +    return (drawingBuffer->m_context) ? drawingBuffer : 0;

Should be drawingBuffer.release() rather than drawingBuffer.

> WebCore/platform/graphics/gpu/DrawingBuffer.cpp:55
> +    m_context = 0;

Should use clear() to clear RefPtr rather than assignment to 0.

> WebCore/platform/graphics/gpu/DrawingBuffer.h:65
> +     // Clear all resources from this object, as well as context. Called when context is destroyed
> +    // to prevent invalid accesses to the resources.
> +    void clear();

The first line of the comment is indented incorrectly.

> WebCore/platform/graphics/gpu/DrawingBuffer.h:88
> +protected:
> +    static PassRefPtr<DrawingBuffer> create(PassRefPtr<GraphicsContext3D>, const IntSize&);

Why protected rather than private?

> WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:64
> -    return adoptRef(new SharedGraphicsContext3D(context.release(), solidFillShader.release(), texShader.release()));
> +    return adoptRef(new SharedGraphicsContext3D(context, solidFillShader.release(), texShader.release()));

This need not be changed: A call to release() here is still a good idea. It will avoid a bit of reference count churn.

> WebCore/platform/graphics/gpu/mac/DrawingBufferMac.mm:63
> +    if (!m_context)
> +        return;
> +        
> +    clear();

Since the clear() function is already safe to call if m_context is 0, I suggest omitting the if statement and early return here.

> WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:86
> -    OwnPtr<GraphicsContext3D> context(new GraphicsContext3D(attrs, hostWindow, false));
> -    return context->m_contextObj ? context.release() : 0;
> +    RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(attrs, hostWindow, false));
> +    return context->m_contextObj ? context : 0;

No reason to remove the release() call here. Including it reduces the reference count churn a bit.

[Chris Marrin]

(In reply to comment #20)
> ...
> > WebCore/platform/graphics/gpu/DrawingBuffer.h:88
> > +protected:
> > +    static PassRefPtr<DrawingBuffer> create(PassRefPtr<GraphicsContext3D>, const IntSize&);
> 
> Why protected rather than private?

This create method is used by GraphicsContext3D, which is a friend, so it has to be protected.

[Chris Marrin]

(In reply to comment #21)
> (In reply to comment #20)
> > ...
> > > WebCore/platform/graphics/gpu/DrawingBuffer.h:88
> > > +protected:
> > > +    static PassRefPtr<DrawingBuffer> create(PassRefPtr<GraphicsContext3D>, const IntSize&);
> > 
> > Why protected rather than private?
> 
> This create method is used by GraphicsContext3D, which is a friend, so it has to be protected.

I realize now that friends can access private members. So I will make this change. I'm not sure where I got the outdated notion that friends could only access protected members.

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/69619

[WebKit Review Bot]

http://trac.webkit.org/changeset/69619 might have broken Chromium Win Release

[WebKit Review Bot]

Attachment 70571 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4392002