1427668 - Move WebGL backbuffer handling into WebGLContext

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement, P1)

Description

[Kelsey Gilbert [:jgilbert]]

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 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Add MozFramebuffer. -

Review commit: https://reviewboard.mozilla.org/r/209778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209778/

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Move the webgl backbuffer into webgl. -

Review commit: https://reviewboard.mozilla.org/r/209780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209780/

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Reject too-large MozFramebuffer requests. -

Review commit: https://reviewboard.mozilla.org/r/209782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209782/

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - ColorMask(0xf) for backbuffer resolve. -

Review commit: https://reviewboard.mozilla.org/r/209784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209784/

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - InvalidateFB should accept incomplete FBs, but just skip them for now. -

Review commit: https://reviewboard.mozilla.org/r/209786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209786/

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Use shadows instead of calling into GL. -

Review commit: https://reviewboard.mozilla.org/r/209788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209788/

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Special-case lazy glEnable caps. -

Review commit: https://reviewboard.mozilla.org/r/209790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209790/

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Unify MaxRenderbufferSize and MaxTextureSize. -

Review commit: https://reviewboard.mozilla.org/r/209792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209792/

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - More lazy state. -

Review commit: https://reviewboard.mozilla.org/r/209794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209794/

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Lose context if EnsureDefaultFB fails. -

Review commit: https://reviewboard.mozilla.org/r/209796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209796/

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Assert that no-alpha backbuffers have 0xff alpha. -

Review commit: https://reviewboard.mozilla.org/r/209798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209798/

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Disable MakeCurrent TLS for ANGLE for now. -

Review commit: https://reviewboard.mozilla.org/r/209800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209800/

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Fixes and spew. -

Review commit: https://reviewboard.mozilla.org/r/209802/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209802/

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Require frag_color_float for color_buffer_*float extensions. -

Add probe for faulty frag_color_float implementations.

Review commit: https://reviewboard.mozilla.org/r/209804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209804/

Comment 15

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8939449 [details]
Bug 1427668 - Add MozFramebuffer. -

https://reviewboard.mozilla.org/r/209778/#review215312

Comment 16

[Daosheng Mu[:daoshengmu]]

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 `/`.

Comment 17

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8939451 [details]
Bug 1427668 - Reject too-large MozFramebuffer requests. -

https://reviewboard.mozilla.org/r/209782/#review215334

Comment 18

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1427668 - Flush on mac after EndTransformFeedback. -

Review commit: https://reviewboard.mozilla.org/r/210010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210010/

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939449 [details]
Bug 1427668 - Add MozFramebuffer. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209778/diff/1-2/

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939450 [details]
Bug 1427668 - Move the webgl backbuffer into webgl. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209780/diff/1-2/

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939451 [details]
Bug 1427668 - Reject too-large MozFramebuffer requests. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209782/diff/1-2/

Comment 22

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939452 [details]
Bug 1427668 - ColorMask(0xf) for backbuffer resolve. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209784/diff/1-2/

Comment 23

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939453 [details]
Bug 1427668 - InvalidateFB should accept incomplete FBs, but just skip them for now. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209786/diff/1-2/

Comment 24

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939454 [details]
Bug 1427668 - Use shadows instead of calling into GL. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209788/diff/1-2/

Comment 25

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939455 [details]
Bug 1427668 - Special-case lazy glEnable caps. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209790/diff/1-2/

Comment 26

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939456 [details]
Bug 1427668 - Unify MaxRenderbufferSize and MaxTextureSize. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209792/diff/1-2/

Comment 27

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939457 [details]
Bug 1427668 - More lazy state. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209794/diff/1-2/

Comment 28

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939458 [details]
Bug 1427668 - Lose context if EnsureDefaultFB fails. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209796/diff/1-2/

Comment 29

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939459 [details]
Bug 1427668 - Assert that no-alpha backbuffers have 0xff alpha. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209798/diff/1-2/

Comment 30

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939460 [details]
Bug 1427668 - Disable MakeCurrent TLS for ANGLE for now. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209800/diff/1-2/

Comment 31

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939461 [details]
Bug 1427668 - Fixes and spew. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209802/diff/1-2/

Comment 32

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939462 [details]
Bug 1427668 - Require frag_color_float for color_buffer_*float extensions. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209804/diff/1-2/

Comment 33

[Kelsey Gilbert [:jgilbert]]

Green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=737cbbf350c7994975411d5aeef42272728ffdd7

Comment 34

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8939452 [details]
Bug 1427668 - ColorMask(0xf) for backbuffer resolve. -

https://reviewboard.mozilla.org/r/209784/#review215612

Comment 35

[Daosheng Mu[:daoshengmu]]

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

Comment 36

[Daosheng Mu[:daoshengmu]]

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();

Comment 37

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8939455 [details]
Bug 1427668 - Special-case lazy glEnable caps. -

https://reviewboard.mozilla.org/r/209790/#review215628

Comment 38

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8939456 [details]
Bug 1427668 - Unify MaxRenderbufferSize and MaxTextureSize. -

