Bug 36262 – Add GraphicsContext3D abstraction to WebKit API

• Component: WebKit API
• Status: RESOLVED FIXED

[Kenneth Russell]

In order to run WebGL in Chrome's sandbox, GraphicsContext3D must call the command buffer implementation of the GLES 2 APIs rather than directly to OpenGL. Because this code lives in the Chrome repository, an abstraction for the GraphicsContext3D API needs to be added to the WebKit API and used by Chrome's GraphicsContext3D implementation.

[Kenneth Russell]

Created attachment 50992 [details]
Patch

ChangeLog message follows:

Added WebGraphicsContext3D to the WebKit API and refactored Chromium's GraphicsContext3D implementation to use it. All of the OpenGL calls have been moved out of WebKit/chromium/src/GraphicsContext3D.cpp and into the WebGraphicsContext3D implementation. GraphicsContext3D is still responsible for the transfer of rendered output from the WebGraphicsContext3D to the HTMLCanvasElement.

The GraphicsContext3DInternal class, which is a data member of GraphicsContext3D for the Chromium port, remains. It is possible to eliminate this class and thereby one level of delegation, but this is being deferred.

The needed entry point for a Chrome implementation of WebGraphicsContext3D has been added to WebKitClient, but it is not being called yet by GraphicsContext3D. It will be once this patch lands and Chromium is rolled forward to support this entry point.

This is a large patch, but the transformation is almost entirely mechanical and there is no change in functionality. Nearly all of GraphicsContext3D and GraphicsContext3DInternal has been moved to WebGraphicsContext3DDefaultImpl. The only area where the splitting of logic is less than mechanical is GraphicsContext3D::beginPaint() and its callees.

[WebKit Review Bot]

Attachment 50992 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.h:54:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:238:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:404:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:408:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:491:  One space before end of line comments  [whitespace/comments] [5]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:499:  One space before end of line comments  [whitespace/comments] [5]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:562:  One space before end of line comments  [whitespace/comments] [5]
WebKit/chromium/src/GraphicsContext3D.cpp:357:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/chromium/src/GraphicsContext3D.cpp:495:  One space before end of line comments  [whitespace/comments] [5]
WebKit/chromium/src/GraphicsContext3D.cpp:654:  TEXTURE_WRAP_R is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 10 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Russell]

Created attachment 51068 [details]
Revised patch

Responded to offline review feedback. Fixed ChangeLog and expanded documentation of WebGraphicsContext3D interface.

[Darin Adler]

Is this really the best way to do this?

[Darin Adler]

Classes like GraphicsContext and GraphicsContext3D are not intended to be stable API. We have to change them all the time. If Chrome has to do things this way, I suppose we can live with it, but it seems twisted and upside-down to do it this way.

[Kenneth Russell]

