1094458 - Implement RenderbufferStorageMultisample

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Attachment: 0005-Implement-RenderbufferStorageMultisample.patch

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8517729 [details] [diff] [review]
0005-Implement-RenderbufferStorageMultisample.patch

Review of attachment 8517729 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContext.h
@@ +212,5 @@
> +    void ErrorInvalidFramebufferOperation(const char* fmt = 0, ...);
> +    void ErrorInvalidEnumInfo(const char* info, GLenum enumValue);
> +    void ErrorInvalidEnumInfo(const char* info, const char* funcName,
> +                              GLenum enumValue);
> +    void ErrorOutOfMemory(const char* fmt = 0, ...);

While we're here, can these 0s be nullptrs?

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Update-formatting.patch

Let's just reuse this bug.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Update-to-allow-RenderbufferStorageMultisample.patch

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: 0003-Don-t-actual-bind-RBs-until-needed.patch

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: 0004-Handle-max-samples-per-format-and-normalize-meaning-.patch

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: 0005-Don-t-check-size-of-FBAttachments-that-have-no-image.patch

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: 0006-Remove-mGLMaxSamples.patch

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8709647 [details] [diff] [review]
0004-Handle-max-samples-per-format-and-normalize-meaning-.patch

Review of attachment 8709647 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLFramebuffer.cpp
@@ +822,5 @@
> +            return true;
> +        }
> +
> +        return (curWidth == width &&
> +                curHeight == height);

This would be nicer if it used a type like IntSize.

@@ +874,3 @@
>      }
>  
> +    return matches;

It seems like the following would be simpler:

uint32_t samples = mColorAttachment0.Samples();

bool matches = true;
matches &= (mDepthAttachement.Samples() == samples);
matches &= (mStencilAttachement.Samples() == samples);
matches &= (mDepthStencilAttachement.Samples() == samples);

for (const auto& cur : mMoreColorAttachments) {
    matches &= (cur.Samples() == samples;
}

This avoids the awkward needsInit business. (I'd prefer just initializing to the first one in AllImageRectsMatch too)

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> @@ +874,3 @@
> >      }
> >  
> > +    return matches;
> 
> It seems like the following would be simpler:

I see that you modify this function in a later patch which makes my simplification no longer practical. My comments about needsInit still apply though.

Comment 10

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Comment on attachment 8709647 [details] [diff] [review]
> 0004-Handle-max-samples-per-format-and-normalize-meaning-.patch
> 
> Review of attachment 8709647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLFramebuffer.cpp
> @@ +822,5 @@
> > +            return true;
> > +        }
> > +
> > +        return (curWidth == width &&
> > +                curHeight == height);
> 
> This would be nicer if it used a type like IntSize.
It would be real nice to go back through and do this for these classes.
> 
> @@ +874,3 @@
> >      }
> >  
> > +    return matches;
> 
> It seems like the following would be simpler:
> 
> uint32_t samples = mColorAttachment0.Samples();
> 
> bool matches = true;
> matches &= (mDepthAttachement.Samples() == samples);
> matches &= (mStencilAttachement.Samples() == samples);
> matches &= (mDepthStencilAttachement.Samples() == samples);
> 
> for (const auto& cur : mMoreColorAttachments) {
>     matches &= (cur.Samples() == samples;
> }
> 
> This avoids the awkward needsInit business. (I'd prefer just initializing to
> the first one in AllImageRectsMatch too)

We can't do that. 
* Attachments may not have an image. (including mColorAttachment0)
* Samples() may return anything {0,1,...}, so none of those is a safe sentinel.

Comment 11

[Kelsey Gilbert [:jgilbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46df4b93c039

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: 0007-Our-compilers-aren-t-ready-for-auto-args-in-lambdas.patch

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: 0008-build.patch

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: 0009-Fix-tests.patch

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c496c7c24113

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c496c7c24113