1012407 - Draw rotated textures with CLAMP and uniform arrays instead of uploading a geometry or GL_REPEAT

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Andreas Gal :gal]

The basic idea here is to upload a geometry with 4 uniform quads (0,0)(1,1) and an additional index attribute stored in the z coordinate (0,1,2,3). We can index into a uniform array for the layer quad rect and texture transform using int(z). With this we can avoid GL_REPEAT and artifacts on outside texture boundaries and we can avoid uploading geometries.

Comment 2

[Andreas Gal :gal]

Attachment: Part 1: Reduce amount of data uploaded to GPU (layer quad rect instead of transform).

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 8424477 [details] [diff] [review]
Part 1: Reduce amount of data uploaded to GPU (layer quad rect instead of transform).

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

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +158,5 @@
>    }
>  
>    vs << "void main() {" << endl;
>    vs << "  vec4 finalPosition = aVertexCoord;" << endl;
> +  vs << "  mat4 layerQuadTransform = mat4(uLayerQuadRect.z, 0.0, 0.0, 0.0," << endl;

Unless the approach I'm suggesting somehow compiles to slower code (it doesn't on D3D) let's just do what we do here: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/CompositorD3D11.fx#83

Should require a little less math.

Comment 4

[Andreas Gal :gal]

Attachment: Part 1: Reduce amount of data uploaded to GPU (layer quad rect instead of transform). r=Bas

Comment 5

[Andreas Gal :gal]

Bas, if you agree with the shader change please checkin-needed it.

Comment 6

[Andreas Gal :gal]

Attachment: Part 2: Use Moz2D to draw graph instead of LOCAL_GL_LINE_STRIP

Comment 7

[Andreas Gal :gal]

Attachment: Part 3: Eliminate DrawQuadInternal since we no longer need it

Comment 8

[Andreas Gal :gal]

Attachment: Part 4: Always pass the layer rect to BindAndDrawQuad (we will need that there in a moment)

Comment 9

[Andreas Gal :gal]

Attachment: Part 5: Allow passing a texture rectangle to BindAndDrawQuad (and shaders)

Comment 10

[Andreas Gal :gal]

Attachment: Part 6: Eliminate VBO use and DrawQuads and manually draw no-repeat quads

Comment 11

[Andreas Gal :gal]

Attachment: Part 7: Remove VBOArena

Comment 12

[Andreas Gal :gal]

Comment on attachment 8426208 [details] [diff] [review]
Part 2: Use Moz2D to draw graph instead of LOCAL_GL_LINE_STRIP

As a side effect this should work on windows, too now.

Comment 13

[Andreas Gal :gal]

Comment on attachment 8426210 [details] [diff] [review]
Part 4: Always pass the layer rect to BindAndDrawQuad (we will need that there in a moment)

This is needed later because we subdivide the layer rect as we draw it in multiple sections from within BindAndDrawQuad.

Comment 14

[Andreas Gal :gal]

Comment on attachment 8426211 [details] [diff] [review]
Part 5: Allow passing a texture rectangle to BindAndDrawQuad (and shaders)

It turned out a lot easier to do the aTextureTransform multiplication in the shader. The only place we really need a full aTextureTransform where Android has some 3D Matrix transform it gives us along with the surface otherwise I would have eliminated it. This is basically free in the VS so who cares.

Comment 15

[Andreas Gal :gal]

Attachment: Part 6: Eliminate VBO use and DrawQuads and manually draw no-repeat quads

Comment 16

[Andreas Gal :gal]

Attachment: Part 7: Eliminate VBO use and DrawQuads and manually draw no-repeat quads

Comment 17

[Andreas Gal :gal]

Attachment: Part 8: Remove VBOArena

Removing VBOArena since its unused. For complex geometries (CSS mesh transforms) I want to cache the geometry instead of the VBO arena stuff.

Comment 18

[Andreas Gal :gal]

Attachment: Part 9: Use CLAMP instead of GL_REPEAT

GL_REPEAT is no longer faster since we can draw rotated textures with a single draw call as well now. This should fix bugs where the one side of a texture bleeds through to the other.

Comment 19

[Bas Schouten (:bas.schouten)]

