Closed Bug 854873 Opened 8 years ago Closed 7 years ago

css animations aren't clipped correctly


(Core :: Graphics: Layers, defect)

Not set



Tracking Status
firefox26 + fixed
firefox27 --- fixed


(Reporter: alexander.jautz, Assigned: nrc)




(1 file)

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.


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?


Expected results:

The whole filmstrip shouldn't be visible.
OS: Windows 7 → Android
Component: General → Graphics, Panning and Zooming
Ever confirmed: true
Hardware: x86_64 → ARM
Moving to gfx and cc'ing BenWa and nrc - sounds like a layers clipping issue, possibly involving tiled layers.
Component: Graphics, Panning and Zooming → Graphics: Layers
Product: Firefox for Android → Core
Version: Firefox 19 → Trunk
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.
Duplicate of this bug: 922097
Any update here, nrc?
Flags: needinfo?(ncameron)
(In reply to Kartikaya Gupta ( 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.
Flags: needinfo?(ncameron)
I gave my Nexus 4 to nrc since it can repro the problem. He said he will look into it.
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.
Looks like we enable the scissor test here:

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).
Not sure if this is relevant at all, but in our Java code (invoked from DrawWindowUnderlay) we do some stuff with scissoring too:

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.
Attached patch patchSplinter Review
Assignee: nobody → ncameron
Attachment #813624 - Flags: review?(bugmail.mozilla)
Comment on attachment 813624 [details] [diff] [review]

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.
Attachment #813624 - Flags: review?(bugmail.mozilla)
Attachment #813624 - Flags: review?
Attachment #813624 - Flags: review+
Attachment #813624 - Flags: review? → review?(nical.bugzilla)
(In reply to Kartikaya Gupta ( 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.
Attachment #813624 - Flags: review?(nical.bugzilla) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
No longer blocks: 890199
Duplicate of this bug: 890199
No longer blocks: 866585
Duplicate of this bug: 866585
Duplicate of this bug: 920548
Target Milestone: mozilla27 → ---
Comment on attachment 813624 [details] [diff] [review]

[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
Attachment #813624 - Flags: approval-mozilla-aurora?
Attachment #813624 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Target milestone is used to track when it landed on m-c. Please don't un-set it unless you're backing out.
Target Milestone: --- → mozilla27
Duplicate of this bug: 927595
Duplicate of this bug: 857282
Duplicate of this bug: 929402
You need to log in before you can comment on or make changes to this bug.