Bug 47501 - Manage DrawingBuffer lifetime in GraphicsContext3D
Summary: Manage DrawingBuffer lifetime in GraphicsContext3D
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords:
Depends on:
Blocks: 47379
  Show dependency treegraph
 
Reported: 2010-10-11 13:10 PDT by Chris Marrin
Modified: 2010-10-12 17:54 PDT (History)
6 users (show)

See Also:


Attachments
Patch (13.05 KB, patch)
2010-10-11 13:30 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch fixing Chromium build (13.09 KB, patch)
2010-10-11 13:49 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch with more Chromium fixes and added m_context null checks to DrawingBuffer functions (14.02 KB, patch)
2010-10-11 14:14 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (14.11 KB, patch)
2010-10-11 14:32 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch adding removeDrawingBuffer calls and fixing more Chromium build problems (14.14 KB, patch)
2010-10-11 14:33 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch adding protection against null m_context in DrawingBuffer ctors (14.24 KB, patch)
2010-10-11 14:41 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch fixing another Chromium build issue (14.54 KB, patch)
2010-10-11 15:30 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch fixing merge conflicts (14.56 KB, patch)
2010-10-11 15:37 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (14.76 KB, patch)
2010-10-12 09:54 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch gets rid of const on platformLayer() so Chromium builds (13.96 KB, patch)
2010-10-12 10:39 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch using ref counting rather than weak pointers (please hold review) (20.06 KB, patch)
2010-10-12 16:16 PDT, Chris Marrin
cmarrin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2010-10-11 13:10:41 PDT
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.
Comment 1 Chris Marrin 2010-10-11 13:30:49 PDT
Created attachment 70462 [details]
Patch
Comment 2 WebKit Review Bot 2010-10-11 13:46:19 PDT
Attachment 70462 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4324027
Comment 3 Chris Marrin 2010-10-11 13:49:02 PDT
Created attachment 70464 [details]
Patch fixing Chromium build
Comment 4 WebKit Review Bot 2010-10-11 14:03:24 PDT
Attachment 70464 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4355019
Comment 5 Chris Marrin 2010-10-11 14:14:54 PDT
Created attachment 70466 [details]
Patch with more Chromium fixes and added m_context null checks to DrawingBuffer functions
Comment 6 WebKit Review Bot 2010-10-11 14:27:08 PDT
Attachment 70466 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4343025
Comment 7 Chris Marrin 2010-10-11 14:32:09 PDT
Created attachment 70468 [details]
Patch
Comment 8 Chris Marrin 2010-10-11 14:33:13 PDT
Created attachment 70469 [details]
Patch adding removeDrawingBuffer calls and fixing more Chromium build problems
Comment 9 Chris Marrin 2010-10-11 14:41:50 PDT
Created attachment 70473 [details]
Patch adding protection against null m_context in DrawingBuffer ctors
Comment 10 WebKit Review Bot 2010-10-11 14:55:34 PDT
Attachment 70469 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4369015
Comment 11 WebKit Review Bot 2010-10-11 15:13:55 PDT
Attachment 70473 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4336025
Comment 12 Chris Marrin 2010-10-11 15:30:06 PDT
Created attachment 70480 [details]
Patch fixing another Chromium build issue
Comment 13 Chris Marrin 2010-10-11 15:37:23 PDT
Created attachment 70483 [details]
Patch fixing merge conflicts
Comment 14 WebKit Review Bot 2010-10-11 18:27:01 PDT
Attachment 70483 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4304034
Comment 15 Chris Marrin 2010-10-12 09:54:42 PDT
Created attachment 70537 [details]
Patch
Comment 16 Eric Seidel (no email) 2010-10-12 10:05:39 PDT
Attachment 70537 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4286037
Comment 17 WebKit Review Bot 2010-10-12 10:09:25 PDT
Attachment 70537 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4289034
Comment 18 Chris Marrin 2010-10-12 10:39:06 PDT
Created attachment 70542 [details]
Patch gets rid of const on platformLayer() so Chromium builds
Comment 19 Chris Marrin 2010-10-12 16:16:00 PDT
Created attachment 70571 [details]
Patch using ref counting rather than weak pointers (please hold review)
Comment 20 Darin Adler 2010-10-12 16:30:29 PDT
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.
Comment 21 Chris Marrin 2010-10-12 16:44:48 PDT
(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.
Comment 22 Chris Marrin 2010-10-12 17:01:12 PDT
(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.
Comment 23 Chris Marrin 2010-10-12 17:18:26 PDT
Landed in http://trac.webkit.org/changeset/69619
Comment 24 WebKit Review Bot 2010-10-12 17:41:18 PDT
http://trac.webkit.org/changeset/69619 might have broken Chromium Win Release
Comment 25 WebKit Review Bot 2010-10-12 17:54:09 PDT
Attachment 70571 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4392002