Closed
Bug 1094458
Opened 10 years ago
Closed 8 years ago
Implement RenderbufferStorageMultisample
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
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+
Assignee | ||
Comment 2•8 years ago
|
||
Let's just reuse this bug.
Attachment #8517729 -
Attachment is obsolete: true
Attachment #8709644 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8709645 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8709646 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8709647 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8709648 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8709651 -
Flags: review?(jmuizelaar)
Updated•8 years ago
|
Attachment #8709644 -
Flags: review?(jmuizelaar) → review+
Updated•8 years ago
|
Attachment #8709646 -
Flags: review?(jmuizelaar) → review+
Updated•8 years ago
|
Attachment #8709645 -
Flags: review?(jmuizelaar) → review+
Comment 8•8 years ago
|
||
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+
Comment 9•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8709648 -
Flags: review?(jmuizelaar) → review+
Updated•8 years ago
|
Attachment #8709651 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 10•8 years ago
|
||
(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.
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46df4b93c039
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8710691 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8710692 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8710694 -
Flags: review?(jmuizelaar)
Updated•8 years ago
|
Attachment #8710691 -
Flags: review?(jmuizelaar) → review+
Updated•8 years ago
|
Attachment #8710692 -
Flags: review?(jmuizelaar) → review+
Updated•8 years ago
|
Attachment #8710694 -
Flags: review?(jmuizelaar) → review+
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c496c7c24113
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•