Closed Bug 682496 Opened 8 years ago Closed 8 years ago

program-test.html test failures

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: drs, Assigned: drs)

References

Details

Attachments

(1 file, 12 obsolete files)

9.98 KB, patch
drs
: review+
Details | Diff | Splinter Review
Running program-test.html gives the following error:
FAIL an attached shader shouldn't be deleted
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/programs/program-test.html

There's no checks within deleteShader() to see if the shader is attached to anything. Curiously enough, there's a function AttachCount() which seems to work (at least logically; I haven't debugged it) but even when gating on that being 0 before deleting, the error still occurs, and another new one appears too.
Will comment on changes in next comment.
Assignee: nobody → dsherk
Attachment #557058 - Flags: review?(bjacob)
Attachment #557058 - Attachment is obsolete: true
Attachment #557058 - Flags: review?(bjacob)
Attachment #557060 - Flags: review?(bjacob)
Failed twice at uploading my changes.
Attachment #557060 - Attachment is obsolete: true
Attachment #557060 - Flags: review?(bjacob)
Attachment #557062 - Flags: review?(bjacob)
What basically ended up happening was that programs and shaders weren't deleting themselves correctly, nor were programs deleting shaders at all. The assumptions I'm making in this patch are the following:

1) When programs and shaders have delete called on them, but are in use (attached for shaders, current program for programs), they should be marked as needing to be deleted. They will then be deleted when they are no longer in use.
2) If a shader is marked as pending deletion, compilation should be silently ignored, which is based on my interpretation of a comment from the WebKit people here: 
http://trac.webkit.org/browser/trunk/WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp?rev=68434#L678
3) If a shader becomes marked for deletion but is still attached to a program, but then this program becomes no longer current, this shader should be deleted too.

>@@ -378,16 +378,18 @@ public:
>                                              : mBoundCubeMapTextures[mActiveTexture];
>     }
> 
>     already_AddRefed<CanvasLayer> GetCanvasLayer(nsDisplayListBuilder* aBuilder,
>                                                  CanvasLayer *aOldLayer,
>                                                  LayerManager *aManager);
>     void MarkContextClean() { mInvalidated = PR_FALSE; }
> 
>+    void DetachShaders(WebGLProgram *program);

This was a result of DetachShaders() needing access to some data stored on the context.

>@@ -1157,16 +1159,22 @@ WebGLContext::DeleteShader(nsIWebGLShade
>     if (!GetConcreteObjectAndGLName("deleteShader", sobj, &shader, &shadername, &isNull, &isDeleted))
>         return NS_OK;
> 
>     if (isNull || isDeleted)
>         return NS_OK;
> 
>     MakeContextCurrent();
> 
>+    if (shader->AttachCount() > 0)
>+    {
>+        shader->SetDeletePending();
>+        return NS_OK;
>+    }

This seemed to be the fix to the original problem, but just adding this actually caused 2 more errors.
Failed again! Forgot to qref before I copied it.
Attachment #557062 - Attachment is obsolete: true
Attachment #557062 - Flags: review?(bjacob)
Attachment #557064 - Flags: review?(bjacob)
(In reply to Doug Sherk from comment #4)
> What basically ended up happening was that programs and shaders weren't
> deleting themselves correctly, nor were programs deleting shaders at all.
> The assumptions I'm making in this patch are the following:
> 
> 1) When programs and shaders have delete called on them, but are in use
> (attached for shaders, current program for programs), they should be marked
> as needing to be deleted. They will then be deleted when they are no longer
> in use.

True

> 2) If a shader is marked as pending deletion, compilation should be silently
> ignored, which is based on my interpretation of a comment from the WebKit
> people here: 
> http://trac.webkit.org/browser/trunk/WebKit/chromium/src/
> WebGraphicsContext3DDefaultImpl.cpp?rev=68434#L678

Notice that is a very old WebKit revision.

I don't understand why compilation of a shader pending deletion should be silently ignored. Either a shader is deleted in which case attempting to compile it should generate GL_INVALID_OPERATION or it's not deleted in which case attempting to compile it should actually compile it. My understanding is that a shader pending deletion is not deleted hence it should still be compiled regardless of the pending-deletion status.

> 3) If a shader becomes marked for deletion but is still attached to a
> program, but then this program becomes no longer current, this shader should
> be deleted too.

Yes, unless of course it's attached to yet another program.

> 
> >@@ -378,16 +378,18 @@ public:
> >                                              : mBoundCubeMapTextures[mActiveTexture];
> >     }
> > 
> >     already_AddRefed<CanvasLayer> GetCanvasLayer(nsDisplayListBuilder* aBuilder,
> >                                                  CanvasLayer *aOldLayer,
> >                                                  LayerManager *aManager);
> >     void MarkContextClean() { mInvalidated = PR_FALSE; }
> > 
> >+    void DetachShaders(WebGLProgram *program);
> 
> This was a result of DetachShaders() needing access to some data stored on
> the context.

Still, why redeclare it here? All the other WebGL functions also access data store on the context and don't need to be redeclared here.
Partially fixes comments by bjacob. Only addresses object design for DetachShaders(), does not address compilation of delete-marked shaders. This will be done pending discussion. Patch comment needs update.
Attachment #557064 - Attachment is obsolete: true
Attachment #557064 - Flags: review?(bjacob)
Attachment #557393 - Flags: review?(bjacob)
Comment on attachment 557393 [details] [diff] [review]
Patch v1.1, partial fix for comments.

