If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement modern context-loss detachment for objects

NEW
Assigned to

Status

()

Core
Canvas: WebGL
P3
normal
a year ago
10 months ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

({feature})

50 Branch
feature
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: gfx-noted)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments)

(Assignee)

Description

a year ago
The rules changed a couple times, but they should be stable after this recent clarification.

Let's take the opportunity to update and simplify some things.
(Assignee)

Comment 1

a year ago
Created attachment 8773663 [details]
Bug 1288643 - Reduce extension boilerplate. -

Review commit: https://reviewboard.mozilla.org/r/66424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66424/
Attachment #8773663 - Flags: review?(hshih)
Attachment #8773664 - Flags: review?(hshih)
Attachment #8773665 - Flags: review?(hshih)
Attachment #8773666 - Flags: review?(hshih)
Attachment #8773667 - Flags: review?(hshih)
Attachment #8773668 - Flags: review?(hshih)
Attachment #8773669 - Flags: review?(hshih)
Attachment #8773670 - Flags: review?(hshih)
(Assignee)

Comment 2

a year ago
Created attachment 8773664 [details]
Bug 1288643 - Don't put up with macro redefinition. -

Review commit: https://reviewboard.mozilla.org/r/66426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66426/
(Assignee)

Comment 3

a year ago
Created attachment 8773665 [details]
Bug 1288643 - mContext is mutable, so is now protected. -

Review commit: https://reviewboard.mozilla.org/r/66428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66428/
(Assignee)

Comment 4

a year ago
Created attachment 8773666 [details]
Bug 1288643 - Delete on Detach. -

Review commit: https://reviewboard.mozilla.org/r/66430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66430/
(Assignee)

Comment 5

a year ago
Created attachment 8773667 [details]
Bug 1288643 - WebGLRefCountedObject implies WebGLContextBoundObject. -

Review commit: https://reviewboard.mozilla.org/r/66432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66432/
(Assignee)

Comment 6

a year ago
Created attachment 8773668 [details]
Bug 1288643 - Since we detach now, just check if the object is detached to determine if lost. -

Review commit: https://reviewboard.mozilla.org/r/66434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66434/
(Assignee)

Comment 7

a year ago
Created attachment 8773669 [details]
Bug 1288643 - Bugs in WebGLExt*. -

Review commit: https://reviewboard.mozilla.org/r/66436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66436/
(Assignee)

Comment 8

a year ago
Created attachment 8773670 [details]
Bug 1288643 - Add basic ctors for non-macro'd extensions. -

Review commit: https://reviewboard.mozilla.org/r/66438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66438/
(Assignee)

Comment 9

a year ago
Created attachment 8773671 [details]
Bug 1288643 - Build fixes.

Review commit: https://reviewboard.mozilla.org/r/66440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66440/
Comment on attachment 8773666 [details]
Bug 1288643 - Delete on Detach. -

https://reviewboard.mozilla.org/r/66430/#review63552
Attachment #8773666 - Flags: review?(hshih) → review+
Comment hidden (obsolete)
Comment on attachment 8773667 [details]
Bug 1288643 - WebGLRefCountedObject implies WebGLContextBoundObject. -

https://reviewboard.mozilla.org/r/66432/#review63556
Attachment #8773667 - Flags: review?(hshih) → review+
Comment on attachment 8773668 [details]
Bug 1288643 - Since we detach now, just check if the object is detached to determine if lost. -

https://reviewboard.mozilla.org/r/66434/#review63558
Attachment #8773668 - Flags: review?(hshih) → review+
Comment on attachment 8773665 [details]
Bug 1288643 - mContext is mutable, so is now protected. -

https://reviewboard.mozilla.org/r/66428/#review63960

