implement webgl required clear semantics

RESOLVED FIXED

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: vlad, Assigned: bjacob)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

10.54 KB, patch
bjacob
: review-
Details | Diff | Splinter Review
5.04 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
6.93 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
9.34 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
12.15 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
929 bytes, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
Created attachment 515264 [details] [diff] [review]
preserveDrwaingBuffe

preserveDrawingBuffer and friends
(Assignee)

Updated

6 years ago
Attachment #515264 - Flags: review?(bjacob)
(Assignee)

Comment 1

6 years ago
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.
Attachment #515264 - Flags: review?(bjacob) → review-
(Assignee)

Comment 2

6 years ago
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.
Attachment #516707 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #516707 - Flags: review? → review?(joe)
Attachment #516707 - Flags: review?(joe) → review+
(Assignee)

Comment 3

6 years ago
Created attachment 531331 [details] [diff] [review]
Part 2: state tracking

Keep track of the state bits that we need to implement buffer clear semantics.
Attachment #531331 - Flags: review?(joe)
(Assignee)

Comment 4

6 years ago
Created attachment 531351 [details] [diff] [review]
Part 2: state tracking
Attachment #531331 - Attachment is obsolete: true
Attachment #531331 - Flags: review?(joe)
Attachment #531351 - Flags: review?(joe)
(Assignee)

Comment 5

6 years ago
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.
Attachment #531354 - Flags: review?(joe)
(Assignee)

Comment 6

6 years ago
Created attachment 531448 [details] [diff] [review]
Part 3: implement ForceClearFramebufferWithDefaultValues from the existing renderbuffer init code

Updated, fixed bugs
Attachment #531354 - Attachment is obsolete: true
Attachment #531354 - Flags: review?(joe)
Attachment #531448 - Flags: review?(joe)
(Assignee)

Comment 7

6 years ago
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).
Attachment #531451 - Flags: review?(joe)
(Assignee)

Comment 8

6 years ago
Try push (will be orange, have yet to update the list of failing test):
http://tbpl.mozilla.org/?tree=Try&rev=b69937feecf8
(Assignee)

Comment 9

6 years ago
Hm, the gl-clear test is failing.
Comment on attachment 531448 [details] [diff] [review]
Part 3: implement ForceClearFramebufferWithDefaultValues from the existing renderbuffer init code

Review of attachment 531448 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #531448 - Flags: review?(joe) → review+
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.
Attachment #531351 - Flags: review?(joe) → review+
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.
Attachment #531451 - Flags: review?(joe)
Created attachment 533990 [details] [diff] [review]
Part 4: implement the clear semantics (updated)

Now fixed and passing all the tests.
Attachment #531451 - Attachment is obsolete: true
Attachment #533990 - Flags: review?(joe)
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.
Attachment #533991 - Flags: review?(joe)
tryserver push, together with other stuff I would like to push today:
http://tbpl.mozilla.org/?tree=Try&rev=08d68b9482c0
Comment on attachment 533990 [details] [diff] [review]
Part 4: implement the clear semantics (updated)

Review of attachment 533990 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #533990 - Flags: review?(joe) → review+
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]:
-----------------------------------------------------------------
Attachment #533991 - Flags: review?(joe) → review+
http://hg.mozilla.org/mozilla-central/rev/d82cec64c7d0
http://hg.mozilla.org/mozilla-central/rev/72d503e4c89e
http://hg.mozilla.org/mozilla-central/rev/4769b42aa4cd
http://hg.mozilla.org/mozilla-central/rev/f13953f3c671
http://hg.mozilla.org/mozilla-central/rev/e7768409bc6a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Assignee: nobody → bjacob
You need to log in before you can comment on or make changes to this bug.