Bug 110817 - Create GraphicsContext3DState to aggregate state objects
Summary: Create GraphicsContext3DState to aggregate state objects
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: Bruno Abinader (history only)
URL:
Keywords:
Depends on:
Blocks: 110883
  Show dependency treegraph
 
Reported: 2013-02-25 16:03 PST by Bruno Abinader (history only)
Modified: 2013-02-28 15:19 PST (History)
11 users (show)

See Also:


Attachments
Patch (26.07 KB, patch)
2013-02-25 16:05 PST, Bruno Abinader (history only)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bruno Abinader (history only) 2013-02-25 16:03:27 PST
Aggregate context state-related objects on a GraphicsContext3DState struct, in a similar fashion as GraphicsContext does. This is useful to avoid duplicated values for platform-specific initialization lists.
Comment 1 Bruno Abinader (history only) 2013-02-25 16:05:13 PST
Created attachment 190145 [details]
Patch

Proposed patch
Comment 2 Antonio Gomes 2013-02-26 06:34:13 PST
(In reply to comment #1)
> Created an attachment (id=190145) [details]
> Patch
> 
> Proposed patch

LGTM, but I will defer to No'am or Mrobinson a final word.
Comment 3 Martin Robinson 2013-02-26 08:03:54 PST
Comment on attachment 190145 [details]
Patch

I'm not sure adding a new layer in an already byzantine class hierarchy is a good tradeoff for deleting six lines of code. I'm not going to block this patch, but I thought I'd mention that.
Comment 4 Noam Rosenthal 2013-02-26 08:07:50 PST
(In reply to comment #3)
> (From update of attachment 190145 [details])
> I'm not sure adding a new layer in an already byzantine class hierarchy is a good tradeoff for deleting six lines of code. I'm not going to block this patch, but I thought I'd mention that.

I agree - something about this seems like refactoring for the sake of refactoring.
Comment 5 Bruno Abinader (history only) 2013-02-26 08:08:16 PST
(In reply to comment #3)
> (From update of attachment 190145 [details])
> I'm not sure adding a new layer in an already byzantine class hierarchy is a good tradeoff for deleting six lines of code. I'm not going to block this patch, but I thought I'd mention that.

The GC3DState struct is also the baseline for an upcoming effort to add more state variables to avoid redundant state changes not covered by this patch (I'll create a bug related to this soon), so in fact the removal of the duplicated values is a plus, not the real goal.
Comment 6 Martin Robinson 2013-02-26 08:17:32 PST
(In reply to comment #5)
 
> The GC3DState struct is also the baseline for an upcoming effort to add more state variables to avoid redundant state changes not covered by this patch (I'll create a bug related to this soon), so in fact the removal of the duplicated values is a plus, not the real goal.

Redundantly setting the bound texture, the active texture unit, or the FBO should be fast, unless you have some really poor drivers.
Comment 7 Bruno Abinader (history only) 2013-02-26 08:22:51 PST
(In reply to comment #6)
> (In reply to comment #5)
> 
> > The GC3DState struct is also the baseline for an upcoming effort to add more state variables to avoid redundant state changes not covered by this patch (I'll create a bug related to this soon), so in fact the removal of the duplicated values is a plus, not the real goal.
> 
> Redundantly setting the bound texture, the active texture unit, or the FBO should be fast, unless you have some really poor drivers.

These are not the target of bug 110883. I'll cover for now just the bottlenecks I've been detecting by profiling EFL's MiniBrowser under gDEBugger tool (so far: blend, stencil, scissor, depth mask-related functions). I've moved these variables to GC3DState for readibility, but I can update the patch just to create GC3DState struct if that's the case.
Comment 8 Bruno Abinader (history only) 2013-02-26 08:23:54 PST
s/readibility/readability/ :)
Comment 9 Bruno Abinader (history only) 2013-02-26 08:31:34 PST
I've uploaded the proposed patch on bug 110883 for your appreciation (depends on the proposed patch from this bug, as it creates the GC3DState struct).
Comment 10 Bruno Abinader (history only) 2013-02-28 08:04:02 PST
@Martin, @Kenneth: I've spoke w/ No'am today about this, he seems fine with the changes (this patch and the complimentary in bug 110883). I agree these changes gives minimum or no optimization on well-implemented drivers, but for others, this could reduce the amount of redundant state changes (speaking of bug 110883). Speaking of which, I've splitted the code in two for clarity, but if it makes more sense to aggregate both patches, please tell me so I can do so.
Comment 11 Kenneth Russell 2013-02-28 14:08:59 PST
Comment on attachment 190145 [details]
Patch

This refactoring looks good.
Comment 12 WebKit Review Bot 2013-02-28 15:19:32 PST
Comment on attachment 190145 [details]
Patch

Clearing flags on attachment: 190145

Committed r144378: <http://trac.webkit.org/changeset/144378>
Comment 13 WebKit Review Bot 2013-02-28 15:19:37 PST
All reviewed patches have been landed.  Closing bug.