821193 - Don't shadow framebuffer bindings in GLContext

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

We should stop shadowing framebuffer bindings in GLContext. We've been lucky so far, but lazy and/or async GL errors make this impractical to assure.

We should switch from shadow-and-emulate to emulate-by-translation. ('emulate' referring to our framebuffer zero trick for offscreen buffers)

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: translate instead of shadowing

You can also reassign this review to someone else, if you have any ideas.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Can you just share a little more insights to help the reviewing. What is the conceptual difference between shadow-and-emulate and emulate-by-translation? What is specificly the problem in the old approach?

Comment 3

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #2)
> Can you just share a little more insights to help the reviewing. What is the
> conceptual difference between shadow-and-emulate and emulate-by-translation?
> What is specificly the problem in the old approach?

Sure. Right now, we intercept calls getting and setting the framebuffer binding points such that, when the user of GLContext tries to bind FB 0 (on an offscreen context), we pretend they did, but behind the scenes, we actually bind our offscreen buffers. Likewise, when they query the current binding, we 'tell them what they want to hear'. If they bound 0 before, we tell them 0 is still bound, even if we've actually bound other framebuffers instead.

This approach means that we either need to track which buffer they've requested we bind, or instead translate back and forth between GL's actual bound FB and what the user should see it as. Currently, we do the former.

The problem is that when BindFramebuffer generates an error, it usually doesn't change the currently bound buffer.[1] This means that if we're shadowing, we need to detect these errors, or we will get mispredictions. Detecting these errors means that we *must* call GetError, since we can't easily predict all errors at bind-time. (and we can't at all predict OOMs)

Instead, we should switch to translating, where we query GL to find out which FB is actually bound, and then translate that value into the value the clients of GLContext expect. (If GL tells us the offscreen FB is bound, we tell the clients that 0 is bound) Likewise, for BindFramebuffer, we intercept calls to bind FB 0, and instead bind our offscreen FB(s).

[1] OOM errors leave things undefined when they happen, unlike other errors. Since GL will often lazily allocate buffers, we can get OOM errors at BindFramebuffer, when GL decides to actually allocate the data needed.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Thanks for the explanation, that makes a lot of sense, and sorry for the reviewing delay, b2g + vacations. reviewing now.

Comment 5

[Kelsey Gilbert [:jgilbert]]

We basically stopped doing this via GLScreenBuffer, and are in the process of deprecating that too.