https://reviewboard.mozilla.org/r/209792/#review215630

Comment 39

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8939457 [details]
Bug 1427668 - More lazy state. -

https://reviewboard.mozilla.org/r/209794/#review215632

Comment 40

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8939458 [details]
Bug 1427668 - Lose context if EnsureDefaultFB fails. -

https://reviewboard.mozilla.org/r/209796/#review215634

Comment 41

[Daosheng Mu[:daoshengmu]]

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.

Comment 42

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8939460 [details]
Bug 1427668 - Disable MakeCurrent TLS for ANGLE for now. -

https://reviewboard.mozilla.org/r/209800/#review215638

Comment 43

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8939461 [details]
Bug 1427668 - Fixes and spew. -

https://reviewboard.mozilla.org/r/209802/#review215640

Comment 44

[Daosheng Mu[:daoshengmu]]

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.

Comment 45

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8939705 [details]
Bug 1427668 - Flush on mac after EndTransformFeedback. -

https://reviewboard.mozilla.org/r/210010/#review215664

Comment 46

[Kelsey Gilbert [:jgilbert]]

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 47

[Kelsey Gilbert [:jgilbert]]

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.

Comment 48

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939449 [details]
Bug 1427668 - Add MozFramebuffer. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209778/diff/2-3/

Comment 49

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939450 [details]
Bug 1427668 - Move the webgl backbuffer into webgl. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209780/diff/2-3/

Comment 50

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939451 [details]
Bug 1427668 - Reject too-large MozFramebuffer requests. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209782/diff/2-3/

Comment 51

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939452 [details]
Bug 1427668 - ColorMask(0xf) for backbuffer resolve. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209784/diff/2-3/

Comment 52

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939453 [details]
Bug 1427668 - InvalidateFB should accept incomplete FBs, but just skip them for now. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209786/diff/2-3/

Comment 53

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939454 [details]
Bug 1427668 - Use shadows instead of calling into GL. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209788/diff/2-3/

Comment 54

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939455 [details]
Bug 1427668 - Special-case lazy glEnable caps. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209790/diff/2-3/

Comment 55

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939456 [details]
Bug 1427668 - Unify MaxRenderbufferSize and MaxTextureSize. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209792/diff/2-3/

Comment 56

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939458 [details]
Bug 1427668 - Lose context if EnsureDefaultFB fails. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209796/diff/2-3/

Comment 57

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939459 [details]
Bug 1427668 - Assert that no-alpha backbuffers have 0xff alpha. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209798/diff/2-3/

Comment 58

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939460 [details]
Bug 1427668 - Disable MakeCurrent TLS for ANGLE for now. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209800/diff/2-3/

Comment 59

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939461 [details]
Bug 1427668 - Fixes and spew. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209802/diff/2-3/

Comment 60

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939462 [details]
Bug 1427668 - Require frag_color_float for color_buffer_*float extensions. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209804/diff/2-3/

Comment 61

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8939705 [details]
Bug 1427668 - Flush on mac after EndTransformFeedback. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210010/diff/1-2/

Comment 62

[Pulsebot]

Pushed by [e-mail]:
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

Comment 63

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/32a6eebb4d6d
https://hg.mozilla.org/mozilla-central/rev/e2b263515feb
https://hg.mozilla.org/mozilla-central/rev/3d1558337a61
https://hg.mozilla.org/mozilla-central/rev/2b126d984d82
https://hg.mozilla.org/mozilla-central/rev/94193e9d22f3
https://hg.mozilla.org/mozilla-central/rev/19c6be93dcad
https://hg.mozilla.org/mozilla-central/rev/cee0e8e73e1e
https://hg.mozilla.org/mozilla-central/rev/e31cefdad582
https://hg.mozilla.org/mozilla-central/rev/25e41ca9c92a
https://hg.mozilla.org/mozilla-central/rev/e1edf80827aa
https://hg.mozilla.org/mozilla-central/rev/1626b467a127
https://hg.mozilla.org/mozilla-central/rev/86488a02187a
https://hg.mozilla.org/mozilla-central/rev/e8afea9b0875
https://hg.mozilla.org/mozilla-central/rev/2ba0c8df9a67

Comment 64

[Jorg K (CEST = GMT+2)]

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.

Comment 65

[Jorg K (CEST = GMT+2)]

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.

Comment 66

[Jorg K (CEST = GMT+2)]

Attachment: 1427668-hack.patch

Hack to get TB to compile: added the file to dom/canvas/gfx/gl/.

I'm still puzzled why FF compiles.

Comment 67

[Tom Prince [:tomprince]]

This appears to be due to https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/context.py#664 and https://hg.mozilla.org/mozilla-central/diff/e2b263515feb/dom/canvas/moz.build#l1.12 interacting poorly.

Comment 68

[Jorg K (CEST = GMT+2)]

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.

Comment 69

[Jorg K (CEST = GMT+2)]

Clearing NI since this is being taken care of in bug 1428608. Sorry about the noise.