(In reply to comment #5)
> Classes like GraphicsContext and GraphicsContext3D are not intended to be
> stable API. We have to change them all the time. If Chrome has to do things
> this way, I suppose we can live with it, but it seems twisted and upside-down
> to do it this way.

It's necessary in order to make WebGL run within Chrome's sandbox; the OpenGL calls need to be remoted to another process. While I agree that GraphicsContext3D is supposed to be able to evolve rapidly, keep in mind that WebGraphicsContext3D is slightly lower level and its methods map almost verbatim to the OpenGL ES 2.0 API, which itself is stable.

[Darin Adler]

(In reply to comment #6)
> WebGraphicsContext3D is slightly lower level and its methods map almost
> verbatim to the OpenGL ES 2.0 API, which itself is stable.

Given that, I think WebGraphicsContext3D is probably not the right name for it.

[Kenneth Russell]

(In reply to comment #7)
> (In reply to comment #6)
> > WebGraphicsContext3D is slightly lower level and its methods map almost
> > verbatim to the OpenGL ES 2.0 API, which itself is stable.
> 
> Given that, I think WebGraphicsContext3D is probably not the right name for it.

The bulk of the GraphicsContext3D interface also maps nearly verbatim to OpenGL ES 2.0. We could call the new class something like WebOpenGLES2Context but that loses the naming connection to GraphicsContext3D. We are also adding a few entry points to WebGL that don't exist in OpenGL ES 2.0. Given the current naming conventions I prefer WebGraphicsContext3D.

[Darin Fisher (:fishd, Google)]

Given the similarity of this interface to OpenGL ES 2, I think we can tolerate exposing such a low-level API.  I would be more concerned if the interface were something likely to change frequently.  I'm happy with the name for the reasons Ken gives.

[Darin Fisher (:fishd, Google)]

Comment on attachment 51068 [details]
Revised patch

> Index: WebKit/chromium/public/WebGraphicsContext3D.h
...
> +    struct Attributes {
> +        Attributes()
> +                : alpha(true)
> +                , depth(true)
> +                , stencil(true)
> +                , antialias(true)
> +                , premultipliedAlpha(true)

nit: ^^^ indent by 4 spaces


> +    // Typedef for server-side objects like OpenGL textures and program objects.
> +    typedef unsigned int WebGLId;

nit: since this typedef has the Web prefix, I'd recommend just moving it
out of WebGraphicsContext3D and into the WebKit namespace.  anyways, it
seems like it might be convenient to not have to type WebGraphicsContext3D
in front of WebGLId in Chromium code.


> +    // Creates a "default" implementation of WebGraphicsContext3D which, in
> +    // Chromium, requires the sandbox to be disabled (strongly discouraged).
> +    static WebGraphicsContext3D* createDefault();

nit: we generally try to avoid references back to chromium specific things
since they can bit rot.  maybe it is enough to just say that this default
implementation uses OpenGL directly.


> +    // Initializes the graphics context; should be the first operation performed
> +    // on newly-constructed instances. Returns true on success.
> +    virtual bool initialize(Attributes attributes) = 0;

nit: webkit style is to leave off the parameter name when the parameter
name matches the parameter type (i.e., the name doesn't add information).


> +    // Helper for software compositing path. Reads back the frame buffer into
> +    // the memory region pointed to by "pixels". It is expected that the storage
> +    // for "pixels" covers (4 * width * height) bytes.
> +    virtual void readBackFramebuffer(unsigned char* pixels) = 0;

nit: perhaps we should have the caller specify a size_t parameter here so
that it will be possible for the callee to check it at runtime.  this call
is already quite expensive so such a runtime check is not going to be an
issue.  it will help avoid memory corruption issues perhaps.


> Index: WebKit/chromium/public/WebKitClient.h
...
> +    // May return null if WebGL is not supported.
> +    // Returns newly allocated WebGraphicsContext3D instance; caller must delete.
> +    virtual WebGraphicsContext3D* createGraphicsContext3D() { return 0; }

nit: The "createFoo" pattern implies "caller must delete", so it is okay
to leave that out of the comment.  but, redundancy is ok too.  i just wanted
to share that with you so you know the convention.


> Index: WebKit/chromium/src/GraphicsContext3D.cpp

> +// There are two levels of delegation in this file:
> +//
> +//   1. GraphicsContext3D delegates to GraphicsContext3DInternal. This is done
> +//      so that we have some place to store data members common among
> +//      implementations; GraphicsContext3D only provides us the m_internal
> +//      pointer. We always delegate to the GraphicsContext3DInternal. While we
> +//      could sidestep it and go directly to the WebGraphicsContext3D in some
> +//      cases, it is better for consistency to always delegate through it.
> +//
> +//   2. GraphicsContext3DInternal delegates to an implementation of
> +//      WebGraphicsContext3D. This is done so we have a place to inject our
> +//      command buffer implementation.
> +//
> +// The legacy, in-process, implementation uses WebGraphicsContext3DDefaultImpl.
> +// The command buffer implementation will use a to-be-decided class in Chrome.

nit: ^^^ again, it would be good to avoid comments that refer to Chrome like
this since they can get stale.  when that class is no longer to-be-decided,
will you remember to go back and revise this comment?  i would just leave off
the last sentence since point #2 basically implies the same thing.

R=me w/ nits addressed.

[Kenneth Russell]

(In reply to comment #10)
> (From update of attachment 51068 [details])
> > Index: WebKit/chromium/public/WebGraphicsContext3D.h
> ...
> > +    struct Attributes {
> > +        Attributes()
> > +                : alpha(true)
> > +                , depth(true)
> > +                , stencil(true)
> > +                , antialias(true)
> > +                , premultipliedAlpha(true)
> 
> nit: ^^^ indent by 4 spaces
> 
> 
> > +    // Typedef for server-side objects like OpenGL textures and program objects.
> > +    typedef unsigned int WebGLId;
> 
> nit: since this typedef has the Web prefix, I'd recommend just moving it
> out of WebGraphicsContext3D and into the WebKit namespace.  anyways, it
> seems like it might be convenient to not have to type WebGraphicsContext3D
> in front of WebGLId in Chromium code.

It turns out that calling code generally does not need to refer to this typedef, but it's fine to. Moved out of class.

> > +    // Creates a "default" implementation of WebGraphicsContext3D which, in
> > +    // Chromium, requires the sandbox to be disabled (strongly discouraged).
> > +    static WebGraphicsContext3D* createDefault();
> 
> nit: we generally try to avoid references back to chromium specific things
> since they can bit rot.  maybe it is enough to just say that this default
> implementation uses OpenGL directly.

Done.

> > +    // Initializes the graphics context; should be the first operation performed
> > +    // on newly-constructed instances. Returns true on success.
> > +    virtual bool initialize(Attributes attributes) = 0;
> 
> nit: webkit style is to leave off the parameter name when the parameter
> name matches the parameter type (i.e., the name doesn't add information).

Done.

> > +    // Helper for software compositing path. Reads back the frame buffer into
> > +    // the memory region pointed to by "pixels". It is expected that the storage
> > +    // for "pixels" covers (4 * width * height) bytes.
> > +    virtual void readBackFramebuffer(unsigned char* pixels) = 0;
> 
> nit: perhaps we should have the caller specify a size_t parameter here so
> that it will be possible for the callee to check it at runtime.  this call
> is already quite expensive so such a runtime check is not going to be an
> issue.  it will help avoid memory corruption issues perhaps.

Done.

> > Index: WebKit/chromium/public/WebKitClient.h
> ...
> > +    // May return null if WebGL is not supported.
> > +    // Returns newly allocated WebGraphicsContext3D instance; caller must delete.
> > +    virtual WebGraphicsContext3D* createGraphicsContext3D() { return 0; }
> 
> nit: The "createFoo" pattern implies "caller must delete", so it is okay
> to leave that out of the comment.  but, redundancy is ok too.  i just wanted
> to share that with you so you know the convention.

Done.

> > Index: WebKit/chromium/src/GraphicsContext3D.cpp
> 
> > +// There are two levels of delegation in this file:
> > +//
> > +//   1. GraphicsContext3D delegates to GraphicsContext3DInternal. This is done
> > +//      so that we have some place to store data members common among
> > +//      implementations; GraphicsContext3D only provides us the m_internal
> > +//      pointer. We always delegate to the GraphicsContext3DInternal. While we
> > +//      could sidestep it and go directly to the WebGraphicsContext3D in some
> > +//      cases, it is better for consistency to always delegate through it.
> > +//
> > +//   2. GraphicsContext3DInternal delegates to an implementation of
> > +//      WebGraphicsContext3D. This is done so we have a place to inject our
> > +//      command buffer implementation.
> > +//
> > +// The legacy, in-process, implementation uses WebGraphicsContext3DDefaultImpl.
> > +// The command buffer implementation will use a to-be-decided class in Chrome.
> 
> nit: ^^^ again, it would be good to avoid comments that refer to Chrome like
> this since they can get stale.  when that class is no longer to-be-decided,
> will you remember to go back and revise this comment?  i would just leave off
> the last sentence since point #2 basically implies the same thing.

Done.

[Kenneth Russell]

Created attachment 51189 [details]
Revised patch

Addressed review feedback.

[WebKit Review Bot]

Attachment 51189 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.h:80:  buffer_size is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:535:  buffer_size is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp:539:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/chromium/public/WebGraphicsContext3D.h:106:  buffer_size is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Russell]

Created attachment 51192 [details]
Revised patch

Fixed style errors introduced in last version of patch.

[WebKit Commit Bot]

Comment on attachment 51192 [details]
Revised patch

Clearing flags on attachment: 51192

Committed r56294: <http://trac.webkit.org/changeset/56294>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

Broke the canary: http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20(webkit.org)/builds/23679/steps/compile/logs/stdio_html

Please fix.

[Dimitri Glazkov (Google)]

Reverted r56294 for reason:

Broke compile on Chromium canaries.

Committed r56303: <http://trac.webkit.org/changeset/56303>

[Dimitri Glazkov (Google)]

Sorry guys, I am trying to switch over layout tests to use expectations upstream, so I had to roll this out in http://trac.webkit.org/changeset/56303.

[Kenneth Russell]

Created attachment 51344 [details]
Revised patch

Fixed compilation issues on Windows. Only changes compared to previous patch are: removal of reference to m_resizingBitmap from WebGraphicsContext3DDefaultImpl, reference to m_scanline from GraphicsContext3D.cpp and double free of m_resizingBitmap in destructor in GraphicsContext3D.cpp (bad merge). Tested on Windows.

[Darin Fisher (:fishd, Google)]

It seems like m_resizingBitmap could at least be an OwnPtr<SkBitmap>.  However, SkBitmap is itself internally reference counted.  It is in effect a smart pointer class.  Is there really need for the extra level of indirection?  Why not just make m_resizingBitmap be a SkBitmap?

[Kenneth Russell]

(In reply to comment #21)
> It seems like m_resizingBitmap could at least be an OwnPtr<SkBitmap>.  However,
> SkBitmap is itself internally reference counted.  It is in effect a smart
> pointer class.  Is there really need for the extra level of indirection?  Why
> not just make m_resizingBitmap be a SkBitmap?

There's no good reason. Will fix.

[Kenneth Russell]

Created attachment 51353 [details]
Revised patch

Made m_resizingBitmap an SkBitmap instead of SkBitmap*. Eliminates potential double free bugs. Rebuilt and retested on Windows.

[Kenneth Russell]

Changing status to assigned to see whether that will make the commit queue notice the latest patch.

[Eric Seidel (no email)]

The commit-queue is simply blocked behind failing tests at the moment:
http://webkit-commit-queue.appspot.com/

[Eric Seidel (no email)]

I just kicked it. It was a flaky SVG test which it last failed on.

[Eric Seidel (no email)]

bug 33433 was the test the commit-queue just failed on, hopefully it will pass it this time.

[WebKit Commit Bot]

Comment on attachment 51353 [details]
Revised patch

Clearing flags on attachment: 51353

Committed r56381: <http://trac.webkit.org/changeset/56381>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.