Bug 149424 – CSSFilterImageValue::image makes an unconditionally unaccelerated ImageBuffer

• Component: CSS
• Status: NEW

[Tim Horton]

The ImageBuffer should inherit the accelerated bit from the context it will eventually be painted into.

Created attachment 407897 [details]
Patch

Should just consult renderer.page().settings() to get the rendering mode setting.

[Simon Fraser (smfr)]

Comment on attachment 407897 [details]
Patch

This seems wrong. It should only make an accelerated buffer if we know the filter chain can be rendered via Core Image.

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 407897 [details]
> Patch
> 
> This seems wrong. It should only make an accelerated buffer if we know the
> filter chain can be rendered via Core Image.

Oh this has nothing to do with CoreImage, this is simply trying to address the fact that the ImageBuffer created inside CSSFilterImageValue is Unaccelerated unconditionally, and pass it on to CSSFilter. CoreImage is not involved in this process.

(In reply to guowei_yang from comment #4)
> (In reply to Simon Fraser (smfr) from comment #3)
> > Comment on attachment 407897 [details]
> > Patch
> > 
> > This seems wrong. It should only make an accelerated buffer if we know the
> > filter chain can be rendered via Core Image.
> 
> Oh this has nothing to do with CoreImage, this is simply trying to address
> the fact that the ImageBuffer created inside CSSFilterImageValue is
> Unaccelerated unconditionally, and pass it on to CSSFilter. CoreImage is not
> involved in this process.

P.S. this patch has nothing to do with my project, just something I spotted while looking through different files

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 407897 [details]
> Patch
> 
> This seems wrong. It should only make an accelerated buffer if we know the
> filter chain can be rendered via Core Image.

Also before we introduced CoreImage into the codebase, CSSFilter::allocateBackingStoreIfNeeded() also consults renderer.page().settings() to determine which kind of ImageBuffer backend to create. So I would only assume the same procedure should happen here.

[Simon Fraser (smfr)]

settings().acceleratedFiltersEnabled() was added for another platform and used to do nothing on Apple platforms. You've started to use it for the CI acceleration, which is fine.

However, we know this code path (on Apple platforms) always hits the non-accelerated filter path, so we should not use this switch to make an accelerated buffer; that will slow things down.

(In reply to Simon Fraser (smfr) from comment #7)
> settings().acceleratedFiltersEnabled() was added for another platform and
> used to do nothing on Apple platforms. You've started to use it for the CI
> acceleration, which is fine.
> 
> However, we know this code path (on Apple platforms) always hits the
> non-accelerated filter path, so we should not use this switch to make an
> accelerated buffer; that will slow things down.

Oh okay. I guess this bug should be ignored then

(In reply to guowei_yang from comment #8)
> (In reply to Simon Fraser (smfr) from comment #7)
> > settings().acceleratedFiltersEnabled() was added for another platform and
> > used to do nothing on Apple platforms. You've started to use it for the CI
> > acceleration, which is fine.
> > 
> > However, we know this code path (on Apple platforms) always hits the
> > non-accelerated filter path, so we should not use this switch to make an
> > accelerated buffer; that will slow things down.
> 
> Oh okay. I guess this bug should be ignored then

"bug"