Closed Bug 1427668 Opened 2 years ago Closed 2 years ago

Move WebGL backbuffer handling into WebGLContext

Categories

(Core :: Canvas: WebGL, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

(Blocks 1 open bug)

Details

(Whiteboard: gfx-noted)

Attachments

(14 files, 2 obsolete files)

59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
This makes it easier for WebGL to control its backbuffer, as well as paving the way for removing GLScreenBuffer, and potentially migrating away from SharedSurface.
Comment on attachment 8939450 [details]
Bug 1427668 - Move the webgl backbuffer into webgl. -

https://reviewboard.mozilla.org/r/209780/#review215332

r=me after fixing the nit at moz.build.

::: dom/canvas/moz.build:204
(Diff revision 1)
>  include('/ipc/chromium/chromium-config.mozbuild')
>  
>  FINAL_LIBRARY = 'xul'
>  LOCAL_INCLUDES += [
>      '../workers',
> +    '/',

It looks like no one needs to include header files from `/`.
Attachment #8939450 - Flags: review?(dmu) → review+
Comment on attachment 8939451 [details]
Bug 1427668 - Reject too-large MozFramebuffer requests. -

https://reviewboard.mozilla.org/r/209782/#review215334
Attachment #8939451 - Flags: review?(dmu) → review+
Comment on attachment 8939452 [details]
Bug 1427668 - ColorMask(0xf) for backbuffer resolve. -

https://reviewboard.mozilla.org/r/209784/#review215612
Attachment #8939452 - Flags: review?(dmu) → review+
Comment on attachment 8939453 [details]
Bug 1427668 - InvalidateFB should accept incomplete FBs, but just skip them for now. -

https://reviewboard.mozilla.org/r/209786/#review215616
Attachment #8939453 - Flags: review?(dmu) → review+
Comment on attachment 8939454 [details]
Bug 1427668 - Use shadows instead of calling into GL. -

https://reviewboard.mozilla.org/r/209788/#review215622

::: dom/canvas/WebGLContextState.cpp:377
(Diff revision 2)
> +        case LOCAL_GL_SAMPLES: {
> +            const auto& fb = mBoundDrawFramebuffer;
> +            auto samples = [&]() -> uint32_t {
> +                if (!fb) {
> +                    if (!EnsureDefaultFB())
> +                        return 0;

Please replace 0 with JS::NullValue();

::: dom/canvas/WebGLContextState.cpp:382
(Diff revision 2)
> +                        return 0;
> +                    return mDefaultFB->mSamples;
> +                }
> +
> +                if (!fb->IsCheckFramebufferStatusComplete(funcName))
> +                    return 0;

Please replace 0 with JS::NullValue();
Attachment #8939454 - Flags: review?(dmu) → review+
Comment on attachment 8939455 [details]
Bug 1427668 - Special-case lazy glEnable caps. -

https://reviewboard.mozilla.org/r/209790/#review215628
Attachment #8939455 - Flags: review?(dmu) → review+
Comment on attachment 8939456 [details]
Bug 1427668 - Unify MaxRenderbufferSize and MaxTextureSize. -

https://reviewboard.mozilla.org/r/209792/#review215630
Attachment #8939456 - Flags: review?(dmu) → review+
Comment on attachment 8939458 [details]
Bug 1427668 - Lose context if EnsureDefaultFB fails. -

https://reviewboard.mozilla.org/r/209796/#review215634
Attachment #8939458 - Flags: review?(dmu) → review+
Comment on attachment 8939459 [details]
Bug 1427668 - Assert that no-alpha backbuffers have 0xff alpha. -

https://reviewboard.mozilla.org/r/209798/#review215636

::: gfx/gl/MozFramebuffer.cpp:10
(Diff revision 2)
>  
>  #include "MozFramebuffer.h"
>  
>  #include "GLContext.h"
> +#include "mozilla/gfx/Logging.h"
>  #include "ScopedGLHelpers.h"

Need include this header? It might be for other patch because it seems to be unnecessary here.
Attachment #8939459 - Flags: review?(dmu) → review+
Comment on attachment 8939460 [details]
Bug 1427668 - Disable MakeCurrent TLS for ANGLE for now. -

https://reviewboard.mozilla.org/r/209800/#review215638
Attachment #8939460 - Flags: review?(dmu) → review+
Comment on attachment 8939462 [details]
Bug 1427668 - Require frag_color_float for color_buffer_*float extensions. -

https://reviewboard.mozilla.org/r/209804/#review215654

lgtm. Please remove printf_stderr() that you don't need.

::: dom/canvas/WebGLContextFramebufferOperations.cpp:112
(Diff revision 2)
>      mColorClearValue[2] = b;
>      mColorClearValue[3] = a;
> +    printf_stderr("mColorClearValue(%f,%f,%f,%f)\n",
> +                  mColorClearValue[0], mColorClearValue[1],
> +                  mColorClearValue[2], mColorClearValue[3]);
>  }

All of these printf_stderr() look like for your debug. You should remove them.

::: gfx/gl/GLContext.h:967
(Diff revision 2)
> +
> +        float driverVals[4] = {};
> +        mSymbols.fGetFloatv(LOCAL_GL_COLOR_CLEAR_VALUE, driverVals);
> +        printf_stderr("%p->~glClearColor(%f,%f,%f,%f)\n", this,
> +                      driverVals[0], driverVals[1], driverVals[2], driverVals[3]);
>          AFTER_GL_CALL;

These two printf_stderr() looks like for debugging, please remove them.
Attachment #8939462 - Flags: review?(dmu) → review+
Comment on attachment 8939705 [details]
Bug 1427668 - Flush on mac after EndTransformFeedback. -

https://reviewboard.mozilla.org/r/210010/#review215664
Attachment #8939705 - Flags: review?(dmu) → review+
Comment on attachment 8939450 [details]
Bug 1427668 - Move the webgl backbuffer into webgl. -

https://reviewboard.mozilla.org/r/209780/#review215332

> It looks like no one needs to include header files from `/`.

This allows including as gfx/gl/MozFramebuffer.h instead of messing with header exports.
Comment on attachment 8939454 [details]
Bug 1427668 - Use shadows instead of calling into GL. -

https://reviewboard.mozilla.org/r/209788/#review215622

> Please replace 0 with JS::NullValue();

So we only need to return null (probably?) on context loss. If EnsureDefaultFB fails, the context is lost, though, so that at least must handle returning null.
Attachment #8939457 - Attachment is obsolete: true
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32a6eebb4d6d
Add MozFramebuffer. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2b263515feb
Move the webgl backbuffer into webgl. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d1558337a61
Reject too-large MozFramebuffer requests. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b126d984d82
ColorMask(0xf) for backbuffer resolve. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/94193e9d22f3
InvalidateFB should accept incomplete FBs, but just skip them for now. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/19c6be93dcad
Use shadows instead of calling into GL. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/cee0e8e73e1e
Special-case lazy glEnable caps. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/e31cefdad582
Unify MaxRenderbufferSize and MaxTextureSize. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/25e41ca9c92a
Lose context if EnsureDefaultFB fails. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1edf80827aa
Assert that no-alpha backbuffers have 0xff alpha. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/1626b467a127
Disable MakeCurrent TLS for ANGLE for now. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/86488a02187a
Fixes and spew. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8afea9b0875
Require frag_color_float for color_buffer_*float extensions. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba0c8df9a67
Flush on mac after EndTransformFeedback. - r=daoshengmu
Strangely that busted Thunderbird builds:
mozilla/dom/canvas/WebGLContext.cpp(17): fatal error C1083: Cannot open include file: 'gfx/gl/MozFramebuffer.h': No such file or directory
From
https://hg.mozilla.org/mozilla-central/rev/e2b263515feb#l5.12
New file here:
https://hg.mozilla.org/mozilla-central/rev/32a6eebb4d6d#l3.1

I don't quite understand what's going on.
Flags: needinfo?(mozilla)
Flags: needinfo?(jgilbert)
Flags: needinfo?(dmu)
It appears to be a "unified source" issue. Locally I get
53:59.24 c:/mozilla-source/comm-central/mozilla/dom/canvas/WebGLContext.cpp(17): fatal error C1083: Cannot open include file: 'gfx/gl/MozFramebuffer.h': No such file or directory
53:59.24 c:/mozilla-source/comm-central/mozilla/config/rules.mk:1040: recipe for target 'Unified_cpp_dom_canvas1.obj' failed
53:59.24 mozmake[4]: *** [Unified_cpp_dom_canvas1.obj] Error 2
53:59.26 mozmake[4]: *** Waiting for unfinished jobs....

Let me see whether I can find a patch to fix this.
Attached patch 1427668-hack.patch (obsolete) — Splinter Review
Hack to get TB to compile: added the file to dom/canvas/gfx/gl/.

I'm still puzzled why FF compiles.
I've filed bug 1428608 for a follow-up. I'll leave the NI in place to avoid further noise. Please head over to bug 1428608.
Clearing NI since this is being taken care of in bug 1428608. Sorry about the noise.
Flags: needinfo?(mozilla)
Flags: needinfo?(jgilbert)
Flags: needinfo?(dmu)
Depends on: 1428746
Blocks: 1428746
No longer depends on: 1428746
Duplicate of this bug: 1441806
Depends on: 1467406
Depends on: 1506665
Duplicate of this bug: 894114
Duplicate of this bug: 774058
Duplicate of this bug: 982477
You need to log in before you can comment on or make changes to this bug.