Bug 149424 - CSSFilterImageValue::image makes an unconditionally unaccelerated ImageBuffer
Summary: CSSFilterImageValue::image makes an unconditionally unaccelerated ImageBuffer
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-21 15:40 PDT by Tim Horton
Modified: 2022-01-19 12:49 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.91 KB, patch)
2020-09-03 11:22 PDT, frankhome61
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-09-21 15:40:32 PDT
The ImageBuffer should inherit the accelerated bit from the context it will eventually be painted into.
Comment 1 frankhome61 2020-09-03 11:22:03 PDT
Created attachment 407897 [details]
Patch
Comment 2 frankhome61 2020-09-03 11:23:43 PDT
Should just consult renderer.page().settings() to get the rendering mode setting.
Comment 3 Simon Fraser (smfr) 2020-09-03 11:39:42 PDT
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.
Comment 4 frankhome61 2020-09-03 11:44:03 PDT
(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.
Comment 5 frankhome61 2020-09-03 11:45:19 PDT
(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
Comment 6 frankhome61 2020-09-03 11:47:18 PDT
(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.
Comment 7 Simon Fraser (smfr) 2020-09-03 11:55:51 PDT
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.
Comment 8 frankhome61 2020-09-03 11:57:24 PDT
(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
Comment 9 frankhome61 2020-09-03 11:57:34 PDT
(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"