Comment on attachment 8426211 [details] [diff] [review]
Part 5: Allow passing a texture rectangle to BindAndDrawQuad (and shaders)

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

Could we rename TextureRect to something a little bit more descriptive maybe? TextureSubRect or TextureCoordRect or something along those lines?

Comment 20

[Bas Schouten (:bas.schouten)]

Comment on attachment 8426217 [details] [diff] [review]
Part 7: Eliminate VBO use and DrawQuads and manually draw no-repeat quads

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

::: gfx/layers/opengl/OGLShaderProgram.cpp
@@ +154,5 @@
>  
>    if (!(aConfig.mFeatures & ENABLE_RENDER_COLOR)) {
>      vs << "uniform mat4 uTextureTransform;" << endl;
> +    vs << "uniform vec4 uTextureRects[4];" << endl;
> +    vs << "attribute vec4 aTexCoord;" << endl;

I don't think this needs to become a vec4, does it?

Comment 21

[Bas Schouten (:bas.schouten)]

Comment on attachment 8426218 [details] [diff] [review]
Part 8: Remove VBOArena

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

So much r+...

Comment 22

[Andreas Gal :gal]

Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/4139e5b0255a

Comment 23

[Andreas Gal :gal]

Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/52942d958c87

Comment 24

[Andreas Gal :gal]

Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb8ef51c6b5

Comment 25

[Andreas Gal :gal]

Part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/03e1605d8bfd

Comment 26

[Andreas Gal :gal]

Bas agreed that the vec4 change is probably best for bad drivers that are confused by the attribute pointer setup that uses a stride of 4 now (was 2).

I am leaving the name aTextureRect for parity with aLayerRect but I agree that we might want to clarify it I just don't think aTextureCoordRect is any less confusing. Very open to bike shedding though. Maybe uTextureRects is already enough? (coming in a later patch)

Comment 27

[Andreas Gal :gal]

Attachment: Part 5 for checkin (please land in one push with part 6)

This patch likely regresses performance stand-alone (on Mac TALOS) so please land together with part 6.

Comment 28

[Andreas Gal :gal]

Attachment: Part 6 for checkin (please land in one push with part 5)

Comment 29

[Andreas Gal :gal]

Attachment: The real part 7 (FML, too many patches in this queue): use one GL draw call to draw up to 4 quads of a rotated texture

Comment 30

[Andreas Gal :gal]

Part 5: https://hg.mozilla.org/integration/mozilla-inbound/rev/560bd45d52de
Part 6: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c692df3f080

Landed together to avoid a regression from part 5. This is inconsistent with the numbering above.

Comment 31

[Andreas Gal :gal]

Part 7: https://hg.mozilla.org/integration/mozilla-inbound/rev/b17b7b59cf88

Comment 32

[Andreas Gal :gal]

Part 8: https://hg.mozilla.org/integration/mozilla-inbound/rev/08e17f48aed1

Comment 33

[Andreas Gal :gal]

The last part switches on CLAMP. I want to wait until the rest has cycled and talos results are back before landing that.

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out all four parts in https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4e5e80de4a because it appears to have turned reftest-5 orange on b2g ICS Emulator: https://tbpl.mozilla.org/php/getParsedLog.php?id=40142321&tree=Mozilla-Inbound

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4139e5b0255a
https://hg.mozilla.org/mozilla-central/rev/52942d958c87
https://hg.mozilla.org/mozilla-central/rev/ebb8ef51c6b5
https://hg.mozilla.org/mozilla-central/rev/03e1605d8bfd

Comment 36

[Andreas Gal :gal]

Thanks Wes!

Comment 37

[Andreas Gal :gal]

Attachment: Test

Comment 38

[Andreas Gal :gal]

I can reproduce a problem with buffer rotation when I force CanUploadNonPowerOfTwo to false. Maybe our b2g emulator does that? Will see if I can chase it down.

Comment 39

[Andreas Gal :gal]

Actually on mac even without my patch if CanUploadNonPowerOfTwo is false everything falls apart so thats not it.

Comment 40

[Andreas Gal :gal]

Part 5 only: https://tbpl.mozilla.org/?tree=Try&rev=92f31853c16f

Comment 41

[Andreas Gal :gal]

Part 5 + 6: https://tbpl.mozilla.org/?tree=Try&rev=c8d3fe12d57e

Comment 42

[Andreas Gal :gal]

Wes, I think I am getting green with part 5 and 6 with those tests. Can you confirm?

Comment 43

[Andreas Gal :gal]

Re-landing part 5 and 6 since I think they went green above:

https://hg.mozilla.org/integration/mozilla-inbound/rev/013e0b64e6ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/78bc7d04bac9

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Yeah, that try run looks fine.

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/013e0b64e6ca
https://hg.mozilla.org/mozilla-central/rev/78bc7d04bac9

Comment 46

[Andreas Gal :gal]

Part 7: https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a259f05a18

Comment 47

[Andreas Gal :gal]

I think part 7 failed again, so it was this part that caused the regression: https://tbpl.mozilla.org/php/getParsedLog.php?id=40252915&tree=Mozilla-Inbound

Comment 48

[Carsten Book [:Tomcat]]

(In reply to Andreas Gal :gal from comment #47)
> I think part 7 failed again, so it was this part that caused the regression:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=40252915&tree=Mozilla-
> Inbound

yeah looks like. Sorry had to back this out for the test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=40253526&tree=Mozilla-Inbound

Comment 49

[Andreas Gal :gal]

Part 1-3 are safe. Part 4- cause bug 1015176. If someone wants to back out, please absolutely go ahead. I am investigating in parallel. My Wifi here is attrocious. I will back out Saturday when I land in the US if nobody gets to it until then.

Comment 50

[Andreas Gal :gal]

When this happens we literally render the layer twice (visually as well).

Correct:

0 layer(763.000000 1284.000000 1362.000000 1522.000000) rect(0.210572 0.043970 0.585303 0.956030) 1 layer(763.000000 2806.000000 1362.000000 70.000000) rect(0.210572 0.000000 0.\
585303 0.043970) xwrap: 0 ywrap: 1
0 layer(2150.000000 1284.000000 450.000000 1522.000000) rect(0.806618 0.043970 0.193382 0.956030) 1 layer(2150.000000 2806.000000 450.000000 70.000000) rect(0.806618 0.000000 0.\
193382 0.043970) xwrap: 0 ywrap: 0

Bad:

0 layer(273.000000 1052.000000 2327.000000 54.000000) rect(0.000000 0.966080 1.000000 0.033920) 1 layer(273.000000 1106.000000 2327.000000 122.000000) rect(0.000000 0.000000 1.0\
00000 0.076633) xwrap: 0 ywrap: 1
0 layer(273.000000 1052.000000 2327.000000 54.000000) rect(0.000000 0.966080 1.000000 0.033920) 1 layer(273.000000 1106.000000 2327.000000 122.000000) rect(0.000000 0.000000 1.0\
00000 0.076633) xwrap: 0 ywrap: 1
0 layer(763.000000 1228.000000 1837.000000 691.994629) rect(0.210572 0.076633 0.789428 0.923367) 1 layer(763.000000 1919.994629 1837.000000 724.005371) rect(0.210572 0.000000 0.\
789428 0.966080) xwrap: 0 ywrap: 1
0 layer(763.000000 1228.000000 1837.000000 691.994629) rect(0.210572 0.076633 0.789428 0.923367) 1 layer(763.000000 1919.994629 1837.000000 724.005371) rect(0.210572 0.000000 0.\
789428 0.966080) xwrap: 0 ywrap: 0

Comment 51

[Andreas Gal :gal]

Alright, so the bug is that the texcoords overflow > 1.0. We had code for this to normalize it and I left it out, thats biting us. I will dig a bit into this to see why we overflow here.

Correct:

Rect(0.000000 0.000000 2880.000000 156.000000) TexCoordRect(0.000000 0.000000 1.000000 1.000000)
Rect(0.000000 156.000000 2880.000000 1592.000000) TexCoordRect(0.000000 0.000000 1.000000 1.000000)
Rect(763.000000 1304.000000 1362.000000 1592.000000) TexCoordRect(0.210572 0.056533 0.585303 1.000000)
Rect(2150.000000 1304.000000 450.000000 1592.000000) TexCoordRect(0.806618 0.056533 0.193382 1.000000)
Rect(0.000000 1715.000000 484.000000 33.000000) TexCoordRect(0.000000 0.000000 1.000000 1.000000)
Rect(0.000000 0.000000 32.000000 32.000000) TexCoordRect(0.000000 0.000000 1.000000 1.000000)

Bad:

Rect(0.000000 0.000000 2880.000000 156.000000) TexCoordRect(0.000000 0.000000 1.000000 1.000000)
Rect(0.000000 156.000000 2880.000000 1592.000000) TexCoordRect(0.000000 0.000000 1.000000 1.000000)
Rect(273.000000 1172.000000 2327.000000 56.000000) TexCoordRect(0.000000 0.976131 1.000000 0.035176)
Rect(273.000000 1172.000000 2327.000000 56.000000) TexCoordRect(0.000000 0.976131 1.000000 0.035176)
Rect(763.000000 1228.000000 1837.000000 1536.000000) TexCoordRect(0.210572 1.011307 0.789428 0.964824)
Rect(763.000000 1228.000000 1837.000000 1536.000000) TexCoordRect(0.210572 1.011307 0.789428 0.964824)
Rect(0.000000 0.000000 32.000000 32.000000) TexCoordRect(0.000000 0.000000 1.000000 1.000000)

Comment 52

[Andreas Gal :gal]

Attachment: Fix for part 6: Fix for bug 1015176

Comment 53

[Andreas Gal :gal]

BenWa will r+ the fix and I will upload the whole part again for review from Bas for context.

Comment 54

[Benoit Girard (:BenWa)]

Comment on attachment 8428171 [details] [diff] [review]
Fix for part 6: Fix for bug 1015176

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

Looks good to me but it took me some time to understand what the code did so it would be nice to add a bit of comments while we're touching the code. Particularly make it clear why we're not clamping to 1f.

Comment 56

[Andreas Gal :gal]

Part 9: https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c58a2a6da4

Comment 57

[Andreas Gal :gal]

Attachment: Part 6: Draw rects manually instead of GL_REPEAT. Has now the missing tex coord normalization in DecomposeIntoNoRepeatRects. The latter is duplicated because DecomposeIntoNoRepeatTriangles is designed to be consumed by VBO uploads and I am about to kill t

Comment 58

[Bas Schouten (:bas.schouten)]

Comment on attachment 8428180 [details] [diff] [review]
Part 6: Draw rects manually instead of GL_REPEAT. Has now the missing tex coord normalization in DecomposeIntoNoRepeatRects. The latter is duplicated because DecomposeIntoNoRepeatTriangles is designed to be consumed by VBO uploads and I am about to kill t

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

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +388,5 @@
> +  // given assume GL_REPEAT, which we explicitly won't use and instead
> +  // manually split into sub-rects.
> +  while (texCoordRect.x >= 1.0f)
> +    texCoordRect.x -= 1.0f;
> +  while (texCoordRect.y >= 1.0f)

nit: Technically we're not normalizing here, just moving it about.

@@ +397,4 @@
>    bool flipped = false;
>    if (texCoordRect.height < 0) {
>      flipped = true;
>      texCoordRect.y = texCoordRect.YMost();

Now that we're passing through this code, let's make a note to fix this to say y + height. Since YMost() -should- be returning y in a negative-rect case if it were working properly.

Comment 59

[Andreas Gal :gal]

Attachment: Part 6 nits

Comment 60

[Andreas Gal :gal]

Landed the nits: https://hg.mozilla.org/integration/mozilla-inbound/rev/634be002dbc9

And part 7 (the fix above should fix the orange): https://hg.mozilla.org/integration/mozilla-inbound/rev/471244c106e9

Comment 61

[Nathan Froyd [:froydnj]]

Backed out for oranges:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f722f6bad43e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2f44877305f

Comment 62

[Andreas Gal :gal]

Ok so the fix was backed out. In that case we should back out part 6 as well. On a plane for 12 hours starting now. That having said the fix looks right. Are you sure that caused oranges?

Comment 63

[Andreas Gal :gal]

21:32:15     INFO -  2029 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/crossorigin/test_webgl_crossorigin_textures.html | Assertion count 3 is greater than expected range 0-0 assertions.
21:32:15     INFO -  2030 INFO TEST-START | /tests/content/canvas/test/test_2d.clearRect.image.offscreen.html
21:32:15     INFO -  [Parent 1265] ###!!! ASSERTION: ylen isn't > 0, what's going on?: '!ywrap || ylen > 0.0f', file /builds/slave/m-in-osx64-d-00000000000000000/build/gfx/layers/opengl/CompositorOGL.cpp, line 453
21:32:15     INFO -  mozilla::layers::CompositorOGL::DrawQuad(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::EffectChain const&, float, mozilla::gfx::Matrix4x4 const&) [gfx/layers/opengl/CompositorOGL.cpp:1103]
21:32:15     INFO -  mozilla::layers::ImageHost::Composite(mozilla::layers::EffectChain&, float, mozilla::gfx::Matrix4x4 const&, mozilla::gfx::Filter const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, nsIntRegion const*, mozilla::layers::TiledLayerProperties*) [gfx/layers/composite/ImageHost.cpp:162]
21:32:15     INFO -  mozilla::layers::CanvasLayerComposite::RenderLayer(nsIntRect const&) [obj-firefox/dist/include/mozilla/RefPtr.h:260]
21:32:15     INFO -  void mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, nsIntRect const&) [gfx/layers/composite/ContainerLayerComposite.cpp:369]
21:32:15     INFO -  void mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, nsIntRect const&) [gfx/layers/composite/ContainerLayerComposite.cpp:369]
21:32:15     INFO -  mozilla::layers::LayerManagerComposite::Render() [gfx/layers/composite/LayerManagerComposite.cpp:442]
21:32:15     INFO -  mozilla::layers::LayerManagerComposite::EndTransaction(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) [gfx/layers/composite/LayerManagerComposite.cpp:247]
21:32:15     INFO -  mozilla::layers::LayerManagerComposite::EndEmptyTransaction(mozilla::layers::LayerManager::EndTransactionFlags) [gfx/layers/composite/LayerManagerComposite.cpp:197]
21:32:15     INFO -  mozilla::layers::CompositorParent::CompositeToTarget(mozilla::gfx::DrawTarget*) [gfx/layers/ipc/CompositorParent.cpp:674]
21:32:15     INFO -  MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) [ipc/chromium/src/base/message_loop.cc:357]
21:32:15     INFO -  MessageLoop::DoDelayedWork(base::TimeTicks*) [ipc/chromium/src/base/message_loop.cc:471]
21:32:15     INFO -  base::MessagePumpDefault::Run(base::MessagePump::Delegate*) [ipc/chromium/src/base/message_pump_default.cc:40]
21:32:15     INFO -  MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:504]
21:32:15     INFO -  base::Thread::ThreadMain() [ipc/chromium/src/base/thread.cc:171]
21:32:15     INFO -  ThreadFunc [ipc/chromium/src/base/platform_thread_posix.cc:39]
21:32:15     INFO -  libsystem_c.dylib + 0x14782
21:32:15     INFO -  [Parent 1265] ###!!! ASSERTION: ylen isn't > 0, what's going on?: '!ywrap || ylen > 0.0f', file /builds/slave/m-in-osx64-d-00000000000000000/build/gfx/layers/opengl/CompositorOGL.cpp, line 453
21:32:15     INFO -  mozilla::layers::CompositorOGL::DrawQuad(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::EffectChain const&, float, mozilla::gfx::Matrix4x4 const&) [gfx/layers/opengl/CompositorOGL.cpp:1103]
21:32:15     INFO -  mozilla::layers::ImageHost::Composite(mozilla::layers::EffectChain&, float, mozilla::gfx::Matrix4x4 const&, mozilla::gfx::Filter const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, nsIntRegion const*, mozilla::layers::TiledLayerProperties*) [gfx/layers/composite/ImageHost.cpp:162]
21:32:15     INFO -  mozilla::layers::CanvasLayerComposite::RenderLayer(nsIntRect const&) [obj-firefox/dist/include/mozilla/RefPtr.h:260]
21:32:15     INFO -  void mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, nsIntRect const&) [gfx/layers/composite/ContainerLayerComposite.cpp:369]
21:32:15     INFO -  void mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, nsIntRect const&) [gfx/layers/composite/ContainerLayerComposite.cpp:369]
21:32:15     INFO -  mozilla::layers::LayerManagerComposite::Render() [gfx/layers/composite/LayerManagerComposite.cpp:442]
21:32:15     INFO -  mozilla::layers::LayerManagerComposite::EndTransaction(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) [gfx/layers/composite/LayerManagerComposite.cpp:247]
21:32:15     INFO -  mozilla::layers::LayerManagerComposite::EndEmptyTransaction(mozilla::layers::LayerManager::EndTransactionFlags) [gfx/layers/composite/LayerManagerComposite.cpp:197]
21:32:15     INFO -  mozilla::layers::CompositorParent::CompositeToTarget(mozilla::gfx::DrawTarget*) [gfx/layers/ipc/CompositorParent.cpp:674]
21:32:15     INFO -  MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) [ipc/chromium/src/base/message_loop.cc:357]
21:32:15     INFO -  MessageLoop::DoWork() [ipc/chromium/src/base/message_loop.cc:443]
21:32:15     INFO -  base::MessagePumpDefault::Run(base::MessagePump::Delegate*) [ipc/chromium/src/base/message_pump_default.cc:34]
21:32:15     INFO -  MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:504]
21:32:15     INFO -  base::Thread::ThreadMain() [ipc/chromium/src/base/thread.cc:171]
21:32:15     INFO -  ThreadFunc [ipc/chromium/src/base/platform_thread_posix.cc:39]
21:32:15     INFO -  libsystem_c.dylib + 0x14782

