Bug 117550 – [CSS Shaders] Animations and transitions should use validated custom programs

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Ralph T]

[CSS Shaders] Animations and transitions should use validated custom programs

[Ralph T]

Created attachment 204448 [details]
Patch

[Ralph T]

CoordinatedGraphics is unable to proxy non-validated custom shaders to the UIProcess. I don't know enough about the architecture of custom filters to know that the filters in keyframes should be validated though.

Also, none of the CoordinatedGraphics ports enable CSS Shaders. With this patch and some others (which I will submit next) I have made everything work on EFL, but I don't know if this change regresses Mac (I'm a long-time reader, first time writer, so please forgive me if I've gone about this all wrong).

[Alexandru Chiculita]

Comment on attachment 204448 [details]
Patch

r=me

[Alexandru Chiculita]

Comment on attachment 204448 [details]
Patch

actually.. you need a test for it.

[Alexandru Chiculita]

Comment on attachment 204448 [details]
Patch

Ok, looks like no platform can run this yet. Add a comment in the changelog saying that you had no new tests as no platform can do custom-filters accelerated animations yet.

[Ralph T]

Created attachment 204456 [details]
Updated test section of changelog

Add note to changelog that there are no new tests because no platform currently runs accelerated custom filter animations.

[WebKit Commit Bot]

Comment on attachment 204456 [details]
Updated test section of changelog

Rejecting attachment 204456 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 204456, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/726829

[Alexandru Chiculita]

Comment on attachment 204456 [details]
Updated test section of changelog

Trying the cq again.

[WebKit Commit Bot]

Comment on attachment 204456 [details]
Updated test section of changelog

Clearing flags on attachment: 204456

Committed r151513: <http://trac.webkit.org/changeset/151513>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 117763

[Ralph T]

This change was rolled out because it caused an assert in StyleCustomFilterProgram.h StyleCustomFilterProgram::isLoaded called from RenderLayer::computeFilterOperations.


    virtual bool isLoaded() const
    {
        // Do not use the CachedResource:isLoaded method here, because it actually means !isLoading(),
        // so missing and canceled resources will have isLoaded set to true, even if they are not loaded yet.
        ASSERT(!m_vertexShader || m_vertexShader->isCachedShader());
        ASSERT(!m_fragmentShader || m_fragmentShader->isCachedShader());
        ASSERT(m_cachedVertexShader.get() || m_cachedFragmentShader.get());
        return (!m_cachedVertexShader.get() || m_isVertexShaderLoaded)
            && (!m_cachedFragmentShader.get() || m_isFragmentShaderLoaded);
    }

The third assert failed on keyframe animations where the custom filter existed only in a keyframe and had never been loaded before -- m_cachedVertexShader and m_cachedFragmentShader were both null.

If I do something to cause StyleCustomFilterProgram::willHaveClients to be called then the layout tests work without assertions.

I'm not sure what the right approach is here. Some questions:

1. Are there any other keyframe animatable resources which are loaded?

2. Does the RenderLayer want to become the loader client for all custom shaders in keyframes for the duration of the keyframe animation?

3. Does anything break like this if the server is slow to serve up fragment and vertex shaders? (i.e.: in the unanimated case, RenderLayer::computeFilterOperations is called with custom shaders in a state that the assertions above fail). I can test this one later today.

[Simon Fraser (smfr)]

 >2. Does the RenderLayer want to become the loader client for all custom shaders in keyframes for the duration of the keyframe animation?
No. It's wrong for RenderLayer to have to know anything about shaders.

[Ralph T]

Ok, would it be OK for RenderLayerFilterInfo to become the CustomFilterClient for all of the custom filters in an animation?

This would need plumbing in RenderLayerBacking::startAnimation, animationFinished, startTransition and transitionFinished. Obviously RenderLayerBacking doesn't see the keyframes after the animation has started, so RenderLayerFilterInfo would have to keep a map from animation name to a list of CustomFilterPrograms (instead of the single list that it keeps right now).

The GraphicsLayer implementations already keep a list of animations and add and remove from it. Maybe the GraphicsLayer implementation has to load and validate custom filters in keyframes? I could add this to GraphicsLayerAnimations so that texmap/CoordinatedGraphics does it -- it will basically duplicate RenderLayer::computeFilterOperations and an empty implementation of CustomFilterProgramClient.

