1274692 - Using CSS filter on elements taller than 16383px causes element to be cropped at 2048px height

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P3)

Description

[toriningen]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160502172042

Steps to reproduce:

Create any element of height 16384px or more and give it any CSS filter.

See interactive fiddle here: https://jsfiddle.net/toriningen/hhcf56nc/1/


Actual results:

Element will crop at 2048px height, rest of element will be rendered transparent.


Expected results:

It should have rendered in full regardless of height.

Comment 1

[toriningen]

Another example where this problem is live:

1. visit https://mrakopedia.ru/wiki/%D0%94%D1%80%D1%83%D0%B6%D0%B1%D0%B0_%E2%80%94_%D1%8D%D1%82%D0%BE_%D0%BE%D0%BF%D1%82%D0%B8%D0%BC%D1%83%D0%BC
2. either click on "ночной режим" link on the topmost bar, or execute `toggleNightMode()` from dev console.
3. scroll approx 2-3 screens down to see truncated content. You can toggle night mode once again to see content appear.

Removing CSS filter via devtools makes content appear back. If there is no plan to fix this soon, is there any workaround that can be applied to make content visible, yet with filter applied?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

CJ, any chance you could look in to what's happening here?

Comment 3

[u459114]

Sure. It happens on Mac only, I can not reproduce it on windows device. Keep need-info

Comment 4

[u459114]

1. drop-shadow/ blur: Element is cropped at 2048px height, even if spanner.style.height == 16383px
2. I can reproduce this problem after switch to other backends, such as Skia.

Comment 5

[u459114]

[68221] WARNING: Surface size too large (exceeds CoreGraphics limit)!: file /Users/cjcool/repository/gecko-dev/gfx/thebes/gfxASurface.cpp, line 400

Comment 6

[u459114]

Because of this limitation, we see surface trimmed:
https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxASurface.cpp#399

This limitation existed, I think, before we support tile rendering, we should extend this limitation  when "layers.enable-tiles" pref-on.

Comment 7

[u459114]

Attachment: MozReview Request: Bug 1274692 - skip CG image height limitation check

Review commit: https://reviewboard.mozilla.org/r/56344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56344/

Comment 8

[u459114]

Basic idea is
nsSVGUtils::ConvertToSurfaceSize is called at three places
1. nsSVGMaskFrame::GetMaskForMaskedFrame
   call ConvertToSurfaceSize to get a boundary size, base on this size, create a DrawTarget. 
   In this case, there is no need to do CG-image-height-upper-bound check[1]. Two reasons.
   I. The created DrawTarget may not be DrawTargetCG. Even on MacOS, it could be a DrawTargetSkia.
   II. Even if it's a DrawTargetCG, we can still check by DrawTargetCG::IsValid on the fly.

2. nsFilterInstance::OutputFilterSpaceBounds(): we are not create any buffer here, there is no need to check CG image height in this path either.
3. nsSVGPatternFrame::PaintPattern: reason is the same with #1.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxASurface.cpp#399

Comment 9

[u459114]

Hi Milan,
It's a domain that I am not that familiar. May I know who it the right person to feedback/review this change?

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

We're actually removing CG as a valid OS X backend, so we may be able to just removed that code completely.  Mason, when did we get rid of CG?

Comment 11

[u459114]

Is there a META bug for remove CG? I want to remove some workaround code, such as [1], after that.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp#467

Comment 12

[Mason Chang [Inactive] [:mchang]]

Deleting CG will happen in bug 1258751, but not until Skia content on OS X hits release. Skia content is riding the trains starting in 48, so we still have a few more releases to go until we can delete CG.

Comment 13

[u459114]

Switch to TPE gfx engineer

Comment 14

[Ethan Lin[:ethlin]]

Attachment: remove cg surface limitation

CG is removed. So we can remove the related limitation.

Comment 15

[Markus Stange [:mstange]]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #8)
> 2. nsFilterInstance::OutputFilterSpaceBounds(): we are not create any buffer
> here, there is no need to check CG image height in this path either.

I agree.

> nsSVGUtils::ConvertToSurfaceSize is called at three places
> 1. nsSVGMaskFrame::GetMaskForMaskedFrame
>    call ConvertToSurfaceSize to get a boundary size, base on this size,
> create a DrawTarget. 

> 3. nsSVGPatternFrame::PaintPattern: reason is the same with #1.

If these two cases cause us to clip rendering, then we're not computing efficient intermediate surface sizes. We should not need any surfaces that are bigger than the display port, regardless of the size of the whole element.

Comment 16

[u459114]

(In reply to Markus Stange [:mstange] from comment #15)
> If these two cases cause us to clip rendering, then we're not computing
> efficient intermediate surface sizes. We should not need any surfaces that
> are bigger than the display port, regardless of the size of the whole
> element.
Hi mstange,
Do you know where this limitation comes from? How to limited clip region of a gfxContext in the size of display port?

Does my patch in bug 1299715 break this limitation? 
http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#2436

Comment 17

[u459114]

(In reply to C.J. Ku[:cjku](UTC+8) from comment #16)
> (In reply to Markus Stange [:mstange] from comment #15)
> > If these two cases cause us to clip rendering, then we're not computing
> > efficient intermediate surface sizes. We should not need any surfaces that
> > are bigger than the display port, regardless of the size of the whole
> > element.
> Hi mstange,
> Do you know where this limitation comes from? How to limited clip region of
> a gfxContext in the size of display port?
> 
> Does my patch in bug 1299715 break this limitation? 
> http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#2436
No, it shouldn't

Comment 18

[Ethan Lin[:ethlin]]

try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85a7502f393e8c1a0e97565c656da7484bfa36d4&selectedJob=31807418

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06ed2770b58d
Remove CG limitation since the backend is skia. r=mstange

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06ed2770b58d