Bug 87878 - [CSS Shaders] Create a shared object between FECustomFilter objects to store the shared resources
Summary: [CSS Shaders] Create a shared object between FECustomFilter objects to store ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords:
: 85013 (view as bug list)
Depends on:
Blocks: 85013
  Show dependency treegraph
 
Reported: 2012-05-30 11:12 PDT by Alexandru Chiculita
Modified: 2012-06-20 09:46 PDT (History)
6 users (show)

See Also:


Attachments
Patch V1 (33.79 KB, patch)
2012-05-31 16:19 PDT, Alexandru Chiculita
achicu: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (740.64 KB, application/zip)
2012-05-31 21:05 PDT, WebKit Review Bot
no flags Details
Patch V2 (24.95 KB, patch)
2012-06-06 11:22 PDT, Alexandru Chiculita
dino: review+
Details | Formatted Diff | Diff
Patch to commit (25.00 KB, patch)
2012-06-19 13:44 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2012-05-30 11:12:32 PDT
Each FECustomFilter is creating its own GraphicsContext3D and always compiles the Shader. There should be only one GraphicsContext3D for all the FECustomFilters and they should share the precompiled shaders.
Comment 1 Alexandru Chiculita 2012-05-31 16:19:21 PDT
Created attachment 145173 [details]
Patch V1
Comment 2 WebKit Review Bot 2012-05-31 21:05:07 PDT
Comment on attachment 145173 [details]
Patch V1

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

New failing tests:
http/tests/media/media-source/video-media-source-event-attributes.html
Comment 3 WebKit Review Bot 2012-05-31 21:05:12 PDT
Created attachment 145205 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 4 Alexandru Chiculita 2012-06-01 09:25:36 PDT
(In reply to comment #3)
> Created an attachment (id=145205) [details]
> Archive of layout-test-results from ec2-cr-linux-01
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

This seems to be flaky. I've seen it on other bugs.
Comment 5 Dean Jackson 2012-06-01 09:59:45 PDT
Comment on attachment 145173 [details]
Patch V1

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

I think it looks ok. I'd like to get some more hints about what things are coming in the future so it doesn't look like you're just adding a single function that you'll remove later.

I'm hesitant to mark r+ because I worry about adding the new inheritance to FrameView.

> Source/WebCore/ChangeLog:13
> +        OwnPtr<CustomFilterHost>. Currently it only contains a single function that returns the HostWindow required 
> +        to create GraphicsContext3D objects.

Could you mention what things will be added in the future?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:1930
> +		7E99AF510B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7E99AF520B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp */; };
> +		7E99AF530B13846468FB01A5 /* WindowFocusAllowedIndicator.h in Headers */ = {isa = PBXBuildFile; fileRef = 7E99AF540B13846468FB01A5 /* WindowFocusAllowedIndicator.h */; settings = {ATTRIBUTES = (Private, ); }; };

Oops.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-5431
> -		7E99AF510B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7E99AF520B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp */; };
> -		7E99AF530B13846468FB01A5 /* WindowFocusAllowedIndicator.h in Headers */ = {isa = PBXBuildFile; fileRef = 7E99AF540B13846468FB01A5 /* WindowFocusAllowedIndicator.h */; settings = {ATTRIBUTES = (Private, ); }; };

Oops.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:9009
> +		7E99AF520B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WindowFocusAllowedIndicator.cpp; sourceTree = "<group>"; };
> +		7E99AF540B13846468FB01A5 /* WindowFocusAllowedIndicator.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WindowFocusAllowedIndicator.h; sourceTree = "<group>"; };

Oops.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-12570
> -		7E99AF520B13846468FB01A5 /* WindowFocusAllowedIndicator.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WindowFocusAllowedIndicator.cpp; sourceTree = "<group>"; };
> -		7E99AF540B13846468FB01A5 /* WindowFocusAllowedIndicator.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WindowFocusAllowedIndicator.h; sourceTree = "<group>"; };

Oops.

> Source/WebCore/page/FrameView.h:61
> -class FrameView : public ScrollView {
> +class FrameView : public ScrollView 
> +#if ENABLE(CSS_SHADERS) && ENABLE(WEBGL)
> +    , public CustomFilterHostHandler
> +#endif

I'd like someone who knows more about FrameView to make sure this is ok.
Comment 6 Alexandru Chiculita 2012-06-01 13:43:55 PDT
Comment on attachment 145173 [details]
Patch V1

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

Thanks! I will update the patch and ask someone else for the FrameView.

>> Source/WebCore/ChangeLog:13
>> +        to create GraphicsContext3D objects.
> 
> Could you mention what things will be added in the future?

Sure.

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-5431
>> -		7E99AF530B13846468FB01A5 /* WindowFocusAllowedIndicator.h in Headers */ = {isa = PBXBuildFile; fileRef = 7E99AF540B13846468FB01A5 /* WindowFocusAllowedIndicator.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> Oops.

Sorry, I usually check the patch for such things, but this time I missed it. xCode usually doesn't do that though.
Comment 7 Simon Fraser (smfr) 2012-06-01 13:49:54 PDT
Comment on attachment 145173 [details]
Patch V1

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

> Source/WebCore/platform/graphics/filters/CustomFilterHost.h:39
> +class CustomFilterHost {

"FilterHost" is a pretty nondescript name. It's really hard to tell what this object does from that name. What does it mean to "host" a filter?
Comment 8 Alexandru Chiculita 2012-06-01 15:09:32 PDT
Comment on attachment 145173 [details]
Patch V1

I'm working on a fix.
Comment 9 Alexandru Chiculita 2012-06-06 11:22:30 PDT
Created attachment 146067 [details]
Patch V2
Comment 10 Dean Jackson 2012-06-15 14:44:01 PDT
Comment on attachment 146067 [details]
Patch V2

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

I'm interested to see what you think about changing the name, so I'm not marking as cq+. As I read more of the code, I don't think this new class controls anything, so Controller is the wrong name. But I didn't have any good ideas.

> Source/WebCore/ChangeLog:8
> +        An object called CustomFilterController is created the first time a new FECustomFilter is needed in a document.

I wonder if CustomFilterController is the best name. It's not really controlling anything - it's just holding the objects that are needed to process the custom filter. It could almost be called CustomFilterGlobalContext or something, because it holds things related to drawing and tied to the context that it creates.

> Source/WebCore/platform/graphics/filters/CustomFilterController.h:34
> +#include <wtf/PassRefPtr.h>

Do you need this include?

> Source/WebCore/platform/graphics/filters/CustomFilterController.h:50
> +    void prepareContextIfNeeded(HostWindow*);
> +private:

Need a space between here.
Comment 11 Alexandru Chiculita 2012-06-15 15:18:30 PDT
(In reply to comment #10)
> (From update of attachment 146067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146067&action=review

Thanks for the review!

> I'm interested to see what you think about changing the name, so I'm not marking as cq+. As I read more of the code, I don't think this new class controls anything, so Controller is the wrong name. But I didn't have any good ideas.

Yes, maybe CustomFilterGlobalContext makes more sense. It will also keep the precompiled shaders around, so CustomFilterGlobalContext makes more sense for that.

> > Source/WebCore/platform/graphics/filters/CustomFilterController.h:34
> > +#include <wtf/PassRefPtr.h>
> 
> Do you need this include?
Oops, I will check that before committing. 

> > Source/WebCore/platform/graphics/filters/CustomFilterController.h:50
> > +    void prepareContextIfNeeded(HostWindow*);
> > +private:
> 
> Need a space between here.

Ok.
Comment 12 Alexandru Chiculita 2012-06-19 13:44:30 PDT
Created attachment 148409 [details]
Patch to commit
Comment 13 WebKit Review Bot 2012-06-19 14:45:09 PDT
Comment on attachment 148409 [details]
Patch to commit

Clearing flags on attachment: 148409

Committed r120757: <http://trac.webkit.org/changeset/120757>
Comment 14 WebKit Review Bot 2012-06-19 14:45:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Alexandru Chiculita 2012-06-20 09:46:46 PDT
*** Bug 85013 has been marked as a duplicate of this bug. ***