::: dom/canvas/WebGLProgram.cpp:124
(Diff revision 1)
>  //#define DUMP_SHADERVAR_MAPPINGS
>  
>  static already_AddRefed<const webgl::LinkedProgramInfo>
>  QueryProgramInfo(WebGLProgram* prog, gl::GLContext* gl)
>  {
> -    WebGLContext* const webgl = prog->mContext;
> +    const auto& webgl = prog->Context();

Is "const auto webgl" enough?

::: dom/canvas/WebGLTextureUpload.cpp:773
(Diff revision 1)
>                                     height == imageInfo->mHeight &&
>                                     depth == imageInfo->mDepth);
>          if (isFullUpload) {
>              *out_uploadWillInitialize = true;
>          } else {
> -            WebGLContext* webgl = tex->mContext;
> +            const auto& webgl = tex->Context();

Is "const auto webgl" enough?
Attachment #8773665 - Flags: review?(hshih) → review+
Comment on attachment 8773664 [details]
Bug 1288643 - Don't put up with macro redefinition. -

https://reviewboard.mozilla.org/r/66426/#review63962
Attachment #8773664 - Flags: review?(hshih) → review+
Comment on attachment 8773667 [details]
Bug 1288643 - WebGLRefCountedObject implies WebGLContextBoundObject. -

https://reviewboard.mozilla.org/r/66432/#review64358

::: dom/canvas/WebGLObjectModel.h:95
(Diff revision 1)
>   * parameter, as a means to allow DeleteOnce to call Delete() on the Derived
>   * class, without either method being virtual. This is a common C++ pattern
>   * known as the "curiously recursive template pattern (CRTP)".
>   */
>  template<typename Derived>
> -class WebGLRefCountedObject
> +class WebGLRefCountedObject : public WebGLContextBoundObject

The WebGLContextBoundObject definition should be placed before WebGLRefCountedObject.
There is a build break here.
Attachment #8773667 - Flags: review+ → review-
Comment on attachment 8773670 [details]
Bug 1288643 - Add basic ctors for non-macro'd extensions. -

https://reviewboard.mozilla.org/r/66438/#review64376

::: dom/canvas/WebGLExtensions.h:72
(Diff revision 1)
>  {
>  public:
> -    explicit WebGLExtensionDebugShaders(WebGLContext*);
> +    explicit WebGLExtensionDebugShaders(WebGLContext* webgl)
> +        : WebGLExtensionBase(webgl)
> +    {
> +        IsSupported(webgl);

moz_assert for all "IsSupport()" checking.

::: dom/canvas/WebGLExtensions.h:87
(Diff revision 1)
>  {
>  public:
> -    explicit WebGLExtensionLoseContext(WebGLContext*);
> +    explicit WebGLExtensionLoseContext(WebGLContext* webgl)
> +        : WebGLExtensionBase(webgl)
> +    {
> +        IsSupported(webgl);

moz_assert

::: dom/canvas/WebGLExtensions.h:136
(Diff revision 1)
>  {
>  public:
> -    explicit WebGLExtensionVertexArray(WebGLContext* webgl);
> +    explicit WebGLExtensionVertexArray(WebGLContext* webgl)
> +        : WebGLExtensionBase(webgl)
> +    {
> +        IsSupported(webgl);

moz_assert

::: dom/canvas/WebGLExtensions.h:154
(Diff revision 1)
>  {
>  public:
> -    explicit WebGLExtensionInstancedArrays(WebGLContext* webgl);
> +    explicit WebGLExtensionInstancedArrays(WebGLContext* webgl)
> +        : WebGLExtensionBase(webgl)
> +    {
> +        IsSupported(webgl);

moz_assert
Attachment #8773670 - Flags: review?(hshih) → review+
https://reviewboard.mozilla.org/r/66422/#review64402

::: dom/canvas/WebGLExtensionCompressedTextureES3.cpp:48
(Diff revision 1)
>  }
>  
> -WebGLExtensionCompressedTextureES3::~WebGLExtensionCompressedTextureES3()
> +/*static*/ bool
> +WebGLExtensionCompressedTextureES3::IsSupported(const WebGLContext* webgl)
>  {
> +    if (webgl->IsWebGL2())

check gfxPrefs::WebGLDraftExtensionsEnabled()

::: dom/canvas/WebGLExtensionDebugShaders.cpp:32
(Diff revision 1)
>  }
>  
> +/*static*/ bool
> +WebGLExtensionDebugShaders::IsSupported(const WebGLContext* webgl)
> +{
> +    return true;

There is no switch case inside "WebGLContext::IsExtensionSupported(WebGLExtensionID ext)".

If there is no gfxPrefs::WebGLPrivilegedExtensionsEnabled(), gecko return false for this.

::: dom/canvas/WebGLExtensionDisjointTimerQuery.cpp:246
(Diff revision 1)
> +}
> +
> +/*static*/ bool
>  WebGLExtensionDisjointTimerQuery::IsSupported(const WebGLContext* webgl)
>  {
> +  if (webgl->IsWebGL2())

check gfxPrefs::WebGLDraftExtensionsEnabled()?
Attachment #8773663 - Flags: review?(hshih) → review+
Comment on attachment 8773663 [details]
Bug 1288643 - Reduce extension boilerplate. -

https://reviewboard.mozilla.org/r/66424/#review64366

::: dom/canvas/WebGLExtensions.h:197
(Diff revision 1)
> +    {                                         \
> +    public:                                   \
> +        explicit T(WebGLContext* webgl);      \
> +            : WebGLExtensionBase(webgl)       \
> +        {                                     \
> +            IsSupported(webgl);               \

MOZ_ASSERT(IsSupported(webgl))?
If the ext is not supported, we should not create that one.
Comment on attachment 8773669 [details]
Bug 1288643 - Bugs in WebGLExt*. -

https://reviewboard.mozilla.org/r/66436/#review64428
Attachment #8773669 - Flags: review?(hshih) → review+
Comment on attachment 8773671 [details]
Bug 1288643 - Build fixes.

https://reviewboard.mozilla.org/r/66440/#review64430
Attachment #8773671 - Flags: review+
Keywords: feature
Priority: -- → P3
Whiteboard: gfx-noted
(Assignee)

Comment 22

10 months ago
Some parts of this have been implemented in bug 1320030.
See Also: → bug 1320030
(Assignee)

Updated

10 months ago
status-firefox50: affected → ---
You need to log in before you can comment on or make changes to this bug.