Bug 93623 – [CSS Shaders] Add blend mode and composite op to compiled program cache key

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Max Vujovic]

Right now, only the vertex shader string and the fragment shader string are hashed to create a key for the compiled program. However, when authors use the mix function to specify a blend mode and/or composite op for a fragment shader, WebKit will rewrite the fragment shader based on the blend mode and composite op. This means the unique key for a compiled program is (original vertex shader string, original fragment shader string, blend mode, composite operator). This patch will include blend mode and composite operator in the hash for the key.

[Max Vujovic]

Created attachment 157477 [details]
Patch

Alex, could you take a look at this patch? Especially the hash computation in CustomFilterProgramInfo::hash(). I was debating between a bunch of ways we could do the hash- I want your opinion on it.

[Alexandru Chiculita]

Comment on attachment 157477 [details]
Patch

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

Looks good. Just a few comments.

> Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:43
> +    , compositeOperator(CompositeSourceOver)

nit: Identation here.

> Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:49
> +        return enabled == o.enabled

I think enabled == o.enabled && enabled == false should ignore the other two properties. Right?

> Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:57
> +} CustomFilterProgramMixSettings;

Why do you need this instance?

> Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:72
> +CustomFilterProgramInfo::CustomFilterProgramInfo(const String& vertexShader, const String& fragmentShader, CustomFilterProgramMixSettings mixSettings)

I would pass a const ref here.

> Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:86
> +    unsigned hash = hashPossiblyNullString(m_vertexShaderString);

This would look better if you encapsulated it into a separate class:

HashBuilder hash;
hash.append(m_vertexShaderString);
hash.append(m_fragmentShaderString);
...

hash.toUnsigned();

> Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.h:46
> +    CustomFilterProgramInfo(const String&, const String&, CustomFilterProgramMixSettings);

const& in here.

[Max Vujovic]

Created attachment 157583 [details]
Patch

Updated patch based on Alex's comments.

[Max Vujovic]

Comment on attachment 157477 [details]
Patch

Thanks for the review, Alex. Responses inline:

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

>> Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:43
>> +    , compositeOperator(CompositeSourceOver)
> 
> nit: Identation here.

Thanks. Done.

>> Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:49
>> +        return enabled == o.enabled
> 
> I think enabled == o.enabled && enabled == false should ignore the other two properties. Right?

Yeah, that would be more robust. Done.

>> Source/WebCore/platform/graphics/CustomFilterProgramMixSettings.h:57
>> +} CustomFilterProgramMixSettings;
> 
> Why do you need this instance?

It's a typedef, but I removed it now. (It's old C habit of mine to typedef structs.)

>> Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:72
>> +CustomFilterProgramInfo::CustomFilterProgramInfo(const String& vertexShader, const String& fragmentShader, CustomFilterProgramMixSettings mixSettings)
> 
> I would pass a const ref here.

Done.

>> Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:86
>> +    unsigned hash = hashPossiblyNullString(m_vertexShaderString);
> 
> This would look better if you encapsulated it into a separate class:
> 
> HashBuilder hash;
> hash.append(m_vertexShaderString);
> hash.append(m_fragmentShaderString);
> ...
> 
> hash.toUnsigned();

I found something similar to a HashBuilder in StringHasher::hashMemory. I use that instead now, so it should look a lot better.

>> Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.h:46
>> +    CustomFilterProgramInfo(const String&, const String&, CustomFilterProgramMixSettings);
> 
> const& in here.

Done.

[Max Vujovic]

Comment on attachment 157583 [details]
Patch

Bots are green. Setting r?, cq?.

[Dirk Schulze]

Comment on attachment 157583 [details]
Patch

Do you need to create a new header for this struct? Can't you put it into CustomFilterProgramInfo.h? Other than that the patch looks fine.

[Max Vujovic]

Created attachment 157779 [details]
Patch

(In reply to comment #6)
> (From update of attachment 157583 [details])
> Do you need to create a new header for this struct? Can't you put it into CustomFilterProgramInfo.h? Other than that the patch looks fine.

It doesn't need a new header file. I've updated the patch and moved it to CustomFilterProgramInfo.h.

I've also made one other change since the last patch. I've added m_mixSettings.enabled to the hash because I realized the following (a) and (b) were hashing to the same value without it:

(a)
m_mixSettings.enabled=1
m_mixSettings.blendMode=0
m_mixSettings.compositeOperator=0

(b)
m_mixSettings.enabled=0
m_mixSettings.blendMode=anything
m_mixSettings.compositeOperator=anything

[Dirk Schulze]

Comment on attachment 157779 [details]
Patch

LGTM. r=me.

[WebKit Review Bot]

Comment on attachment 157779 [details]
Patch

Clearing flags on attachment: 157779

Committed r125331: <http://trac.webkit.org/changeset/125331>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.