Summary: | [Texmap] Implement BGRA swizzling detection | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Hausmann <hausmann> | ||||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | igor.oliveira, jturcotte, noam, ostap73, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Simon Hausmann
2012-03-14 07:25:55 PDT
From http://www.khronos.org/opengles/sdk/docs/man/xhtml/glTexSubImage2D.xml it appears that the format passed to glTexSubImage2D is supposed to be the same: "GL_INVALID_OPERATION is generated if the texture array has not been defined by a previous glTexImage2D or glCopyTexImage2D operation whose internalformat matches the format of glTexSubImage2D." Another interesting point: <k5kangas> You might also want to quote OpenGLES2 spec, v2.0.25, page 72, "Each of the TexSubImage commands <k5kangas> interprets and processes pixel groups in exactly the manner of its TexImage coun- <k5kangas> terpart, except that the assignment of R, G, B, and A pixel group values to the <k5kangas> texture components is controlled by the internalformat of the texture array, not <k5kangas> by an argument to the command. (In reply to comment #3) > Another interesting point: > > <k5kangas> You might also want to quote OpenGLES2 spec, v2.0.25, page 72, "Each of the TexSubImage commands > <k5kangas> interprets and processes pixel groups in exactly the manner of its TexImage coun- > <k5kangas> terpart, except that the assignment of R, G, B, and A pixel group values to the > <k5kangas> texture components is controlled by the internalformat of the texture array, not > <k5kangas> by an argument to the command. Interesting. I wonder why this is the only thing that had worked for desktop and Harmattan then :) Created attachment 186167 [details]
Patch
Proposed patch
Comment on attachment 186167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186167&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:589 > + return context->getExtensions()->supports("GL_EXT_texture_format_BGRA8888"); It is called so many times. I doubt that swizzling support can change during app execution and would cache result here. Something like this: static bool canSwizzle = context->getExtensions()->supports("GL_EXT_texture_format_BGRA8888"); return canSwizzle; Created attachment 186175 [details]
Patch V2
Proposed patch.
Created attachment 186178 [details]
Patch v3
Fix left over from other patch.
Comment on attachment 186178 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=186178&action=review I remember this kind of patch had a tendency to break stuff :) Can someone validate it on more than one GLES2 hardware? > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:591 > #if defined(TEXMAP_OPENGL_ES_2) > - // FIXME: Implement reliable detection. See also https://bugs.webkit.org/show_bug.cgi?id=81103. > - return false; > + static bool supportsExternalTextureBGRA = context->getExtensions()->supports("GL_EXT_texture_format_BGRA8888"); > + return supportsExternalTextureBGRA; > #else I think we shoul switch here to isGLES2Compliant() instead of an #ifdef (In reply to comment #9) > (From update of attachment 186178 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186178&action=review > > I remember this kind of patch had a tendency to break stuff :) > Can someone validate it on more than one GLES2 hardware? I tested in mesa3d llvmpipe, intel and mali t400. The reverted implementation(changeset r110703) was slightly different. Basically it was just using BGRA in the external format, however in the GLESv2 case, when BGRA is used it also needs to be used in the internal texture format. > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:591 > > #if defined(TEXMAP_OPENGL_ES_2) > > - // FIXME: Implement reliable detection. See also https://bugs.webkit.org/show_bug.cgi?id=81103. > > - return false; > > + static bool supportsExternalTextureBGRA = context->getExtensions()->supports("GL_EXT_texture_format_BGRA8888"); > > + return supportsExternalTextureBGRA; > > #else > > I think we shoul switch here to isGLES2Compliant() instead of an #ifdef (In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 186178 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=186178&action=review > > > > I remember this kind of patch had a tendency to break stuff :) > > Can someone validate it on more than one GLES2 hardware? > > I tested in mesa3d llvmpipe, intel and mali t400. > The reverted implementation(changeset r110703) was slightly different. Basically it was just using BGRA in the external format, however in the GLESv2 case, when BGRA is used it also needs to be used in the internal texture format. Yes, in the N9 case that's the only thing that worked (and N9 was the only GLESv2 device running TextureMapper at the time). But I think this should be ok, if you can address the other comment :) Comment on attachment 186178 [details]
Patch v3
See previous comment.
Created attachment 186427 [details]
Patch
Patch for landing.
Comment on attachment 186427 [details] Patch Clearing flags on attachment: 186427 Committed r141807: <http://trac.webkit.org/changeset/141807> All reviewed patches have been landed. Closing bug. |