Bug 43804 – [CHROMIUM] Use the BGRA format for canvas 2D accel upload and readback.

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

[Stephen White]

[CHROMIUM] Use the BGRA format for canvas 2D accel upload and readback.

[Stephen White]

Created attachment 64039 [details]
Patch

[James Robinson]

Comment on attachment 64039 [details]
Patch

> Index: WebCore/platform/graphics/chromium/GLES2Texture.cpp
> +    case GLES2Texture::BGRA8:
> +        if (true) { // FIXME:  check for BGRA extension support
> +            *glFormat = 0x80E1;
> +            *glType = GraphicsContext3D::UNSIGNED_BYTE;
> +        } else {
> +            *glFormat = GraphicsContext3D::RGBA;
> +            *glType = GraphicsContext3D::UNSIGNED_BYTE;
> +            *swizzle = true;

Don't keep dead code around.  If we need this later (which I think we will) we can get it out of SVN history.

>  void PlatformContextSkia::readbackHardwareToSoftware() const
> +        // FIXME:  Check for BGRA extension support
> +        if (true)
> +            context->readPixels(0, height - 1 - y, width, 1, 0x80E1, GraphicsContext3D::UNSIGNED_BYTE, pixels);
> +        else {
> +            context->readPixels(0, height - 1 - y, width, 1, GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, pixels);
> +            for (int i = 0; i < width; ++i) {
> +                uint32_t pixel = pixels[i];
> +                // Swizzles from RGBA -> BGRA.
> +                pixels[i] = pixel & 0xFF00FF00 | ((pixel & 0x00FF0000) >> 16) | ((pixel & 0x000000FF) << 16);
> +            }
>          }

Same here - keep the FIXME but nuke the unreachable code.

[Stephen White]

(In reply to comment #2)
> (From update of attachment 64039 [details])
> > Index: WebCore/platform/graphics/chromium/GLES2Texture.cpp
> > +    case GLES2Texture::BGRA8:
> > +        if (true) { // FIXME:  check for BGRA extension support
> > +            *glFormat = 0x80E1;
> > +            *glType = GraphicsContext3D::UNSIGNED_BYTE;
> > +        } else {
> > +            *glFormat = GraphicsContext3D::RGBA;
> > +            *glType = GraphicsContext3D::UNSIGNED_BYTE;
> > +            *swizzle = true;
> 
> Don't keep dead code around.  If we need this later (which I think we will) we can get it out of SVN history.

In that case, I'll see about landing the change to GraphicsContext3D first.  We'll definitely need this code later.

> >  void PlatformContextSkia::readbackHardwareToSoftware() const
> > +        // FIXME:  Check for BGRA extension support
> > +        if (true)
> > +            context->readPixels(0, height - 1 - y, width, 1, 0x80E1, GraphicsContext3D::UNSIGNED_BYTE, pixels);
> > +        else {
> > +            context->readPixels(0, height - 1 - y, width, 1, GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, pixels);
> > +            for (int i = 0; i < width; ++i) {
> > +                uint32_t pixel = pixels[i];
> > +                // Swizzles from RGBA -> BGRA.
> > +                pixels[i] = pixel & 0xFF00FF00 | ((pixel & 0x00FF0000) >> 16) | ((pixel & 0x000000FF) << 16);
> > +            }
> >          }
> 
> Same here - keep the FIXME but nuke the unreachable code.

[Stephen White]

Created attachment 64233 [details]
Patch

[James Robinson]

Looks good to me!

[David Levin]

Comment on attachment 64233 [details]
Patch


> Index: WebCore/platform/graphics/chromium/GLES2Texture.cpp
> ===================================================================
> --- WebCore/platform/graphics/chromium/GLES2Texture.cpp	(revision 65251)
> +++ WebCore/platform/graphics/chromium/GLES2Texture.cpp	(working copy)
> @@ -54,6 +54,30 @@ GLES2Texture::~GLES2Texture()
>      m_context->deleteTexture(m_textureId);
>  }
>  
> +static void convertFormat(GraphicsContext3D* context, GLES2Texture::Format format, unsigned int* glFormat, unsigned int* glType, bool* swizzle)
..
> +    default:
> +        ASSERT(!"bad format");

This is odd, but I see it existed before. Anyway please consider using ASSERT_NOT_REACHED(); instead.

(Also if the list of enum is short, it is considered preferable to list them and remove the default to allow the compiler to flag enums not being handled.)

> +        break;
> +    }
> +}
> +

[Stephen White]

Committed r65323: <http://trac.webkit.org/changeset/65323>