997374 - Check FB status in GLScreenBuffer

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Right now, we assert that our FBs are complete. They might not be, so we should check, and fail gracefully if they fail.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: check-fb

Comment 2

[Kelsey Gilbert [:jgilbert]]

I started hitting these asserts in bug 996266, with this try run on WinXP:
https://tbpl.mozilla.org/?tree=Try&rev=7196140c4056

Comment 3

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

Comment on attachment 8407770 [details] [diff] [review]
check-fb

Review of attachment 8407770 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLScreenBuffer.cpp
@@ +405,3 @@
>          ReadBuffer* read = CreateRead(surf);
> +        if (!read)
> +            ok = false;

Rather than having a single 'ok' variable that gets mutated, changing its meaning, I would rather have separate immutable bools. It would make the flow of logic easier for me to follow, and the names of these variables would have to be more specific, self-documenting them. Does that make sense?

bool drawOk = CreateDraw(size, &draw);
ReadBuffer* read = CreateRead(surf);
bool readOk = read != nullptr;
bool ok = drawOk && readOk;

::: gfx/gl/GLScreenBuffer.h
@@ +51,5 @@
> +    static bool Create(GLContext* const gl,
> +                       const SurfaceCaps& caps,
> +                       const GLFormats& formats,
> +                       const gfx::IntSize& size,
> +                       DrawBuffer** out_buffer);

A two-star michelin pointer. Delicious.

Comment 4

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8871dd41a10

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8871dd41a10