Also note that I have to delete the test from the failing_tests_* list again.
bjacob was completely right about my second assumption being wrong. What turned out happening was that, when compileShader() was called, the source code (or rather an untranslated version of it) was reloaded and then the shader was recompiled even if nothing had changed. This code path has been removed.
Attachment #557393 - Attachment is obsolete: true
Attachment #557393 - Flags: review?(bjacob)
Attachment #557733 - Flags: review?(bjacob)
Comment on attachment 557733 [details] [diff] [review]
Patch v1.2, program-test.html error fix and changes to deletion marking behavior.

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

This looks all right! Just 1 trivial change needed below. Please carry forward r+.

Only thing I'm worried about is the complexity to ensure that there's no leak here i.e. that shaders that should be destroyed actually are destroyed. But I don't see any leak introduced by your patch.

::: content/canvas/src/WebGLContextUtils.cpp
@@ +212,5 @@
>      }
>  };
> +
> +void 
> +WebGLProgram::DetachShaders() {

Please keep all WebGLProgram methods inline in WebGLContext.h for now.

I know that's ugly but the real solution will be to have 1 file per class, not to put this in WebGLContextUtils.cpp
Attachment #557733 - Flags: review?(bjacob) → review+
+r carried (r=bjacob)

This needs more work to actually fix the errors in program-test.html. However, the code was previously more flawed than the conformance tests showed.
Attachment #557733 - Attachment is obsolete: true
Unbitrotted the previous patch, fixed other errors that have since cropped up in program-test.html, and removed changes to shader source compilation (someone else committed changes to it that fixed what this patch was supposed to fix). program-test.html now passes all OSX mochitests (haven't tried it elsewhere).
Attachment #558693 - Attachment is obsolete: true
Attachment #566092 - Flags: review?(bjacob)
Made an accidental addition in the last patch to fix something on my local machine, also noticed another regression that I missed and should now be fixed.
Attachment #566092 - Attachment is obsolete: true
Attachment #566092 - Flags: review?(bjacob)
Attachment #566106 - Flags: review?(bjacob)
Comment on attachment 566106 [details] [diff] [review]
Patch v1.5, program-test.html error fix and changes to deletion marking behavior.

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

Almost r+ but I still have a problem with one hunk:

::: content/canvas/src/WebGLContextGL.cpp
@@ +4143,5 @@
> +            GLint i = 0;
> +            gl->fGetShaderiv(shadername, pname, &i);
> +            wrval->SetAsBool(bool(i));
> +        }
> +            break;

What is the use of this hunk? It seems to be doing exactly the same as the existing code in next case statement.

Or did you want to let it check the WebGL deleted status?
Attachment #566106 - Flags: review?(bjacob) → review-
So a combination of deciding that I didn't like my fix and also finding a regression that was difficult to fix convinced me to rewrite this basically from scratch. I addressed the code review comment, though :p

Also note that, although the previous code worked (except for one conformance error that I found on Linux), it was completely wrong in several areas.
Attachment #566106 - Attachment is obsolete: true
Attachment #567965 - Flags: review?(bjacob)
Comment on attachment 567965 [details] [diff] [review]
Patch v2.0, program-test.html error fix and changes to deletion marking behavior.

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

::: content/canvas/src/WebGLContext.h
@@ +1419,5 @@
>      WebGLenum ShaderType() { return mType; }
>  
>      PRUint32 AttachCount() { return mAttachCount; }
>      void IncrementAttachCount() { mAttachCount++; }
>      void DecrementAttachCount() { mAttachCount--; }

mAttachCount should really be signed.

@@ +1496,4 @@
>      }
>  
>      void DetachShaders() {
> +        WebGLShader* shader;

make it local to the loop

::: content/canvas/src/WebGLContextGL.cpp
@@ +1184,1 @@
>      shader->Delete();

let's keep the Removes below the Deletes
Addressed code review comments.
Attachment #567965 - Attachment is obsolete: true
Attachment #567965 - Flags: review?(bjacob)
Attachment #569392 - Flags: review?(bjacob)
Attachment #569392 - Flags: review?(bjacob) → review+
backed out for leaks in mochitest-1

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 34760 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 44 instances of WebGLProgram with size 240 bytes each (10560 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 88 instances of WebGLShader with size 176 bytes each (15488 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 75 instances of WebGLUniformLocation with size 80 bytes each (6000 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 88 instances of nsStringBuffer with size 8 bytes each (704 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 251 instances of nsTArray_base with size 8 bytes each (2008 bytes total)
Fixed memory leaks.
Attachment #569392 - Attachment is obsolete: true
Attachment #569994 - Flags: review?(bjacob)
Comment on attachment 569994 [details] [diff] [review]
Patch v2.2, program-test.html error fix and changes to deletion marking behavior.

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

I'm not a fan of the name SafeDelete. I would prefer an explicit descriptive name, even if it's going to be long and clunky.

But since I can't think of a good name at the moment, r=me.
Attachment #569994 - Flags: review?(bjacob) → review+
Blocks: 697211
Changed name of SafeDelete() functions. +r carried.
Attachment #569994 - Attachment is obsolete: true
Attachment #570912 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/7da669483434
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.