Also the GraphicsLayer is the only thing that knows if it can run custom filter animations itself and the only thing that can invalidate if an animated shader loads partway though an animation. So maybe making GraphicsLayer responsible for loading and validating is OK?

[Simon Fraser (smfr)]

(In reply to comment #14)
> Ok, would it be OK for RenderLayerFilterInfo to become the CustomFilterClient for all of the custom filters in an animation?
> 
> This would need plumbing in RenderLayerBacking::startAnimation, animationFinished, startTransition and transitionFinished. Obviously RenderLayerBacking doesn't see the keyframes after the animation has started, so RenderLayerFilterInfo would have to keep a map from animation name to a list of CustomFilterPrograms (instead of the single list that it keeps right now).
> 
> The GraphicsLayer implementations already keep a list of animations and add and remove from it. Maybe the GraphicsLayer implementation has to load and validate custom filters in keyframes? I could add this to GraphicsLayerAnimations so that texmap/CoordinatedGraphics does it -- it will basically duplicate RenderLayer::computeFilterOperations and an empty implementation of CustomFilterProgramClient.
> 
> Also the GraphicsLayer is the only thing that knows if it can run custom filter animations itself and the only thing that can invalidate if an animated shader loads partway though an animation. So maybe making GraphicsLayer responsible for loading and validating is OK?

GraphicsLayer should not do any loading. If you are going to kick off resource loads, you should do that in a similar way to how we load images for style, or something.

[Alexandru Chiculita]

(In reply to comment #15)
> (In reply to comment #14)
> GraphicsLayer should not do any loading. If you are going to kick off resource loads, you should do that in a similar way to how we load images for style, or something.

I think we should teach RenderLayer::updateOrRemoveFilterClients about the animations. It should iterate on the "-webkit-filter" property keyframes and add the clients as needed. I suppose you would need to slightly change RenderLayerFilterInfo::updateCustomFilterClients to do that. This way the shaders are going to be loaded ahead of time, even if the animation is not running in the compositor.

AnimationController should be able to tell what are the animations running on a RenderObject and iterate on the keyframes of a specific property.

[Ralph T]

(In reply to comment #15)
> GraphicsLayer should not do any loading. If you are going to kick off resource loads, you should do that in a similar way to how we load images for style, or something.

I misspoke. The loads are scheduled by StyleResolver when the animated style is computed.

I have a patch which does the following in RenderLayerBacking:

 1. Checks a filter list for custom filters. These will be of type StyleCustomFilterProgram. If any shaders haven't been downloaded then we don't offer them to GraphicsLayer to animate.

  XXX: The isLoaded method on StyleCustomFilterProgram isn't exactly what I want; I want to know if all of the shaders are available and not ASSERT that they are. Currently I add an empty CustomFilterProgramClient prior to calling isLoaded but this isn't really correct either.

 2. If a filter is loaded, then we can add a CustomFilterProgramClient for it, which grabs the cached shaders and lets us retrieve the shader strings.

 3. We can validate loaded shaders. This creates a list of filters with ValidatedCustomFilterPrograms, not StyleCustomFilterPrograms. We don't care about keeping the cached shaders or StyleCustomFilterPrograms anymore.

 4. Remove our empty client from the StyleCustomFilterPrograms, since they're unneeded.

My reasoning for doing the above is:
 1. If a shader isn't loaded yet, we can't animate it anyway, so the compositor shouldn't be offered it.

 2. It's OK to use an empty CustomFilterProgramClient in this case, because we only need the shader strings to mangle through ANGLE. We check that the shader is loaded, so the notifyCustomFilterProgram loaded is useless to us.

[Ralph T]

Created attachment 205273 [details]
Patch

[Ralph T]

Comment on attachment 205273 [details]
Patch

Simon, Alexandru, is this on the right track or am I off in the weeds?

[Alexandru Chiculita]

(In reply to comment #19)
> (From update of attachment 205273 [details])
> Simon, Alexandru, is this on the right track or am I off in the weeds?

I think you can update computeFilterOperations to avoid adding the clients, but I think that we still need a real client. For example, what happens if the shaders are loaded when the animation is already supposed to be running? My expectation would be that the animation will load the shaders and still be able to run. At least that's what happens in the software pipeline.

Also, this problem might be a good question for the w3c CSS group: Should the navigator postpone the start of the filter animations until the shaders are loaded? I think shaders are the first to have this problem.

[Ralph T]

(In reply to comment #20)
> I think you can update computeFilterOperations to avoid adding the clients, but I think that we still need a real client. For example, what happens if the shaders are loaded when the animation is already supposed to be running? My expectation would be that the animation will load the shaders and still be able to run. At least that's what happens in the software pipeline.

With this patch I'm saying that if a shader isn't loaded at the start of the animation then the compositor shouldn't run the animation -- WebCore will run it instead (as it runs all custom filter animations currently). The compositor running animations is just a fast path to avoid recalculating style for every frame.

So if a shader does load while the animation is running then this patch doesn't change behavior -- and I think it's OK to say that the compositor won't handle any "hard" cases.

> Also, this problem might be a good question for the w3c CSS group: Should the navigator postpone the start of the filter animations until the shaders are loaded? I think shaders are the first to have this problem.

Personally I wouldn't want that in the places I'm using CSS Shaders -- for transitional effects where I wouldn't want the page to block on a load.

I think of it like animating background-position when the background-image hasn't loaded.

[Alexandru Chiculita]

(In reply to comment #21)
> (In reply to comment #20)
> > I think you can update computeFilterOperations to avoid adding the clients, but I think that we still need a real client. For example, what happens if the shaders are loaded when the animation is already supposed to be running? My expectation would be that the animation will load the shaders and still be able to run. At least that's what happens in the software pipeline.
> 
> With this patch I'm saying that if a shader isn't loaded at the start of the animation then the compositor shouldn't run the animation -- WebCore will run it instead (as it runs all custom filter animations currently). The compositor running animations is just a fast path to avoid recalculating style for every frame.
> 
> So if a shader does load while the animation is running then this patch doesn't change behavior -- and I think it's OK to say that the compositor won't handle any "hard" cases.
Ok, now I get it. Makes sense.

> 
> > Also, this problem might be a good question for the w3c CSS group: Should the navigator postpone the start of the filter animations until the shaders are loaded? I think shaders are the first to have this problem.
> 
> Personally I wouldn't want that in the places I'm using CSS Shaders -- for transitional effects where I wouldn't want the page to block on a load.
> 
> I think of it like animating background-position when the background-image hasn't loaded.
Ok.

[Ralph T]

So should I clean this up and submit something for review?

I don't like the empty client, but I'm not sure how to do away with it.

I need to coerce the StyleCustomFilterProgram into being loaded if the shader strings are available and the client mechanism causes all of this to happen (it extracts the "CachedShaders", becomes a client of them which updates the m_isXYZShaderLoaded values, which makes isLoaded return a reasonable value).

Is the empty client something that would cause an r-? I think this case is unique since there are no animations of something that's loaded currently...

[Simon Fraser (smfr)]

Comment on attachment 205273 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:2120
> +        // Calling CustomFilterProgram::isLoaded with no clients will make it assert; add

If you need EmptyCustomFilterProgramClient just for this, why not fix the assert?

> Source/WebCore/rendering/RenderLayerBacking.cpp:2268
> +            FilterOperations validatedFromFilters;
> +            FilterOperations validatedToFilters;
> +            if (validateCustomFilters(owningLayer(), fromStyle, validatedFromFilters))
> +                filterVector.insert(FilterAnimationValue::create(0, validatedFromFilters));
> +            if (validateCustomFilters(owningLayer(), toStyle, validatedToFilters))
> +                filterVector.insert(FilterAnimationValue::create(1, validatedToFilters));

I don't think we should pay the cost of two FilterOperations on the stack when there are no custom filters.

[Csaba Osztrogonác]

Comment on attachment 204448 [details]
Patch

Cleared Alexandru Chiculita's review+ from obsolete attachment 204448 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Ralph T]

CSS Shaders are gone. Don't need this bug anymore.