Comment 64

[Andreas Gal :gal]

This failed tests on mac with an assert (amongst others). I cleaned up the tex coord wrapping code. Try:

https://tbpl.mozilla.org/?tree=Try&rev=d71c3864619d

Comment 65

[Andreas Gal :gal]

Attachment: Fix tex coord wrapping.

Comment 66

[Andreas Gal :gal]

Try with debug that actually caught the exceptions:

https://tbpl.mozilla.org/?tree=Try&rev=8aab19771902

Comment 67

[Andreas Gal :gal]

The try chooser seems to hate me. Hopefully this works: https://tbpl.mozilla.org/?tree=Try&rev=902a053490d1

Comment 68

[Andreas Gal :gal]

./mach mochitest-plain dom/events/test/test_bug946632.html

produces:

tl.x tl.y br.x br.y

 0:05.77 0.925000 0.000000 1.925000 1.000000
 0:05.77 0.925000 0.000000 1.925000 1.000000

br.x is >= 1.0 so the exception fires

Comment 69

[Andreas Gal :gal]

Sanitize tex coords even harder (and saner):

https://tbpl.mozilla.org/?tree=Try&rev=ef5bb323140a

Comment 70

[Andreas Gal :gal]

Attachment: Fix tex coord wrapping that was broken by part 6

