Closed
Bug 1045955
Opened 10 years ago
Closed 10 years ago
ANGLE share handle compositing makes preserveDrawingBuffer test fail
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: jgilbert, Assigned: jgilbert)
References
()
Details
Attachments
(5 files, 3 obsolete files)
22.06 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
2.15 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
8.81 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
3.44 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
5.31 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
I found out that this test on trunk: https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-attribute-preserve-drawing-buffer.html It doesn't appear to be failing for our test slaves. I'm not sure why not. That's pretty bad, in terms of having test coverage.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8464406 -
Flags: review?(bjacob)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8464407 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 3•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b60441ba3ccd
Attachment #8464407 -
Flags: review?(dglastonbury) → review+
Comment on attachment 8464406 [details] [diff] [review] patch 0: Fix style issues in touched code. Review of attachment 8464406 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLReadTexImageHelper.cpp @@ +263,5 @@ > > + bool needsRBSwap = false; > + if (aDest->GetFormat() == SurfaceFormat::B8G8R8A8 || > + aDest->GetFormat() == SurfaceFormat::B8G8R8X8) { > + needsRBSwap = true; bool needsRBSwap = (aDest->GetFormat() == SurfaceFormat::B8G8R8A8 || aDest->GetFormat() == SurfaceFormat::B8G8R8X8); @@ +269,5 @@ > > + bool needsConvertTo16Bits = false; > + if (aDest->GetFormat() == SurfaceFormat::R5G6B5) { > + needsConvertTo16Bits = true; > + } bool needsConvertTo16Bits = (aDest->GetFormat() == SurfaceFormat::R5G6B5); @@ +282,5 @@ > + uint8_t g = srcRow[1]; > + uint8_t b = needsRBSwap ? srcRow[0] : srcRow[2]; > + uint8_t a = srcRow[3]; > + > + if (needsConvertTo16Bits) { I don't know how often this function is called, but why branch inside the hot loop? if (needsConvertTo16Bits) { while (srcRow != srcRowEnd) { ... *(uint16_t*) destRow = PackRGB565(r, g, b); } } else { while (srcRow != srcRowEnd) { ... } }
Attachment #8464406 -
Flags: review?(bjacob) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Dan Glastonbury :djg :kamidphish from comment #4) > Comment on attachment 8464406 [details] [diff] [review] > patch 0: Fix style issues in touched code. > > Review of attachment 8464406 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/gl/GLReadTexImageHelper.cpp > @@ +263,5 @@ > > > > + bool needsRBSwap = false; > > + if (aDest->GetFormat() == SurfaceFormat::B8G8R8A8 || > > + aDest->GetFormat() == SurfaceFormat::B8G8R8X8) { > > + needsRBSwap = true; > > bool needsRBSwap = (aDest->GetFormat() == SurfaceFormat::B8G8R8A8 || > aDest->GetFormat() == SurfaceFormat::B8G8R8X8); Yep. > > @@ +269,5 @@ > > > > + bool needsConvertTo16Bits = false; > > + if (aDest->GetFormat() == SurfaceFormat::R5G6B5) { > > + needsConvertTo16Bits = true; > > + } > > bool needsConvertTo16Bits = (aDest->GetFormat() == SurfaceFormat::R5G6B5); Yep. > > @@ +282,5 @@ > > + uint8_t g = srcRow[1]; > > + uint8_t b = needsRBSwap ? srcRow[0] : srcRow[2]; > > + uint8_t a = srcRow[3]; > > + > > + if (needsConvertTo16Bits) { > > I don't know how often this function is called, but why branch inside the > hot loop? > > if (needsConvertTo16Bits) { > while (srcRow != srcRowEnd) { > ... > *(uint16_t*) destRow = PackRGB565(r, g, b); > } > } else { > while (srcRow != srcRowEnd) { > ... > > } > } Because it was there already. I got half-way though making a better-optimized version of this, but then decided that we should just use a common fast pixel format converter somewhere in the tree. This is out of scope for the current bug, so I just kept it as it was.
Assignee | ||
Comment 6•10 years ago
|
||
r=kamidphish
Attachment #8464406 -
Attachment is obsolete: true
Attachment #8464812 -
Flags: review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/834a458dae0b https://hg.mozilla.org/integration/mozilla-inbound/rev/aef0101ff775
Comment 8•10 years ago
|
||
Backed out for B2G reftest failures. I left the style fixup in, so adding leave-open to the keywords so the bug isn't resolved when that merges to m-c. You'll want to remove it when re-pushing to inbound. https://hg.mozilla.org/integration/mozilla-inbound/rev/ba2784ebc197 https://tbpl.mozilla.org/php/getParsedLog.php?id=44910023&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=44909193&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=44910456&tree=Mozilla-Inbound
Keywords: leave-open
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/834a458dae0b
Comment 10•10 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #5) > Because it was there already. I got half-way though making a > better-optimized version of this, but then decided that we should just use a > common fast pixel format converter somewhere in the tree. This is out of > scope for the current bug, so I just kept it as it was. Oh sure, I didn't expect you to fix that one. It was more for prosperity. I wish we had a system not linked to bugzilla where we could leave code reviews. Kinda like this: http://www.bounceapp.com/116414
Updated•10 years ago
|
Keywords: leave-open
Assignee | ||
Comment 11•10 years ago
|
||
Alright, I found the issue.
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8468976 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8468977 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8464407 -
Attachment is obsolete: true
Attachment #8468979 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8468980 -
Flags: review?(dglastonbury)
Attachment #8468976 -
Flags: review?(dglastonbury) → review+
Attachment #8468977 -
Flags: review?(dglastonbury) → review+
Comment 16•10 years ago
|
||
Comment on attachment 8468979 [details] [diff] [review] patch 3.5: Fix issues caused by patch 3 on B2G Emu Review of attachment 8468979 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLBlitHelper.cpp @@ +556,5 @@ > > ScopedBindFramebuffer boundFB(mGL); > ScopedGLState scissor(mGL, LOCAL_GL_SCISSOR_TEST, false); > > + if (internalFBs && mGL->Screen()) { If you ask for internalFBs and there is no Screen(), should it fallback or do nothing and report error?
Attachment #8468979 -
Flags: review?(dglastonbury) → review-
Attachment #8468980 -
Flags: review?(dglastonbury) → review+
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Dan Glastonbury :djg :kamidphish from comment #16) > Comment on attachment 8468979 [details] [diff] [review] > patch 3.5: Fix issues caused by patch 3 on B2G Emu > > Review of attachment 8468979 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/gl/GLBlitHelper.cpp > @@ +556,5 @@ > > > > ScopedBindFramebuffer boundFB(mGL); > > ScopedGLState scissor(mGL, LOCAL_GL_SCISSOR_TEST, false); > > > > + if (internalFBs && mGL->Screen()) { > > If you ask for internalFBs and there is no Screen(), should it fallback or > do nothing and report error? It should only ask for internal if mGL->Screen() is non-null. I can add an explicit assert for this.
Assignee | ||
Comment 18•10 years ago
|
||
Oops, this was the wrong patch.
Attachment #8468979 -
Attachment is obsolete: true
Attachment #8469558 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 19•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4fedbc53a342
Attachment #8469558 -
Flags: review?(dglastonbury) → review+
Assignee | ||
Comment 20•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2260b0f9e424 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/27af4489326f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7593d2b0bd remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b065f0ebf575
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2260b0f9e424 https://hg.mozilla.org/mozilla-central/rev/27af4489326f https://hg.mozilla.org/mozilla-central/rev/ea7593d2b0bd https://hg.mozilla.org/mozilla-central/rev/b065f0ebf575
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
QA Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•