990283 - Use GLDrawRectHelper in GLBlitHelper and remove some excessive state saving

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Andreas Gal :gal]

We don't really do all this state saving anywhere else either.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8399682 [details] [diff] [review]
patch

Pretty sure this is all being saved since this function might get called on a WebGL context, which will need to have its own state preserved (due to being used in SharedSurfaceGL).  Jeff, is that true?  If we're using SharedSurfaceGL, I assume with context sharing, could this blit function be called from the WebGL context itself?

Comment 3

[Kelsey Gilbert [:jgilbert]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #2)
> Comment on attachment 8399682 [details] [diff] [review]
> patch
> 
> Pretty sure this is all being saved since this function might get called on
> a WebGL context, which will need to have its own state preserved (due to
> being used in SharedSurfaceGL).  Jeff, is that true?  If we're using
> SharedSurfaceGL, I assume with context sharing, could this blit function be
> called from the WebGL context itself?

This is correct. We call this on the WebGL GLContext, so we have to restore all state. WebGL should be tracking (most of) this state, and some of this state might actually be being cached in GLContext now. (we started down this path, at least) 

Even when we don't use context sharing, we still use these sorts of functions for doing copies for PreserveDrawingBuffer:true cases. That said, this function is a path we're hopefully not using if we can avoid it, since BlitFramebuffer and CopyTexImage are both faster than drawing.

Since this is called on WebGL GLContexts, we have to restore all state that we change, and set our own temporary state such that no WebGL state will change how this draw behaves. (blend, scissor, colorMask, etc)

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8399682 [details] [diff] [review]
patch

So r- based on Jeff's comment above.. we do actually need to save and explicitly set all this state, unfortunately.

Comment 5

[Kelsey Gilbert [:jgilbert]]

If there's a case (compositor) where we shouldn't need to state-save, we can just split this into two functions, something like:
Blit() // doesn't save-restore state
SafeBlit() // save-restores all state

SafeBlit can save and restore, and call Blit to share the code.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:bhood, could you have a look please?
For more information, please visit auto_nag documentation.