Bug 43858 – Add support for BGRA pixel formats to GraphicsContext3D

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Stephen White]

Add support for BGRA pixel formats to GraphicsContext3D

[Stephen White]

Created attachment 64127 [details]
Patch

[James Robinson]

Comment on attachment 64127 [details]
Patch

> +    // FIXME:  This should be made pure virtual, once the chrome side change
> +    // has landed.
> +    virtual bool supportsBGRA() { return true; }

Land the chrome side (implementation) first and you won't have to do this.  You'll temporarily have an uncallable function definition in chromium land, but that's OK.

I think this looks fine.

[Stephen White]

(In reply to comment #2)
> (From update of attachment 64127 [details])
> > +    // FIXME:  This should be made pure virtual, once the chrome side change
> > +    // has landed.
> > +    virtual bool supportsBGRA() { return true; }
> 
> Land the chrome side (implementation) first and you won't have to do this.  You'll temporarily have an uncallable function definition in chromium land, but that's OK.

Good idea.  Done.

[Stephen White]

Created attachment 64172 [details]
Patch

[James Robinson]

I think you need implementations in GraphicsContext3DQt.cpp and GraphicsContext3DMac.mm or the builds on those platforms will break.  Otherwise, this looks good to me.

[Stephen White]

(In reply to comment #5)
> I think you need implementations in GraphicsContext3DQt.cpp and GraphicsContext3DMac.mm or the builds on those platforms will break.  Otherwise, this looks good to me.

There's an implementation in GraphicsContext3D.cpp for !PLATFORM(CHROMIUM).

[David Levin]

Comment on attachment 64172 [details]
Patch

r=me (I'm not setting r+ so that the ews bots will still run your patch through them, but consider this to be r+ by me).


Please make sure to rev DEPS (WebKit/chromium/DEPS) to pick up the necessary chromium change.

> Index: WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp
> +bool WebGraphicsContext3DDefaultImpl::supportsBGRA()
> +{
> +    // Supported since OpenGL 1.2.  However, glTexImage2D() must be modified
> +    // to translate the internalFormat from GL_BGRA to GL_RGBA, since the
> +    // former is not accepted by desktop GL.  Return false until this is done.

nit: WebKit uses single spaces after "."

[James Robinson]

Comment on attachment 64172 [details]
Patch

Chromium side's landed, flipping cq bit.

[WebKit Commit Bot]

Comment on attachment 64172 [details]
Patch

Clearing flags on attachment: 64172

Committed r65221: <http://trac.webkit.org/changeset/65221>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

This broke the snow leopard release build. Within minutes after it was submitted, this was noticeable.

[David Levin]

(In reply to comment #11)
> This broke the snow leopard release build. Within minutes after it was submitted, this was noticeable.

As jamesr pointed out, this is due to a missing return value here:
819 #if !PLATFORM(CHROMIUM)
 820 GraphicsContext3D::supportsBGRA()

Also, please consider fixing:
> Index: WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp
> +bool WebGraphicsContext3DDefaultImpl::supportsBGRA()
> +{
> +    // Supported since OpenGL 1.2.  However, glTexImage2D() must be modified
> +    // to translate the internalFormat from GL_BGRA to GL_RGBA, since the
> +    // former is not accepted by desktop GL.  Return false until this is done.

nit: WebKit uses single spaces after "."

r=me with these addressed (mostly the build break but the spaces issue would be nice).

[Stephen White]

Committed r65247: <http://trac.webkit.org/changeset/65247>

[WebKit Review Bot]

http://trac.webkit.org/changeset/65247 might have broken Leopard Intel Debug (Tests)