Fixed the tex coord wrapping code for real now (passed try). This should also be a lot more readable now.

Comment 71

[Andreas Gal :gal]

Attachment: Fix tex coord wrapping that was broken by part 6

Comment 74

[Markus Stange [:mstange]]

Comment on attachment 8428727 [details] [diff] [review]
Fix tex coord wrapping that was broken by part 6

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

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +379,5 @@
> +static inline bool
> +FuzzyEqual(float a, float b)
> +{
> +  return fabs(a - b) < 0.0001f;
> +}

This function is only used in the NS_ASSERTIONs below. Doesn't this give you unused function warnings in opt builds? If this was green on try, what was the try configuration?

It looks like Bas is around now, so I'll let him do the rest of the review.

Comment 75

[Andreas Gal :gal]

static inline should not produce a warning if unused.

Comment 76

[Andreas Gal :gal]

opt build in gfx/layers after touch CompositorOGL.cpp:

furball:layers gal$ make
/Applications/Xcode.app/Contents/Developer/usr/bin/make export
make[1]: Nothing to be done for `export'.
/Applications/Xcode.app/Contents/Developer/usr/bin/make compile
Unified_cpp_gfx_layers4.o
/usr/bin/clang++ -o Unified_cpp_gfx_layers4.o -c  -fvisibility=hidden -DOS_POSIX=1 -DOS_MACOSX=1 -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL  -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -I/Users/gal/workspace/mozilla-central/gfx/layers -I. -I/Users/gal/workspace/mozilla-central/ipc/chromium/src -I/Users/gal/workspace/mozilla-central/ipc/glue -I/Users/gal/workspace/mozilla-central/obj-x86_64-apple-darwin13.1.0/ipc/ipdl/_ipdlheaders -I../../dist/include  -I/Users/gal/workspace/mozilla-central/obj-x86_64-apple-darwin13.1.0/dist/include/nspr -I/Users/gal/workspace/mozilla-central/obj-x86_64-apple-darwin13.1.0/dist/include/nss  -I/Users/gal/workspace/mozilla-central/obj-x86_64-apple-darwin13.1.0/dist/include -I/Users/gal/workspace/mozilla-central/modules/zlib/src    -fPIC  -Qunused-arguments  -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/Unified_cpp_gfx_layers4.o.pp -Qunused-arguments  -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-inline-new-delete -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -DNO_X11 -pipe  -DNDEBUG -DTRIMMED -g -O3 -fomit-frame-pointer  -I/Users/gal/workspace/mozilla-central/obj-x86_64-apple-darwin13.1.0/dist/include/cairo -DNO_X11 -I/frameworks/base/include/media/stagefright -I/frameworks/base/include/media/stagefright/openmax -I/frameworks/av/include/media/stagefright -I/frameworks/native/include/media/openmax    /Users/gal/workspace/mozilla-central/obj-x86_64-apple-darwin13.1.0/gfx/layers/Unified_cpp_gfx_layers4.cpp
/Applications/Xcode.app/Contents/Developer/usr/bin/make libs
libgfx_layers.a.desc
rm -f libgfx_layers.a 
/Users/gal/workspace/mozilla-central/obj-x86_64-apple-darwin13.1.0/_virtualenv/bin/python /Users/gal/workspace/mozilla-central/config/expandlibs_gen.py --depend .deps/libgfx_layers.a.desc.pp -o libgfx_layers.a.desc ShadowLayerUtilsMac.o MacIOSurfaceImage.o BasicImageLayer.o ImageContainer.o Layers.o LayerTreeInvalidation.o MacIOSurfaceTextureHostBasic.o MacIOSurfaceTextureClientOGL.o MacIOSurfaceTextureHostOGL.o Unified_cpp_gfx_layers0.o Unified_cpp_gfx_layers1.o Unified_cpp_gfx_layers2.o Unified_cpp_gfx_layers3.o Unified_cpp_gfx_layers4.o Unified_cpp_gfx_layers5.o 
/Applications/Xcode.app/Contents/Developer/usr/bin/make tools
make[1]: Nothing to be done for `tools'.
furball:layers gal$

