Closed Bug 1274692 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Web Painting, defect, P3)

46 Branch
Unspecified
All
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: toriningen.me, Assigned: ethlin)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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.
OS: Unspecified → All
Version: 46 Branch → 49 Branch
Component: Untriaged → Layout: View Rendering
Product: Firefox → Core
Version: 49 Branch → 46 Branch
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?
CJ, any chance you could look in to what's happening here?
Flags: needinfo?(cku)
Sure. It happens on Mac only, I can not reproduce it on windows device. Keep need-info
Assignee: nobody → cku
Flags: needinfo?(cku)
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.
[68221] WARNING: Surface size too large (exceeds CoreGraphics limit)!: file /Users/cjcool/repository/gecko-dev/gfx/thebes/gfxASurface.cpp, line 400
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.
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
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?
Flags: needinfo?(milan)
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?
Flags: needinfo?(milan) → needinfo?(mchang)
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
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.
Flags: needinfo?(mchang)
Depends on: 1258751
Switch to TPE gfx engineer
Assignee: cku → ethlin
CG is removed. So we can remove the related limitation.
Attachment #8757993 - Attachment is obsolete: true
Attachment #8802844 - Flags: review?(mstange)
Attachment #8802844 - Flags: review?(mstange) → review+
(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.
(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
Flags: needinfo?(mstange)
(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
Flags: needinfo?(mstange)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06ed2770b58d
Remove CG limitation since the backend is skia. r=mstange
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/06ed2770b58d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.