757526 - Use stdint instead of PRInt types in WebGL implementation

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

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)]

Attachment: updated patch for landing

Applied review comments.

On try:
https://tbpl.mozilla.org/?tree=Try&rev=e1bc59211166

Comment 3

[Landry Breuil (:gaston)]

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)]

Oops, my feedback was about att #626085, i'll retry with #626165 in a few..

Comment 5

[Landry Breuil (:gaston)]

Seems to build fine too with att #626165 (with WebGLContextReporter.cpp 'fixed')

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

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]]

https://hg.mozilla.org/mozilla-central/rev/dd6c4f6a2448