Comment 77

[Markus Stange [:mstange]]

That was new to me, interesting!

Comment 78

[Bas Schouten (:bas.schouten)]

Comment on attachment 8428727 [details] [diff] [review]
Fix tex coord wrapping that was broken by part 6

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

This looks fine. As Markus says, if it generates warnings for opts, let's fix it, but I don't think statics ever do.

Comment 79

[Andreas Gal :gal]

The fixes for part 6: https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6ae4c0a754

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b6ae4c0a754

Comment 85

[Andreas Gal :gal]

Part 7: https://hg.mozilla.org/integration/mozilla-inbound/rev/e6ab1b91f61c

Comment 86

[Andreas Gal :gal]

Part 8: https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b417d564b5

Comment 87

[Andreas Gal :gal]

Part 9: https://hg.mozilla.org/integration/mozilla-inbound/rev/8815630582d1

If this sticks we are done here. We no longer generate any textures that use GL_REPEAT, and as a result this should eliminate the visual glitches related to a layer edge bleeding into the opposite side (as seen on FFOS in the task switcher, for example).

Comment 88

[Andreas Gal :gal]

Fix for rebase breakage: https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe2a43a768b

Comment 89

[Ryan VanderMeulen [:RyanVM]]

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

Comment 90

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a1b417d564b5
https://hg.mozilla.org/mozilla-central/rev/8815630582d1
https://hg.mozilla.org/mozilla-central/rev/0fe2a43a768b