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

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla32
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 11 obsolete attachments)

4.10 KB, patch
gal
: review+
Details | Diff | Splinter Review
12.32 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
5.59 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
14.28 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
16.59 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
15.33 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
6.48 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
3.31 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
963 bytes, patch
BenWa
: review+
Details | Diff | Splinter Review
19.05 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
6.42 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
No description provided.
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.
Attachment #8424477 - Attachment is patch: true
Attachment #8424477 - Attachment mime type: text/x-patch → text/plain
Attachment #8424477 - Flags: review?(bas)
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.
Attachment #8424477 - Flags: review?(bas) → review+
Whiteboard: keep-open
Bas, if you agree with the shader change please checkin-needed it.
Flags: needinfo?(bas)
Posted patch Part 7: Remove VBOArena (obsolete) — Splinter Review
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.
Attachment #8426208 - Flags: review?(bas)
Attachment #8426209 - Flags: review?(bas)
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.
Attachment #8426210 - Flags: review?(bas)
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.
Attachment #8426211 - Flags: review?(bas)
Attachment #8426212 - Flags: review?(bas)
Attachment #8426212 - Attachment is obsolete: true
Attachment #8426212 - Flags: review?(bas)
Attachment #8426215 - Flags: review?(bas)
Attachment #8426215 - Attachment is obsolete: true
Attachment #8426215 - Flags: review?(bas)
Attachment #8426217 - Flags: review?(bas)
Removing VBOArena since its unused. For complex geometries (CSS mesh transforms) I want to cache the geometry instead of the VBO arena stuff.
Attachment #8426213 - Attachment is obsolete: true
Attachment #8426218 - Flags: review?(bas)
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.
Attachment #8426224 - Flags: review?(bas)
Attachment #8426208 - Flags: review?(bas) → review+
Attachment #8426209 - Flags: review?(bas) → review+
Attachment #8426210 - Flags: review?(bas) → review+
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?
Attachment #8426211 - Flags: review?(bas) → review+
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 on attachment 8426218 [details] [diff] [review]
Part 8: Remove VBOArena

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

So much r+...
Attachment #8426218 - Flags: review?(bas) → review+
Attachment #8426224 - Flags: review?(bas) → review+
Attachment #8426217 - Flags: review?(bas) → review+
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)
This patch likely regresses performance stand-alone (on Mac TALOS) so please land together with part 6.
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.
Attachment #8426586 - Attachment is obsolete: true
Attachment #8426588 - Attachment is obsolete: true
Attachment #8426589 - Attachment is obsolete: true
The last part switches on CLAMP. I want to wait until the rest has cycled and talos results are back before landing that.
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
Flags: needinfo?(gal)
Thanks Wes!
Flags: needinfo?(gal)
Posted file Test (obsolete) —
Attachment #8426781 - Attachment is obsolete: true
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.
Actually on mac even without my patch if CanUploadNonPowerOfTwo is false everything falls apart so thats not it.
Wes, I think I am getting green with part 5 and 6 with those tests. Can you confirm?
Flags: needinfo?(kwierso)
Yeah, that try run looks fine.
Flags: needinfo?(kwierso)
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
(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
Depends on: 1015176
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.
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
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)
BenWa will r+ the fix and I will upload the whole part again for review from Bas for context.
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.
Attachment #8428171 - Flags: review+
Duplicate of this bug: 1015176
Attachment #8428171 - Attachment description: Fix for bug 1015176 → Part 9: Fix for bug 1015176
Attachment #8426217 - Attachment description: Part 6: Eliminate VBO use and DrawQuads and manually draw no-repeat quads → Part 7: Eliminate VBO use and DrawQuads and manually draw no-repeat quads
Attachment #8426218 - Attachment description: Part 7: Remove VBOArena → Part 8: Remove VBOArena
Attachment #8426224 - Attachment description: Part 8: Use CLAMP instead of GL_REPEAT → Part 9: Use CLAMP instead of GL_REPEAT
Attachment #8428171 - Attachment description: Part 9: Fix for bug 1015176 → Fix for part 6: Fix for bug 1015176
Attachment #8428180 - Attachment description: DecomposeIntoNoRepeatRects with added tex coord normalization → 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
Attachment #8428180 - Flags: review?(bas)
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.
Attachment #8428180 - Flags: review?(bas) → review+
Posted patch Part 6 nits (obsolete) — Splinter Review
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?
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
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
Posted patch Fix tex coord wrapping. (obsolete) — Splinter Review
Attachment #8428195 - Attachment is obsolete: true
Attachment #8428335 - Attachment description: patch → Fix tex coord wrapping.
Attachment #8428335 - Flags: review?(bas)
Try with debug that actually caught the exceptions:

https://tbpl.mozilla.org/?tree=Try&rev=8aab19771902
The try chooser seems to hate me. Hopefully this works: https://tbpl.mozilla.org/?tree=Try&rev=902a053490d1
./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
Sanitize tex coords even harder (and saner):

https://tbpl.mozilla.org/?tree=Try&rev=ef5bb323140a
Fixed the tex coord wrapping code for real now (passed try). This should also be a lot more readable now.
Attachment #8428335 - Attachment is obsolete: true
Attachment #8428335 - Flags: review?(bas)
Attachment #8428516 - Flags: review?(bas)
Attachment #8428516 - Attachment is obsolete: true
Attachment #8428516 - Flags: review?(bas)
Attachment #8428727 - Flags: review?(bas)
Duplicate of this bug: 1015964
Duplicate of this bug: 1015922
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.
static inline should not produce a warning if unused.
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$
That was new to me, interesting!
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.
Attachment #8428727 - Flags: review?(bas) → review+
Depends on: 1016055
Blocks: 1016086
Duplicate of this bug: 1016513
Duplicate of this bug: 1016023
Duplicate of this bug: 1016210
Duplicate of this bug: 1016877
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).
Whiteboard: keep-open
Flags: needinfo?(bas)
Depends on: 1018996
You need to log in before you can comment on or make changes to this bug.