Bug 60377 – [Qt] fast/canvas/webgl/gl-uniform-arrays.html failing for Qt on Linux

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Idrees]

fast/canvas/webgl/gl-uniform-arrays.html failing for webkit2 on qt (linux). 

Also the debug version is giving assertion failing. The bug might be reproducible on other platforms as well.

[Idrees]

Created attachment 92593 [details]
Fix the debug version plus test case

I have attached the fix for test case. The comment was already there but not followed correctly.

We do not need to put assert in useProgram as program can be null (according to the test case).

[Eric Seidel (no email)]

Comment on attachment 92593 [details]
Fix the debug version plus test case

Need a ChangeLog and test.

[Idrees]

Created attachment 93319 [details]
Second version of the fix

Trying again after editing the changelog.

[Idrees]

Created attachment 93320 [details]
Test case

[WebKit Review Bot]

Attachment 93319 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Idrees]

Comment on attachment 93319 [details]
Second version of the fix

Removing because of format failing.

[Idrees]

Created attachment 93321 [details]
Trying Fix again

[Andreas Kling]

Comment on attachment 93321 [details]
Trying Fix again

View in context: https://bugs.webkit.org/attachment.cgi?id=93321&action=review

> Source/WebKit2/Shared/WebPreferencesStore.h:61
> -    macro(WebGLEnabled, webGLEnabled, Bool, bool, false) \
> +    macro(WebGLEnabled, webGLEnabled, Bool, bool, true) \

This enables WebGL for all WK2 ports, that kind of change should not be done behind a bug marked [Qt].
Not to mention that it has nothing to do with the failing test this bug is about.

[Idrees]

(In reply to comment #8)
> This enables WebGL for all WK2 ports, that kind of change should not be done behind a bug marked [Qt].
> Not to mention that it has nothing to do with the failing test this bug is about.

I agree. But i put this in the patch just for everybody to enable webgl without changing the code. Any i take out this part from the patch.

[Idrees]

Created attachment 93807 [details]
Removing the webgl enabler for wk2

[Benjamin Poulain]

Comment on attachment 93807 [details]
Removing the webgl enabler for wk2

You need to describe the change in the changelog, like why the assertion is not needed.

Also, without changelog, it looks like you are fixing 2 unrelated things:
-The assert which is Qt specific
-The uniform name, which is common code.

I would prefer 2 patches with the test for each change.

Finally, how is this related to WebKit 2? Your changes only WebCore common code?

[Jarkko Sakkinen]

Nice to see that more people are contributing to this work :)

Fix itself looks good but why you have added ASSERT()? How is it related fixing *this* particular bug?

[Idrees]

