Last Comment Bug 757526 - Use stdint instead of PRInt types in WebGL implementation
: Use stdint instead of PRInt types in WebGL implementation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on: 766796
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-22 10:47 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-06-21 02:43 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix integer types in WebGL (70.24 KB, patch)
2012-05-22 10:47 PDT, Benoit Jacob [:bjacob] (mostly away)
Ms2ger: review+
Details | Diff | Splinter Review
updated patch for landing (71.80 KB, patch)
2012-05-22 13:45 PDT, Benoit Jacob [:bjacob] (mostly away)
jacob.benoit.1: review+
landry: feedback+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-05-22 10:47:24 PDT
Created attachment 626085 [details] [diff] [review]
fix integer types in WebGL

The tricky part is that XPIDL still uses PRInt types. Fortunately, in WebGL's case, this is easy to isolate as WebGL functions taking integer parameters take them as special typedefs like WebGLint, WebGLenum, WebGLsizei, etc, and we want to keep using these typedefs anyway, so it doesn't bother me too much that these typedefs really remain typedefs for PRInt types.

Found and fixed a couple mis-uses of integer types along the way.

On tryserver:
https://tbpl.mozilla.org/?tree=Try&rev=e68b3faa9f58
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-05-22 11:05:03 PDT
Comment on attachment 626085 [details] [diff] [review]
fix integer types in WebGL

Review of attachment 626085 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not terribly impressed by the WebGL* typedefs, but that's a fight for another day.

If this builds on try and BSD, I'm all for.

::: content/canvas/src/WebGLContext.h
@@ +1128,5 @@
>      bool mShaderValidation;
>  
>      // some GL constants
> +    int32_t mGLMaxVertexAttribs;
> +    int32_t mGLMaxTextureUnits;

There's a |for (int i = 0; i < mGLMaxTextureUnits; i++) {| you should fix too

@@ +1175,5 @@
>  
>      nsTArray<WebGLenum> mCompressedTextureFormats;
>  
>      bool InitAndValidateGL();
> +    bool ValidateBuffers(int32_t* maxAllowedCount, const char *info);

Inconsistent side for the *

@@ +1225,5 @@
>      void TexParameter_base(WebGLenum target, WebGLenum pname,
>                             WebGLint *intParamPtr, WebGLfloat *floatParamPtr);
>  
>      void ConvertImage(size_t width, size_t height, size_t srcStride, size_t dstStride,
> +                      const uint8_t*src, uint8_t *dst,

Add a space

@@ +1532,5 @@
>      }
>  
>      // this method too is only for element array buffers. It returns the maximum value in the part of
>      // the buffer starting at given offset, consisting of given count of elements. The type T is the type
>      // to interprete the array elements as, must be GLushort or GLubyte.

interpret

@@ +1539,1 @@
>      {

Shouldn't this return T?

::: content/canvas/src/WebGLContextGL.cpp
@@ +438,5 @@
>      }
>  }
>  
>  NS_IMETHODIMP
> +WebGLContext::BufferData(GLenum target, const JS::Value& data, GLenum usage,

GLenum or WebGLenum?

@@ +2253,5 @@
>  
>  NS_IMETHODIMP
>  WebGLContext::GetAttribLocation(nsIWebGLProgram *pobj,
>                                  const nsAString& name,
> +                                int32_t *retval)

This builds on Windows?

@@ +3962,5 @@
>  
>          if (needAlphaFixup) {
>              if (format == LOCAL_GL_ALPHA && type == LOCAL_GL_UNSIGNED_BYTE) {
>                  // this is easy; it's an 0xff memset per row
> +                uint8_t *row = (uint8_t*)data;

static_cast

@@ +3969,5 @@
>                      row += checked_alignedRowSize.value();
>                  }
>              } else if (format == LOCAL_GL_RGBA && type == LOCAL_GL_UNSIGNED_BYTE) {
>                  // this is harder, we need to just set the alpha byte here
> +                uint8_t *row = (uint8_t*)data;

And here

@@ +4498,5 @@
>                                   arrayLength);                                  \
>      }                                                                           \
>      if (aTranspose) {                                                           \
>          return ErrorInvalidValue(#name ": transpose must be FALSE as per the "  \
>                                   "OpenGL ES 2.0 spec");                         \

(Ridiculous)

@@ +4894,5 @@
>          // 7-bit ASCII range, so we can skip the NS_IsAscii() check.
>          const nsCString& sourceCString = NS_LossyConvertUTF16toASCII(flatSource);
>  
>          if (gl->WorkAroundDriverBugs()) {
> +            const uint32_t maxSourceLength = (uint32_t(1)<<18) - 1;

Add spaces around <<

@@ +5611,2 @@
>              ConvertImage(width, height, srcStride, dstStride,
> +                        (uint8_t*)data, convertedData,

static_cast

@@ +5856,2 @@
>          ConvertImage(width, height, srcStride, dstStride,
> +                    (const uint8_t*)pixels, convertedData,

static_cast
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-05-22 13:45:58 PDT
Created attachment 626165 [details] [diff] [review]
updated patch for landing

Applied review comments.

On try:
https://tbpl.mozilla.org/?tree=Try&rev=e1bc59211166
Comment 3 Landry Breuil (:gaston) 2012-05-22 14:02:03 PDT
Comment on attachment 626165 [details] [diff] [review]
updated patch for landing

I only had a single failure with it (but my build is not complete due to other unrelated errors)

/home/landry/src/mozilla-central/content/canvas/src/WebGLContextReporter.cpp:29: error: prototype for 'nsresult WebGLMemoryMultiReporter::GetExplicitNonHeap(int64_t*)' does not match any in class 'WebGLMemoryMultiReporter'
/home/landry/src/mozilla-central/content/canvas/src/WebGLContextReporter.cpp:16: error: candidate is: virtual nsresult WebGLMemoryMultiReporter::GetExplicitNonHeap(PRInt64*)

Reverting the first chunk of WebGLContextReporter.cpp made the error go away.

In the future don't hesitate to feedback? me on such patches, i'll gladly try them before they land (instead of filing followup bugs when it hits my buildbot :)
Comment 4 Landry Breuil (:gaston) 2012-05-22 14:20:12 PDT
Oops, my feedback was about att #626085, i'll retry with #626165 in a few..
Comment 5 Landry Breuil (:gaston) 2012-05-22 14:51:44 PDT
Seems to build fine too with att #626165 (with WebGLContextReporter.cpp 'fixed')
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-05-23 09:20:47 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd6c4f6a2448

With the WebGLContextReporter.cpp fix --- thanks Landry, that was a real bug.
Comment 7 Ed Morley [:emorley] 2012-05-24 10:54:07 PDT
https://hg.mozilla.org/mozilla-central/rev/dd6c4f6a2448

Note You need to log in before you can comment on or make changes to this bug.