The default bug view has changed. See this FAQ.

Use stdint instead of PRInt types in WebGL implementation

RESOLVED FIXED in mozilla15

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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
Attachment #626085 - Flags: review?(Ms2ger)
(Assignee)

Updated

5 years ago
Assignee: nobody → bjacob
OS: Linux → All
Hardware: x86_64 → All
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
Attachment #626085 - Flags: review?(Ms2ger)
Attachment #626085 - Flags: review+
Attachment #626085 - Flags: feedback?(landry)
(Assignee)

Comment 2

5 years ago
Created attachment 626165 [details] [diff] [review]
updated patch for landing

Applied review comments.

On try:
https://tbpl.mozilla.org/?tree=Try&rev=e1bc59211166
Attachment #626165 - Flags: review+
Attachment #626165 - Flags: feedback?(landry)
(Assignee)

Updated

5 years ago
Attachment #626085 - Flags: feedback?(landry)
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 :)
Attachment #626165 - Flags: feedback?(landry) → feedback+
Oops, my feedback was about att #626085, i'll retry with #626165 in a few..
Seems to build fine too with att #626165 (with WebGLContextReporter.cpp 'fixed')
(Assignee)

Comment 6

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd6c4f6a2448

With the WebGLContextReporter.cpp fix --- thanks Landry, that was a real bug.
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/dd6c4f6a2448
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 766796
You need to log in before you can comment on or make changes to this bug.