Bug 43804

Summary: [CHROMIUM] Use the BGRA format for canvas 2D accel upload and readback.
Product: WebKit Reporter: Stephen White <senorblanco>
Component: New BugsAssignee: Stephen White <senorblanco>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fishd, jamesr
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 43858    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch levin: review+

Description Stephen White 2010-08-10 12:21:17 PDT
[CHROMIUM] Use the BGRA format for canvas 2D accel upload and readback.
Comment 1 Stephen White 2010-08-10 12:25:33 PDT
Created attachment 64039 [details]
Patch
Comment 2 James Robinson 2010-08-10 13:28:12 PDT
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.
Comment 3 Stephen White 2010-08-10 14:59:13 PDT
(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.
Comment 4 Stephen White 2010-08-12 10:08:07 PDT
Created attachment 64233 [details]
Patch
Comment 5 James Robinson 2010-08-12 10:36:40 PDT
Looks good to me!
Comment 6 David Levin 2010-08-12 13:44:50 PDT
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;
> +    }
> +}
> +
Comment 7 Stephen White 2010-08-13 06:59:33 PDT
Committed r65323: <http://trac.webkit.org/changeset/65323>