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.

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) { ... } }

(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.

r=kamidphish

https://hg.mozilla.org/integration/mozilla-inbound/rev/834a458dae0b https://hg.mozilla.org/integration/mozilla-inbound/rev/aef0101ff775

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

(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

Alright, I found the issue.

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?

(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.

Oops, this was the wrong patch.

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

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