Bug 47501

Summary:

Manage DrawingBuffer lifetime in GraphicsContext3D

Product:

WebKit

Reporter:

Chris Marrin <cmarrin>

Component:

Layout and Rendering

Assignee:

Chris Marrin <cmarrin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dglazkov, eric, jamesr, kbr, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

47379

Attachments:

Description	Flags
Patch	none
Patch fixing Chromium build	none
Patch with more Chromium fixes and added m_context null checks to DrawingBuffer functions	none
Patch	none
Patch adding removeDrawingBuffer calls and fixing more Chromium build problems	none
Patch adding protection against null m_context in DrawingBuffer ctors	none
Patch fixing another Chromium build issue	none
Patch fixing merge conflicts	none
Patch	none
Patch gets rid of const on platformLayer() so Chromium builds	none
Patch using ref counting rather than weak pointers (please hold review)	cmarrin: review+

Chris Marrin

Reported 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.

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
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Chris Marrin

Comment 1 2010-10-11 13:30:49 PDT

Created attachment 70462 [details] Patch

WebKit Review Bot

Comment 2 2010-10-11 13:46:19 PDT

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

Chris Marrin

Comment 3 2010-10-11 13:49:02 PDT

Created attachment 70464 [details] Patch fixing Chromium build

WebKit Review Bot

Comment 4 2010-10-11 14:03:24 PDT

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

Chris Marrin

Comment 5 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

WebKit Review Bot

Comment 6 2010-10-11 14:27:08 PDT

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

Chris Marrin

Comment 7 2010-10-11 14:32:09 PDT

Created attachment 70468 [details] Patch

Chris Marrin

Comment 8 2010-10-11 14:33:13 PDT

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

Chris Marrin

Comment 9 2010-10-11 14:41:50 PDT

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

WebKit Review Bot

Comment 10 2010-10-11 14:55:34 PDT

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

WebKit Review Bot

Comment 11 2010-10-11 15:13:55 PDT

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

Chris Marrin

Comment 12 2010-10-11 15:30:06 PDT

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

Chris Marrin

Comment 13 2010-10-11 15:37:23 PDT

Created attachment 70483 [details] Patch fixing merge conflicts

WebKit Review Bot

Comment 14 2010-10-11 18:27:01 PDT

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

Chris Marrin

Comment 15 2010-10-12 09:54:42 PDT

Created attachment 70537 [details] Patch

Eric Seidel (no email)

Comment 16 2010-10-12 10:05:39 PDT

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

WebKit Review Bot

Comment 17 2010-10-12 10:09:25 PDT

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

Chris Marrin

Comment 18 2010-10-12 10:39:06 PDT

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

Chris Marrin

Comment 19 2010-10-12 16:16:00 PDT

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

Darin Adler

Comment 20 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.

Chris Marrin

Comment 21 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.

Chris Marrin

Comment 22 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.

Chris Marrin

Comment 23 2010-10-12 17:18:26 PDT

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

WebKit Review Bot

Comment 24 2010-10-12 17:41:18 PDT

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

WebKit Review Bot

Comment 25 2010-10-12 17:54:09 PDT

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

Note You need to log in before you can comment on or make changes to this bug.