Last Comment Bug 636913 - implement webgl required clear semantics
: implement webgl required clear semantics
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-25 17:46 PST by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2011-06-11 00:37 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
preserveDrwaingBuffe (10.54 KB, patch)
2011-02-25 17:46 PST, Vladimir Vukicevic [:vlad] [:vladv]
jacob.benoit.1: review-
Details | Diff | Splinter Review
part 1: fix context options (5.04 KB, patch)
2011-03-03 14:26 PST, Benoit Jacob [:bjacob] (mostly away)
joe: review+
Details | Diff | Splinter Review
Part 2: state tracking (10.56 KB, patch)
2011-05-10 08:11 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
Part 2: state tracking (6.93 KB, patch)
2011-05-10 09:39 PDT, Benoit Jacob [:bjacob] (mostly away)
joe: review+
Details | Diff | Splinter Review
Part 3: implement ForceClearFramebufferWithDefaultValues from the existing renderbuffer init code (9.31 KB, patch)
2011-05-10 09:43 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
Part 3: implement ForceClearFramebufferWithDefaultValues from the existing renderbuffer init code (9.34 KB, patch)
2011-05-10 13:39 PDT, Benoit Jacob [:bjacob] (mostly away)
joe: review+
Details | Diff | Splinter Review
Part 4: implement the clear semantics (8.13 KB, patch)
2011-05-10 13:42 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
Part 4: implement the clear semantics (updated) (12.15 KB, patch)
2011-05-20 08:57 PDT, Benoit Jacob [:bjacob] (mostly away)
joe: review+
Details | Diff | Splinter Review
Part 5: fix buffer-preserve-test.html so it works in the mochitest (929 bytes, patch)
2011-05-20 09:01 PDT, Benoit Jacob [:bjacob] (mostly away)
joe: review+
Details | Diff | Splinter Review

Description Vladimir Vukicevic [:vlad] [:vladv] 2011-02-25 17:46:27 PST
Created attachment 515264 [details] [diff] [review]
preserveDrwaingBuffe

preserveDrawingBuffer and friends
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2011-03-03 14:20:33 PST
Comment on attachment 515264 [details] [diff] [review]
preserveDrwaingBuffe

OK, had a look at the patch today: as Vlad said, there's a little bit of work left to do.
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2011-03-03 14:26:13 PST
Created attachment 516707 [details] [diff] [review]
part 1: fix context options

This first patch is easy: it fixes the WebGLContext options according to the spec, that is:
 * rename antialiasHint to antialias, default to true (the spec does not mandate antialiasing being actually happening)
 * add preserveDrawingBuffer

This patch also does this:
-    if (!GetBoolFromPropertyBag(aOptions, "stencil", &newOpts.stencil))
-        newOpts.stencil = false;
+    GetBoolFromPropertyBag(aOptions, "stencil", &newOpts.stencil);

This is from Vlad's patch. The point is that GetBoolFromPropertyBag already does nothing to the output value if it fails.
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2011-05-10 08:11:36 PDT
Created attachment 531331 [details] [diff] [review]
Part 2: state tracking

Keep track of the state bits that we need to implement buffer clear semantics.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2011-05-10 09:39:12 PDT
Created attachment 531351 [details] [diff] [review]
Part 2: state tracking
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2011-05-10 09:43:45 PDT
Created attachment 531354 [details] [diff] [review]
Part 3: implement ForceClearFramebufferWithDefaultValues from the existing renderbuffer init code

We will need this function to actually implement the buffer clearing. Vlad's patch was using GLContext::ClearSafely() and making it a little more powerful, but still not powerful enough. Meanwhile we already had the needed code in WebGL for renderbuffer initialization. So, this patch takes this reusable code into a self-contained function.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2011-05-10 13:39:29 PDT
Created attachment 531448 [details] [diff] [review]
Part 3: implement ForceClearFramebufferWithDefaultValues from the existing renderbuffer init code

Updated, fixed bugs
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2011-05-10 13:42:15 PDT
Created attachment 531451 [details] [diff] [review]
Part 4: implement the clear semantics

This is where we actually implement the clear semantics, i.e. the notion that after the WebGL back buffer has been presented to the compositor, if the preserveDrawingBuffer context option is false, before the next WebGL draw/clear/readPixels call, the back buffer must be cleared with the default values (color=black, depth=1.0, etc).
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-05-10 16:42:41 PDT
Try push (will be orange, have yet to update the list of failing test):
http://tbpl.mozilla.org/?tree=Try&rev=b69937feecf8
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2011-05-12 15:16:17 PDT
Hm, the gl-clear test is failing.
Comment 10 Joe Drew (not getting mail) 2011-05-18 08:55:56 PDT
Comment on attachment 531448 [details] [diff] [review]
Part 3: implement ForceClearFramebufferWithDefaultValues from the existing renderbuffer init code

Review of attachment 531448 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 11 Joe Drew (not getting mail) 2011-05-18 11:05:36 PDT
Comment on attachment 531351 [details] [diff] [review]
Part 2: state tracking

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

::: content/canvas/src/WebGLContext.cpp
@@ +127,5 @@
>      mFakeVertexAttrib0BufferObjectSize = 0;
>      mFakeVertexAttrib0BufferObject = 0;
>      mFakeVertexAttrib0BufferStatus = VertexAttrib0Status::Default;
> +
> +    // these are de default values, see 6.2 State tables in the OpenGL ES 2.0.25 spec

I did not verify these.
Comment 12 Joe Drew (not getting mail) 2011-05-18 11:10:51 PDT
Comment on attachment 531451 [details] [diff] [review]
Part 4: implement the clear semantics

Benoit tells me that there's going to have to be a new version of this patch due to test failures, so I'll look at this patch later.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2011-05-20 08:57:11 PDT
Created attachment 533990 [details] [diff] [review]
Part 4: implement the clear semantics (updated)

Now fixed and passing all the tests.
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2011-05-20 09:01:20 PDT
Created attachment 533991 [details] [diff] [review]
Part 5: fix buffer-preserve-test.html so it works in the mochitest

This test relies on the canvas getting composited. It worked fine standalone, but when included as a small frame in the test runner page, the canvas fell off the visible part and therefore wasn't getting composited. Fixing this by putting the canvas at the top of the page. Going to upstream this.
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2011-05-20 09:27:52 PDT
tryserver push, together with other stuff I would like to push today:
http://tbpl.mozilla.org/?tree=Try&rev=08d68b9482c0
Comment 16 Joe Drew (not getting mail) 2011-05-20 12:46:06 PDT
Comment on attachment 533990 [details] [diff] [review]
Part 4: implement the clear semantics (updated)

Review of attachment 533990 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 17 Joe Drew (not getting mail) 2011-05-20 12:46:56 PDT
Comment on attachment 533991 [details] [diff] [review]
Part 5: fix buffer-preserve-test.html so it works in the mochitest

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

Note You need to log in before you can comment on or make changes to this bug.