[chromium] Avoid calling getUniformLocation??() in the compositor startup

• Component: New Bugs
• Status: RESOLVED FIXED

Use the newly introduced GL_CHROMIUM_bind_uniform_location extension instead.

I plan to fix this.

Created attachment 150194 [details]
Patch

Created attachment 150200 [details]
Patch

[Adrienne Walker]

Comment on attachment 150200 [details]
Patch

The approach looks great.  You need to pull and rebase this patch against jamesr's change of GraphicsContext3D -> WebGraphicsContext3D.

A rename would definitely cause a massive merge failure. Thank you, on it.

[James Robinson]

Comment on attachment 150200 [details]
Patch

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

If you update this patch to ToT it'll apply cleanly and trigger the auto-cc behavior, etc.  The EWS bubbles are purple since they couldn't apply this patch.

> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:162
> +    void bindUniformLocationCHROMIUM(Platform3DObject program, GC3Dint location, const GC3Dchar* uniform);

I think your WebKit checkout is out of data - you don't have to modify Extensions3DChromium at all for compositor changes as of http://trac.webkit.org/changeset/121204.  Just adding the entry point to WebGraphicsContext3D is sufficient

> Source/WebCore/platform/graphics/chromium/ProgramBinding.cpp:64
>  void ProgramBindingBase::init(GraphicsContext3D* context, const String& vertexShader, const String& fragmentShader)

ProgramBinding is based on WebGraphicsContext3D, not GraphicsContext3D (cuts down on one level of indirection)

> Source/WebCore/platform/graphics/chromium/ProgramBinding.cpp:156
> +bool ProgramBindingBase::hasBindUniformsExtension(GraphicsContext3D* context) const

In the compositor we query all the extensions we may need up front (inside LayerRendererChromium::initialize) and then propagate them through as needed - this avoids any unnecessary get() calls and makes sure that all of our extensions use is centralized in one place.  All of the ProgramBinding uses and initialization calls are inside of LayerRendererChromium, so it should be pretty easy to plumb a bit for this through.

The extension name is bind_uniform (singular), so any functions/variables should stick to that and not plural "uniforms"

> Source/WebCore/platform/graphics/chromium/ShaderChromium.cpp:228
> +        "point[0]",

the example text in CHROMIUM_bind_uniform_location.txt queries the location of an array with just "point", not "point[0]". are both syntaxes allowed in getProgramUniform..() ?

My webkit is out of date, I was treading carefully to maintain a working build. I'll rebase.

>the example text in CHROMIUM_bind_uniform_location.txt queries the location of an array with just >"point", not "point[0]". are both syntaxes allowed in getProgramUniform..() ?

Both point and point[0] are allowed. Sub-indices are there in case you want to update only one of the array elements since "pointer arithmetic" doesn't work with uniform locations. I used "point[0]" for debugging but I think "point" is probably cleaner, I'll update.

Thanks for the rest of the comments, will address.

Created attachment 150285 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/ChangeLog:11:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 11 files


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

[James Robinson]

Comment on attachment 150285 [details]
Patch

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

> Source/Platform/ChangeLog:7
> +        This change replaces getUniformLocation calls in the chrome compositor with bindUniformLocation
> +        to speed up startup time.

The changelog entries should be specific to changes the particular ChangeLog covers - so this one is just describing the changes to Platform/chromium/public/WebGraphicsContext3D.h. You should just say here that you are adding an entry point for bindUniformLocationCHROMIUM

> Source/Platform/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

in all ChangeLogs this line should go above your description of the patch.

> Source/WebCore/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

move this up

>> Source/WebCore/ChangeLog:11
>> +        No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]

You will need to address this in some way - either by adding tests where appropriate or documenting the test coverage that we have.  In this case, the actual implementation guts of bindUniformLocation are in a different codebase and (hopefully) well-tested there, so we don't really need to duplicate coverage for that.  Since the only logic in the compositor is pretty basic glue, and we have integration pixel tests to make sure we aren't completely flubbing the programs so we are probably OK with just a description of which tests cover this behavior (i.e. pretty much all of them that use any shaders)

> Source/Platform/chromium/public/WebGraphicsContext3D.h:416
> +    virtual void bindUniformLocationCHROMIUM(WebGLId program, WGC3Dint location, const WGC3Dchar* uniform) { }