(In reply to comment #12)
> Fix itself looks good but why you have added ASSERT()? How is it related fixing *this* particular bug?
I have not added but removed the assert because in debug mode compilation, the test case is giving assertion failed. As the test case itself mentions that it is ok to have program NULL, we do not need it in any case.

[Idrees]

(In reply to comment #11)
> (From update of attachment 93807 [details])
> You need to describe the change in the changelog, like why the assertion is not needed.

Ok i will do so.

> 
> Also, without changelog, it looks like you are fixing 2 unrelated things:

Both are related to same test case. Thats why i created a single patch for it. Also both are one liners. so in my view creating a single patch makes sense.

> -The assert which is Qt specific

agreed.

> -The uniform name, which is common code.

agreed. I have not checked it with other platforms but i have mentioned the possibility that it might be reproducible on other platforms. I was just checking it on qt webkit2.

> 
> I would prefer 2 patches with the test for each change.
> 
> Finally, how is this related to WebKit 2? Your changes only WebCore common code?

I had only tested with Webkit2 for Qt. Now i tested with qt webkit and got the same behaviour. so will remove wk2 from the summary.

[Idrees]

Created attachment 93949 [details]
Another patch for fix

[Idrees]

Comment on attachment 93949 [details]
Another patch for fix

Putting for review.

[WebKit Review Bot]

Attachment 93949 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2446:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Idrees]

Created attachment 93952 [details]
Another patch for fix

[Luiz Agostini]

Comment on attachment 93952 [details]
Another patch for fix

View in context: https://bugs.webkit.org/attachment.cgi?id=93952&action=review

> Source/WebCore/ChangeLog:8
> +        [Qt] fast/canvas/webgl/gl-uniform-arrays.html failing for Qt on Linux
> +        https://bugs.webkit.org/show_bug.cgi?id=60377 
> +
> +        LayoutTests/fast/canvas/webgl/gl-uniform-arrays.html

It would be nice to have a description of what was causing the problem and how this patch solves it.

[Idrees]

Created attachment 94081 [details]
More description about the issue in changelog

[Idrees]

(In reply to comment #19)
> (From update of attachment 93952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93952&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        [Qt] fast/canvas/webgl/gl-uniform-arrays.html failing for Qt on Linux
> > +        https://bugs.webkit.org/show_bug.cgi?id=60377 
> > +
> > +        LayoutTests/fast/canvas/webgl/gl-uniform-arrays.html
> 
> It would be nice to have a description of what was causing the problem and how this patch solves it.


The problem was that we were truncating the name of the active uniform array without checking if it contains the array braces "[0]". As a result we were truncating the actual name of the active uniform. My patch checks for the array braces before taking them out.

Also we do not need assert in useProgram as program can be null. 

I have added these comments to the changelog as well.

[Alexis Menard (darktears)]

Comment on attachment 93952 [details]
Another patch for fix

Clearing that one because a new version has been uploaded.

[Alexis Menard (darktears)]

Comment on attachment 94081 [details]
More description about the issue in changelog

View in context: https://bugs.webkit.org/attachment.cgi?id=94081&action=review

> Source/WebCore/ChangeLog:10
> +        for an array of active uniform, we can receive just a name without 

Uppercase for the first letter, it's a sentence.

> Source/WebCore/ChangeLog:11
> +        array braces. Currently i donot see any [0] in the active uniform arrays name 

missing space.

> Source/WebCore/ChangeLog:12
> +        and as a result we were trucating the the actual name of the active uniforms. 

typo again.

[Idrees]

Created attachment 94622 [details]
Fixing typos.

[Andreas Kling]

Comment on attachment 94622 [details]
Fixing typos.

View in context: https://bugs.webkit.org/attachment.cgi?id=94622&action=review

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2446
> -        if (info.size > 1)
> +        if (info.size > 1 && info.name.endsWith("[0]"))

Is "[0]" the only thing we should be stripping. What about "[1]"?
This looks like a testable change, why is there no new test in this patch?

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:-1448
> -    ASSERT(program);

This should be in a separate patch.

[Idrees]

(In reply to comment #25)
> (From update of attachment 94622 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94622&action=review
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2446
> > -        if (info.size > 1)
> > +        if (info.size > 1 && info.name.endsWith("[0]"))
> 
> Is "[0]" the only thing we should be stripping. What about "[1]"?

Yes, because if its an array the first element will be either just a name or it will end with [0]. There is no way it will end with [1].

> This looks like a testable change, why is there no new test in this patch?

The test case that i have attached is actually failing without this change for qtwebkit. because we are truncating the last three characters without verifying if they are really [0]. "getActiveUniform" gives only "color" as the name for "info" even its an array. So we need to make sure we are not truncating the name of the uniform itself.

> 
> > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:-1448
> > -    ASSERT(program);
> 
> This should be in a separate patch.

Ok. I have created two different patches now. But the same test case can be applied for both. So i have added it to the changelog.

[Idrees]

Created attachment 94631 [details]
Assenrtion not needed patch.

[Idrees]

Created attachment 94633 [details]
array name should be checked before removing the [0].

[Idrees]

Created attachment 94636 [details]
Another try for name checking patch

[Idrees]

Comment on attachment 94636 [details]
Another try for name checking patch

Sorry again tab :(

[WebKit Review Bot]

Attachment 94636 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Idrees]

Created attachment 94639 [details]
Another try for name checking patch

[Idrees]

Created attachment 94640 [details]
getting crazy. Trying again

[WebKit Commit Bot]

Comment on attachment 94631 [details]
Assenrtion not needed patch.

Clearing flags on attachment: 94631

Committed r87208: <http://trac.webkit.org/changeset/87208>

[Zhenyao Mo]

Looks good.  Thanks for catching this.

[Andreas Kling]

Comment on attachment 94640 [details]
getting crazy. Trying again

r=me based on zmo's lgtm.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 94640 [details]:

inspector/debugger/debugger-breakpoints-not-activated-on-reload.html bug 55551 (authors: [e-mail] and [e-mail])
java/lc3/JavaObject/JavaObjectToInt-002-n.html bug 58299 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 94640 [details]
getting crazy. Trying again

Clearing flags on attachment: 94640

Committed r87451: <http://trac.webkit.org/changeset/87451>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r87208 cherry-picked into qtwebkit-2.2 with commit 122c9e6 <http://gitorious.org/webkit/qtwebkit/commit/122c9e6>
Revision r87451 cherry-picked into qtwebkit-2.2 with commit 9682ccf <http://gitorious.org/webkit/qtwebkit/commit/9682ccf>