Bug 110817 – Create GraphicsContext3DState to aggregate state objects

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

Created attachment 190145 [details]
Patch

Proposed patch

[Antonio Gomes]

(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.

[Martin Robinson]

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.

[Noam Rosenthal]

(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.

[Bruno Abinader (history only)]

(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.

[Martin Robinson]

(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.

[Bruno Abinader (history only)]

(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.

[Bruno Abinader (history only)]

s/readibility/readability/ :)

[Bruno Abinader (history only)]

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).

[Bruno Abinader (history only)]

@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.

[Kenneth Russell]

Comment on attachment 190145 [details]
Patch

This refactoring looks good.

[WebKit Review Bot]

Comment on attachment 190145 [details]
Patch

Clearing flags on attachment: 190145

Committed r144378: <http://trac.webkit.org/changeset/144378>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.