This isn't part of GL_EXT_occlusion_query, so it doesn't belong in that section.  Give this extension a new section (i.e. separate with newlines) with a comment naming the extension in case we ever get more entry points with this extension. We probably won't, but it's better to be consistent with the other extensions in this file.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:230
> +    m_capabilities.usingBindUniform = extensions.contains("GL_CHROMIUM_bind_uniform_location");

For this, I don't think we need to put a bit on m_capabilities since nobody other than LayerRendererChromium needs to know if this extension exists on the compositor's context or not. Instead, you can just store a member variable on the LRC.  (This appears be true for some other things currently on m_capabilities as well, things tend to just get cargo-culted, but for some things like maxTextureSize other systems do have a legit need to know the capability)

> Source/WebCore/platform/graphics/chromium/ProgramBinding.cpp:56
>      // If you hit these asserts, you initialized but forgot to call cleanup().
>      ASSERT(!m_program);
> +    ASSERT(!m_vertexShader);
> +    ASSERT(!m_fragmentShader);

Did you patch cleanup to zero these guys out?  If not, please update either the comment or the code to match the other.

> Source/WebCore/platform/graphics/chromium/ProgramBinding.cpp:106
> +    m_vertexShader = 0;
> +    m_fragmentShader = 0;

Hm, this is a bit confusing - do these two member variables just exist to store some transient state?  If so, why are they member variables and not just maintained on the stack?

> Source/WebCore/platform/graphics/chromium/ProgramBinding.h:86
> +            ProgramBindingBase::link(context);

do you need the ProgramBindingBase:: qualifier here, or can you just call link() ?

> Source/WebCore/platform/graphics/chromium/TextureCopier.h:65
> +    explicit AcceleratedTextureCopier(WebKit::WebGraphicsContext3D*, bool usingBindUniforms);

no explicit on 2-argument constructors. the "explicit" keyword is to prevent implicit conversion of one type to another in the case where we provide a one-argument constructor.  I.e. the fact that we provide the "explicit AcceleratedTextureCopier(WebKit::WebGraphicsContext3D*);" constructor does not mean that we should be able to pass a WebKit::WebGraphicsContext3D* type to a function expecting an AcceleratedTextureCopier and have a copier created for us behind the scenes

> +    m_vertexShader = 0;
> +    m_fragmentShader = 0;

>Hm, this is a bit confusing - do these two member variables just exist to store some transient state?  If so, why are they member variables and not just maintained on the stack?

Well, the previous implementation assumed that nothing outside of the base class needs to happen between compile and link. That changed with the introduction of bindUniforms and could further change if  another type of shader is introduced. For example, one that needs to call bindAttribLocation on some new vertex data. Holding on to shader id's allows for such behavior to happen without re-implementing a bunch of compile logic in the subclass.

That said, I refactored the cleanup code to be more robust/explicit.

New patch on the way.

Created attachment 150488 [details]
Patch

[WebKit Review Bot]

Comment on attachment 150488 [details]
Patch

Attachment 150488 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13132246

Created attachment 150528 [details]
Patch

[Adrienne Walker]

Comment on attachment 150528 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ShaderChromium.cpp:47
> +        if (uniformIndex >= maxLocations)
> +            return;

Can you just ASSERT here instead?  This should never happen.  Also, if this happens and you return after only getting some of the uniforms, the shader isn't going to work at all.

Created attachment 150650 [details]
Patch

[Adrienne Walker]

Comment on attachment 150650 [details]
Patch

R=me.  Thanks.

[WebKit Review Bot]

Comment on attachment 150650 [details]
Patch

Rejecting attachment 150650 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
iled to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2

Updating OpenSource
fatal: Unable to look up git.webkit.org (port 9418) (Temporary failure in name resolution)
Died at Tools/Scripts/update-webkit line 162.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
fatal: Unable to look up git.webkit.org (port 9418) (Temporary failure in name resolution)
Died at Tools/Scripts/update-webkit line 162.

Full output: http://queues.webkit.org/results/13137479

[Adrienne Walker]

Comment on attachment 150650 [details]
Patch

Let's give the CQ another try.

[WebKit Review Bot]

Comment on attachment 150650 [details]
Patch

Clearing flags on attachment: 150650

Committed r121823: <http://trac.webkit.org/changeset/121823>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.