| Summary: | Define GC3D types to match GL types and use them in GraphicsContext3D | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kenneth Russell <kbr> | ||||||
| Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, cmarrin, enne, eric, jamesr, webkit.review.bot, zmo | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 51908, 51947 | ||||||||
| Attachments: |
|
||||||||
|
Description
Kenneth Russell
2010-09-10 12:16:27 PDT
(In reply to comment #0) > There are uses of the type "long" scattered throughout the WebGLRenderingContext and GraphicsContext3D classes. I believe this originated from a misunderstanding of how the type "long" in WebKit IDL maps to a C integer type. In fact, "long" in this IDL maps to a 32-bit integer type, which also maps well to the 32-bit integer values that are used in the OpenGL APIs in these classes. All of the uses of "long" in these classes and in referencing code should be changed to "int", and "unsigned long" to "unsigned". If you're planning on making type changes, you should make sure there are not some better types to use, like int32_t and uint32_t. int, unsigned, etc. are no more well defined cross-platform than long. Let's make sure we're using types that best match the IDL, so we don't find ourselves doing this work twice. Also, we use several long types in functions and members for WebGLObject classes. Need to update them too. In GraphicsContext3D, we define GC3D types (for example, GC3Dsizei) to match GL types. Then we can use them in both GraphicsContext3D and WebGLRenderingContext. This cleanup will make the type mapping happening in one place and make it very easy to change in the future if needed. Also, we should double check WebGLRenderingContext.idl to make sure types are correct in it. Break this bug into two: one for GraphicsContext3D, and the other for WebGLRenderingContext and related WebGL classes. GraphicsContext3D needs to deal with multiple platforms, whereas WebGLRenderingContext needs to deal with conflicts with other patches. Besides, the change will be really large, so it seems reasonable to de-couple them into two. Created attachment 78030 [details]
Patch
Comment on attachment 78030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78030&action=review This cleanup mostly looks good and I definitely think it's the right thing to do. While it's huge it will make maintaining this code easier in the future. As we discussed, the style errors are because arguments have names in the headers, but because in OpenGL there's so much information encoded in those names, I think they should be kept despite the style violations. There are a few changes that need to be made. > WebCore/html/canvas/WebGLRenderingContext.cpp:2333 > - unsigned long componentsPerPixel, bytesPerComponent; > + unsigned int componentsPerPixel, bytesPerComponent; See below about using one of the new typedefs here. > WebCore/html/canvas/WebGLRenderingContext.cpp:3859 > - unsigned long componentsPerPixel, bytesPerComponent; > + unsigned int componentsPerPixel, bytesPerComponent; Here as well. > WebCore/platform/graphics/GraphicsContext3D.cpp:46 > + unsigned int bytesPerComponent(GC3Denum type) Use one of the new typedefs for the return type; specifically, GC3Duint. > WebCore/platform/graphics/GraphicsContext3D.cpp:62 > + unsigned int componentsPerPixel(GC3Denum format, GC3Denum type) Return type GC3Duint. > WebCore/platform/graphics/GraphicsContext3D.cpp:90 > + unsigned int imageSizeInBytes(GC3Dsizei width, GC3Dsizei height, GC3Denum format, GC3Denum type) Use GC3Duint as the return type. > WebCore/platform/graphics/GraphicsContext3D.cpp:142 > + uint32_t* componentsPerPixel, > + uint32_t* bytesPerComponent) Use one of your new typedefs instead of uint32_t; specifically, GC3Duint. > WebCore/platform/graphics/GraphicsContext3D.h:43 > +typedef void GC3Dvoid; I don't think the GC3Dvoid typedef adds any value. The gl2.h header doesn't reference its own GLvoid typedef. I would recommend just removing it and continuing to use const void* for blocks of data throughout. > WebCore/platform/graphics/GraphicsContext3D.h:52 > +typedef signed long int GC3Dsizeiptr; For completeness add GC3Dintptr. > WebCore/platform/graphics/GraphicsContext3D.h:606 > + void bufferData(GC3Denum target, GC3Dsizei size, GC3Denum usage); > + void bufferData(GC3Denum target, GC3Dsizei size, const GC3Dvoid* data, GC3Denum usage); To match the gl2.h header, the GC3Dsizei uses here should be GC3Dsizeiptr. > WebCore/platform/graphics/GraphicsContext3D.h:607 > + void bufferSubData(GC3Denum target, GC3Dsizeiptr offset, GC3Dsizei size, const GC3Dvoid* data); To match gl2.h, offset should be a GC3Dintptr, and size should be a GC3Dsizeiptr. > WebCore/platform/graphics/GraphicsContext3D.h:630 > + void drawElements(GC3Denum mode, GC3Dsizei count, GC3Denum type, GC3Dsizeiptr offset); I think offset should be a GC3Dintptr. > WebCore/platform/graphics/GraphicsContext3D.h:700 > + bool texImage2D(GC3Denum target, GC3Dint level, GC3Denum internalformat, GC3Dsizei width, GC3Dsizei height, GC3Dint border, GC3Denum format, GC3Denum type, GC3Dvoid* pixels); I note that gl2.h uses GLint for internalformat but I think your choice of GC3Denum is better. Should we take this opportunity to add "const" to the pixels argument? > WebCore/platform/graphics/GraphicsContext3D.h:703 > + void texSubImage2D(GC3Denum target, GC3Dint level, GC3Dint xoffset, GC3Dint yoffset, GC3Dsizei width, GC3Dsizei height, GC3Denum format, GC3Denum type, GC3Dvoid* pixels); Same question about making pixels const void*. > WebCore/platform/graphics/GraphicsContext3D.h:723 > + void uniform1f(GC3Dint location, GC3Dfloat x); > + void uniform1fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size); > + void uniform1i(GC3Dint location, GC3Dint x); > + void uniform1iv(GC3Dint location, GC3Dint* v, GC3Dsizei size); > + void uniform2f(GC3Dint location, GC3Dfloat x, float y); > + void uniform2fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size); > + void uniform2i(GC3Dint location, GC3Dint x, GC3Dint y); > + void uniform2iv(GC3Dint location, GC3Dint* v, GC3Dsizei size); > + void uniform3f(GC3Dint location, GC3Dfloat x, GC3Dfloat y, GC3Dfloat z); > + void uniform3fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size); > + void uniform3i(GC3Dint location, GC3Dint x, GC3Dint y, GC3Dint z); > + void uniform3iv(GC3Dint location, GC3Dint* v, GC3Dsizei size); > + void uniform4f(GC3Dint location, GC3Dfloat x, GC3Dfloat y, GC3Dfloat z, GC3Dfloat w); > + void uniform4fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size); > + void uniform4i(GC3Dint location, GC3Dint x, GC3Dint y, GC3Dint z, GC3Dint w); > + void uniform4iv(GC3Dint location, GC3Dint* v, GC3Dsizei size); > + void uniformMatrix2fv(GC3Dint location, GC3Dboolean transpose, GC3Dfloat* value, GC3Dsizei size); > + void uniformMatrix3fv(GC3Dint location, GC3Dboolean transpose, GC3Dfloat* value, GC3Dsizei size); > + void uniformMatrix4fv(GC3Dint location, GC3Dboolean transpose, GC3Dfloat* value, GC3Dsizei size); Could you add a FIXME about changing the argument order to these methods to match OpenGL's? The discrepancy is historical and there's no good reason for it. > WebCore/platform/graphics/GraphicsContext3D.h:737 > + GC3Dsizei stride, GC3Dsizeiptr offset); As above I think offset should be GC3Dintptr. Thanks for reviewing this huge and boring patch! (In reply to comment #6) > (From update of attachment 78030 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78030&action=review > > This cleanup mostly looks good and I definitely think it's the right thing to do. While it's huge it will make maintaining this code easier in the future. As we discussed, the style errors are because arguments have names in the headers, but because in OpenGL there's so much information encoded in those names, I think they should be kept despite the style violations. There are a few changes that need to be made. > > > WebCore/html/canvas/WebGLRenderingContext.cpp:2333 > > - unsigned long componentsPerPixel, bytesPerComponent; > > + unsigned int componentsPerPixel, bytesPerComponent; > > See below about using one of the new typedefs here. My strategy is to restrict the using of CG3D types to GL functions only. For bytePerComponent (and a few others), since input parameter type if directly connected to gl type, I use GC3Denum; however, the returned value is not directly connected with GL parameters, then I use a regular type (although I try to avoid using long). For the same reasoning, I didn't touch packPixels and all the helper functions for it. Let me know if you disagree with my strategy here. > > > WebCore/html/canvas/WebGLRenderingContext.cpp:3859 > > - unsigned long componentsPerPixel, bytesPerComponent; > > + unsigned int componentsPerPixel, bytesPerComponent; > > Here as well. > > > WebCore/platform/graphics/GraphicsContext3D.cpp:46 > > + unsigned int bytesPerComponent(GC3Denum type) > > Use one of the new typedefs for the return type; specifically, GC3Duint. > > > WebCore/platform/graphics/GraphicsContext3D.cpp:62 > > + unsigned int componentsPerPixel(GC3Denum format, GC3Denum type) > > Return type GC3Duint. > > > WebCore/platform/graphics/GraphicsContext3D.cpp:90 > > + unsigned int imageSizeInBytes(GC3Dsizei width, GC3Dsizei height, GC3Denum format, GC3Denum type) > > Use GC3Duint as the return type. > > > WebCore/platform/graphics/GraphicsContext3D.cpp:142 > > + uint32_t* componentsPerPixel, > > + uint32_t* bytesPerComponent) > > Use one of your new typedefs instead of uint32_t; specifically, GC3Duint. > > > WebCore/platform/graphics/GraphicsContext3D.h:43 > > +typedef void GC3Dvoid; > > I don't think the GC3Dvoid typedef adds any value. The gl2.h header doesn't reference its own GLvoid typedef. I would recommend just removing it and continuing to use const void* for blocks of data throughout. > > > WebCore/platform/graphics/GraphicsContext3D.h:52 > > +typedef signed long int GC3Dsizeiptr; > > For completeness add GC3Dintptr. > > > WebCore/platform/graphics/GraphicsContext3D.h:606 > > + void bufferData(GC3Denum target, GC3Dsizei size, GC3Denum usage); > > + void bufferData(GC3Denum target, GC3Dsizei size, const GC3Dvoid* data, GC3Denum usage); > > To match the gl2.h header, the GC3Dsizei uses here should be GC3Dsizeiptr. In webgl spec, bufferData function signature, GLsizei is used. This is to match the webgl spec. Let me know if you think the webgl spec should be fixed. > > > WebCore/platform/graphics/GraphicsContext3D.h:607 > > + void bufferSubData(GC3Denum target, GC3Dsizeiptr offset, GC3Dsizei size, const GC3Dvoid* data); > > To match gl2.h, offset should be a GC3Dintptr, and size should be a GC3Dsizeiptr. Same here, try to match the webgl spec. > > > WebCore/platform/graphics/GraphicsContext3D.h:630 > > + void drawElements(GC3Denum mode, GC3Dsizei count, GC3Denum type, GC3Dsizeiptr offset); > > I think offset should be a GC3Dintptr. Same here. > > > WebCore/platform/graphics/GraphicsContext3D.h:700 > > + bool texImage2D(GC3Denum target, GC3Dint level, GC3Denum internalformat, GC3Dsizei width, GC3Dsizei height, GC3Dint border, GC3Denum format, GC3Denum type, GC3Dvoid* pixels); > > I note that gl2.h uses GLint for internalformat but I think your choice of GC3Denum is better. Again, webgl spec says GLenum. Just want to point this out. > > Should we take this opportunity to add "const" to the pixels argument? Will do. > > > WebCore/platform/graphics/GraphicsContext3D.h:703 > > + void texSubImage2D(GC3Denum target, GC3Dint level, GC3Dint xoffset, GC3Dint yoffset, GC3Dsizei width, GC3Dsizei height, GC3Denum format, GC3Denum type, GC3Dvoid* pixels); > > Same question about making pixels const void*. > > > WebCore/platform/graphics/GraphicsContext3D.h:723 > > + void uniform1f(GC3Dint location, GC3Dfloat x); > > + void uniform1fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size); > > + void uniform1i(GC3Dint location, GC3Dint x); > > + void uniform1iv(GC3Dint location, GC3Dint* v, GC3Dsizei size); > > + void uniform2f(GC3Dint location, GC3Dfloat x, float y); > > + void uniform2fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size); > > + void uniform2i(GC3Dint location, GC3Dint x, GC3Dint y); > > + void uniform2iv(GC3Dint location, GC3Dint* v, GC3Dsizei size); > > + void uniform3f(GC3Dint location, GC3Dfloat x, GC3Dfloat y, GC3Dfloat z); > > + void uniform3fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size); > > + void uniform3i(GC3Dint location, GC3Dint x, GC3Dint y, GC3Dint z); > > + void uniform3iv(GC3Dint location, GC3Dint* v, GC3Dsizei size); > > + void uniform4f(GC3Dint location, GC3Dfloat x, GC3Dfloat y, GC3Dfloat z, GC3Dfloat w); > > + void uniform4fv(GC3Dint location, GC3Dfloat* v, GC3Dsizei size); > > + void uniform4i(GC3Dint location, GC3Dint x, GC3Dint y, GC3Dint z, GC3Dint w); > > + void uniform4iv(GC3Dint location, GC3Dint* v, GC3Dsizei size); > > + void uniformMatrix2fv(GC3Dint location, GC3Dboolean transpose, GC3Dfloat* value, GC3Dsizei size); > > + void uniformMatrix3fv(GC3Dint location, GC3Dboolean transpose, GC3Dfloat* value, GC3Dsizei size); > > + void uniformMatrix4fv(GC3Dint location, GC3Dboolean transpose, GC3Dfloat* value, GC3Dsizei size); > > Could you add a FIXME about changing the argument order to these methods to match OpenGL's? The discrepancy is historical and there's no good reason for it. Will do. > > > WebCore/platform/graphics/GraphicsContext3D.h:737 > > + GC3Dsizei stride, GC3Dsizeiptr offset); > > As above I think offset should be GC3Dintptr. Same here, try to match webgl spec. (In reply to comment #7) > My strategy is to restrict the using of CG3D types to GL functions only. For bytePerComponent (and a few others), since input parameter type if directly connected to gl type, I use GC3Denum; however, the returned value is not directly connected with GL parameters, then I use a regular type (although I try to avoid using long). > > For the same reasoning, I didn't touch packPixels and all the helper functions for it. > > Let me know if you disagree with my strategy here. I see. That sounds okay. > > > WebCore/platform/graphics/GraphicsContext3D.h:606 > > > + void bufferData(GC3Denum target, GC3Dsizei size, GC3Denum usage); > > > + void bufferData(GC3Denum target, GC3Dsizei size, const GC3Dvoid* data, GC3Denum usage); > > > > To match the gl2.h header, the GC3Dsizei uses here should be GC3Dsizeiptr. > > In webgl spec, bufferData function signature, GLsizei is used. This is to match the webgl spec. Let me know if you think the webgl spec should be fixed. GraphicsContext3D is supposed to mirror the OpenGL ES 2.0 API, not WebGL specifically. I think it would be clearer if we added the missing typedefs to GC3D and made the signatures look as much like the ES 2.0 ones as possible. We should probably consider updating the WebGL spec to fix up the discrepancies; as we discussed offline, let's pursue that in parallel. > > > WebCore/platform/graphics/GraphicsContext3D.h:607 > > > + void bufferSubData(GC3Denum target, GC3Dsizeiptr offset, GC3Dsizei size, const GC3Dvoid* data); > > > > To match gl2.h, offset should be a GC3Dintptr, and size should be a GC3Dsizeiptr. > > Same here, try to match the webgl spec. See above. > > > > > WebCore/platform/graphics/GraphicsContext3D.h:630 > > > + void drawElements(GC3Denum mode, GC3Dsizei count, GC3Denum type, GC3Dsizeiptr offset); > > > > I think offset should be a GC3Dintptr. > > Same here. See above. > > > > > WebCore/platform/graphics/GraphicsContext3D.h:700 > > > + bool texImage2D(GC3Denum target, GC3Dint level, GC3Denum internalformat, GC3Dsizei width, GC3Dsizei height, GC3Dint border, GC3Denum format, GC3Denum type, GC3Dvoid* pixels); > > > > I note that gl2.h uses GLint for internalformat but I think your choice of GC3Denum is better. > > Again, webgl spec says GLenum. Just want to point this out. In this particular case I think we should use GLenum. The ES 2.0 spec seems wrong. > > > WebCore/platform/graphics/GraphicsContext3D.h:737 > > > + GC3Dsizei stride, GC3Dsizeiptr offset); > > > > As above I think offset should be GC3Dintptr. > > Same here, try to match webgl spec. See above. In the upcoming patch, below is a list of changes from the previous patch 1) removed GC3Dvoid type. Use void instead. 2) Change void* to const void* for texImage2D and texSubImage2D 3) bufferData/bufferSubData: size's type changed from GC3Dsizei to GC3Dsizeiptr 4) bufferSubData/drawElements/vertexAttribPointer: offset's type changed from GC3Dsizeiptr to GC3Dintptr. Hope that could save some time reviewing the changes. Created attachment 78190 [details]
Patch
(In reply to comment #9) > In the upcoming patch, below is a list of changes from the previous patch > > 1) removed GC3Dvoid type. Use void instead. > 2) Change void* to const void* for texImage2D and texSubImage2D > 3) bufferData/bufferSubData: size's type changed from GC3Dsizei to GC3Dsizeiptr > 4) bufferSubData/drawElements/vertexAttribPointer: offset's type changed from GC3Dsizeiptr to GC3Dintptr. > > Hope that could save some time reviewing the changes. Forget to mention 5) Add a FIXME for uniformMatrix function signatures. Comment on attachment 78190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78190&action=review Thanks for doing this tedious cleanup. It will ease maintenance of this code in the future. Only one tiny issue, can be fixed upon landing. > WebCore/platform/graphics/GraphicsContext3D.h:722 > + // FIXME: change the argument orders to match OpenGL's. This FIXME should actually be moved above the call of uniform1f. All of the uniform*v entry points have the wrong argument order. Committed r75214: <http://trac.webkit.org/changeset/75214> http://trac.webkit.org/changeset/75214 might have broken GTK Linux 32-bit Debug |