Bug 50716 – Update validation of stencil mask and ref values

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

[Kenneth Russell]

Based on feedback from Daniel Koch from TransGaming, the section in the WebGL spec on restrictions on the stencil mask and ref values has been changed to do the validation at draw time rather than when stencilMaskSeparate and stencilFuncSeparate are called, since generating an INVALID_OPERATION error from those entry points may make it hard for applications to transition between states. The WebKit implementation needs to be updated to do the validation then as well. Ideally stencilMaskSeparate and stencilFuncSeparate would set a boolean flag which can be easily checked in drawArrays and drawElements.

[Zhenyao Mo]

Created attachment 77631 [details]
Patch

[Kenneth Russell]

Comment on attachment 77631 [details]
Patch

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

Looks good; one cleanup upon landing please.

> WebCore/html/canvas/WebGLRenderingContext.cpp:1119
> +    if (m_stencilMask != m_stencilMaskBack || m_stencilFuncRef != m_stencilFuncRefBack || m_stencilFuncMask != m_stencilFuncMaskBack) {
> +        m_context->synthesizeGLError(GraphicsContext3D::INVALID_OPERATION);
> +        return;
> +    }
> +

Could you please refactor these tests into a helper function similar to validateDrawMode?

[Zhenyao Mo]

(In reply to comment #2)
> (From update of attachment 77631 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77631&action=review
> 
> Looks good; one cleanup upon landing please.
> 
> > WebCore/html/canvas/WebGLRenderingContext.cpp:1119
> > +    if (m_stencilMask != m_stencilMaskBack || m_stencilFuncRef != m_stencilFuncRefBack || m_stencilFuncMask != m_stencilFuncMaskBack) {
> > +        m_context->synthesizeGLError(GraphicsContext3D::INVALID_OPERATION);
> > +        return;
> > +    }
> > +
> 
> Could you please refactor these tests into a helper function similar to validateDrawMode?

Will do.

[Zhenyao Mo]

Committed r74818: <http://trac.webkit.org/changeset/74818>

[Zhenyao Mo]

Again, webkit-patch land does not close the bug automatically and I had to manually add the committed message and close the bug.

This is the second time this happened.  Both time I called webkit-patch land remotely through PUTTY.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74818 might have broken SnowLeopard Intel Release (Tests)

[Zhenyao Mo]

Committed r74820: <http://trac.webkit.org/changeset/74820>

[Zhenyao Mo]

(In reply to comment #7)
> Committed r74820: <http://trac.webkit.org/changeset/74820>

I had to commit test changes a second time.  I really don't understand what's happening here.  Seems like r74818 only picked up changes under WebCore but not under LayoutTests; however, it picked up both change logs.  Really strange.

Again, I did the two commits through PUTTY.

[Kenneth Russell]

(In reply to comment #8)
> (In reply to comment #7)
> > Committed r74820: <http://trac.webkit.org/changeset/74820>
> 
> I had to commit test changes a second time.  I really don't understand what's happening here.  Seems like r74818 only picked up changes under WebCore but not under LayoutTests; however, it picked up both change logs.  Really strange.
> 
> Again, I did the two commits through PUTTY.

I'm not sure this could be the issue, but I always run webkit-patch land while cd'd into the top-level WebKit directory, just like svn-create-patch.