854873 - css animations aren't clipped correctly

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[alexander.jautz]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130307023931

Steps to reproduce:

Animating a filmstrip image with css animation in a div container.

example: http://gt.letzfetz.com/


Actual results:

The animation with the smaller image (2280px x 152px) doesn't get clipped correctly by the surrounding div and the whole filmstrip is visible.
The animation with the larger image (4097px x 152px) works.

This happens with a Galaxy S3 
Android 4.1.1
Firefox 19.0.2 

With a Galaxy Nexus the minimum size for a working animation is different (>2048). So I think this has something to do with the max texture size of the mobile device?

example: http://gt.letzfetz.com/


Expected results:

The whole filmstrip shouldn't be visible.

Comment 1

[Chris Lord [:cwiiis]]

Moving to gfx and cc'ing BenWa and nrc - sounds like a layers clipping issue, possibly involving tiled layers.

Comment 2

[Nick Cameron [:nrc]]

We think this is a regression since 18. I can't reproduce on my Nexus 7 or HTC One, but kentuckyfriedtakahe did repro on an S3. I'll see if I can mess with texture sizes or something to repro.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Any update here, nrc?

Comment 5

[Nick Cameron [:nrc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Any update here, nrc?

Afraid not, I couldn't repro it on any of my devices and been busy with other things so haven't got to it. Do you have a device I can repro on? If so, then I could have a look while I'm in TO.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I gave my Nexus 4 to nrc since it can repro the problem. He said he will look into it.

Comment 7

[Nick Cameron [:nrc]]

So this is bug is a barrel of fun. Turns out that clipping doesn't work because we never actually enable GL_SCISSOR_TEST. Lucky we never really use clipping :-p I'm amazed we didn't notice this. On the bright side, we ought to make a performance gain from this. Note that we would only notice this when an _active_ layer would otherwise over-write something else (I noticed this on Mac last year because an animated transform caused painted over the browser chrome). This bug did not appear on my devices because the page is layerised differently - the animated numbers are Thebes layers, whereas on Nexus 4 (where I can repro) the top number is an image layer and the bottom is a Thebes layer, on desktop both are image layers - I assume in the Thebes layer case, clipping is done by layout. A hypothesis for why this happens is that it is due to the maximum texture size on each device - that fits with the bug in this test case only manifesting for the larger image (hypothesis courtesy of roc; I'll test it tomorrow). I'm also assuming that scissor test is working on desktop (we would notice if it were not, and we don't see the bug even though we have image layers). I have not tested b2g.

Enabling scissor test doesn't actually fix this - the transform is wrong so we need to fix that as well as making sure scissor test is enabled. Again, I'll look into it tomorrow.

Comment 8

[Nick Cameron [:nrc]]

Looks like we enable the scissor test here:

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/CompositorOGL.cpp#l829

I have no idea why it doesn't actually work (I guess it should happen before the calls to fScissor just above, but maybe it doesn't matter as long as we call before presenting).

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Not sure if this is relevant at all, but in our Java code (invoked from DrawWindowUnderlay) we do some stuff with scissoring too: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/LayerRenderer.java#594

We've had problems before where the GL state left by Java wasn't what was expected by C++ (and vice-versa) so it might be another instance of that.

Comment 10

[Nick Cameron [:nrc]]

Attachment: patch

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 813624 [details] [diff] [review]
patch

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

This looks sane to me, but I don't think I'm qualified to really r+ this change. Looking at some file histories suggests maybe mattwoodrow or nsilva could also review? Not really sure.

Comment 12

[Nick Cameron [:nrc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Comment on attachment 813624 [details] [diff] [review]
> patch
> 
> Review of attachment 813624 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks sane to me, but I don't think I'm qualified to really r+ this
> change. Looking at some file histories suggests maybe mattwoodrow or nsilva
> could also review? Not really sure.

Thanks, going for nical.

Comment 13

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=72e18be07a2e

Comment 14

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23b807108524

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/23b807108524

Comment 19

[Nick Cameron [:nrc]]

Comment on attachment 813624 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: incorrect clipping in some cases
Testing completed (on m-c, etc.): been on trunk for a few days
Risk to taking this patch (and alternatives if risky): low to medium - not many changes and nothing outrageous, but not trivial either and we obviously don't have great testing in this area or we would have caught this bug earlier.
String or IDL/UUID changes made by this patch: none

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f80fd27d367f

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Target milestone is used to track when it landed on m-c. Please don't un-set it unless you're backing out.