Closed Bug 1288643 Opened 8 years ago Closed 5 years ago

Implement modern context-loss detachment for objects

Categories

(Core :: Graphics: CanvasWebGL, defect, P3)

50 Branch
defect

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Keywords: feature, Whiteboard: gfx-noted)

Attachments

(9 files)

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.
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)
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.
Keywords: feature
Priority: -- → P3
Whiteboard: gfx-noted
Some parts of this have been implemented in bug 1320030.
See Also: → 1320030
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: