Closed Bug 1094458 Opened 10 years ago Closed 8 years ago

Implement RenderbufferStorageMultisample

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(9 files, 1 obsolete file)

5.00 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
10.19 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
12.14 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
17.77 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.95 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.31 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.31 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.32 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.76 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
      No description provided.
Attachment #8517729 - Flags: review?(dvander)
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?
Attachment #8517729 - Flags: review?(dvander) → review+
Let's just reuse this bug.
Attachment #8517729 - Attachment is obsolete: true
Attachment #8709644 - Flags: review?(jmuizelaar)
Attachment #8709651 - Flags: review?(jmuizelaar)
See Also: → 1240438
Attachment #8709644 - Flags: review?(jmuizelaar) → review+
Attachment #8709646 - Flags: review?(jmuizelaar) → review+
Attachment #8709645 - Flags: review?(jmuizelaar) → review+
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)
Attachment #8709647 - Flags: review?(jmuizelaar) → review+
(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.
Attachment #8709648 - Flags: review?(jmuizelaar) → review+
Attachment #8709651 - Flags: review?(jmuizelaar) → review+
(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.
Attached patch 0008-build.patchSplinter Review
Attachment #8710692 - Flags: review?(jmuizelaar)
Attachment #8710694 - Flags: review?(jmuizelaar)
Attachment #8710691 - Flags: review?(jmuizelaar) → review+
Attachment #8710692 - Flags: review?(jmuizelaar) → review+
Attachment #8710694 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/c496c7c24113
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: