Closed Bug 1415034 Opened 3 years ago Closed 3 years ago

Convert drop-shadow CSS filter to WebRender

Categories

(Core :: Graphics: WebRender, enhancement, P1)

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

(Keywords: feature, Whiteboard: [wr-reserve])

Attachments

(1 file, 4 obsolete files)

Add drop-shadow support. (carry r+: kats)
9.29 KB, patch
mtseng
: review+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #1370564 +++
Whiteboard: [wr-mvp][wr-triage] → [wr-mvp] [triage]
Assignee: nobody → ethlin
Blocks: stage-wr-trains
No longer blocks: stage-wr-nightly
Whiteboard: [wr-mvp] [triage] → [wr-mvp] [triage][wr-reserve-candidate]
Whiteboard: [wr-mvp] [triage][wr-reserve-candidate] → [wr-reserve]
Assignee: ethlin → nobody
This bug is related fb performance. I think we probably want to address this for wr-nightly based on bug 1411813 comment 13.
Whiteboard: [wr-reserve] → [wr-reserve][triage]
Whiteboard: [wr-reserve][triage] → [wr-mvp] [triage]
Whiteboard: [wr-mvp] [triage] → [wr-reserve]
Sure, we can move it to nightly.  Still want to wait a bit before we work on it, until we clear out some of the non-fallback types of performance issues.
Blocks: stage-wr-nightly
No longer blocks: stage-wr-trains
Duplicate of this bug: 1417643
(In reply to Milan Sreckovic [:milan] from comment #2)
> Sure, we can move it to nightly.  Still want to wait a bit before we work on
> it, until we clear out some of the non-fallback types of performance issues.

I think this bug is needed to reduce the fallback items on fb and MotionMark. It's not improving the fallback path performance. Change to P2?
Flags: needinfo?(milan)
I'm starting work on this.
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [wr-reserve] → [wr-mvp]
Flags: needinfo?(milan)
Upload a gecko wip. Still depend on webrender pull request. pr is here https://github.com/servo/webrender/pull/2091
Whiteboard: [wr-mvp] → [wr-reserve]
Attached patch Add drop-shadow support. (obsolete) — — Splinter Review 
The long-chain.html failure is tracking by
https://github.com/servo/webrender/issues/2197

MozReview-Commit-ID: FECidSvTQrY
Attachment #8935625 - Flags: review?(bugmail)
Attachment #8931585 - Attachment is obsolete: true
Comment on attachment 8935625 [details] [diff] [review]
Add drop-shadow support.

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

r+. One suggestion below. I don't feel too strongly about it but it would make it a little cleaner I think.

::: gfx/webrender_bindings/src/bindings.rs
@@ +365,5 @@
>  #[repr(C)]
>  #[derive(Copy, Clone)]
>  pub struct WrFilterOp {
>      filter_type: WrFilterOpType,
> +    argument: [c_float; 7],

Instead of lumping all 7 floats into one array it might be a bit cleaner to separate this into three fields:

argument: c_float, // holds radius for DropShadow; value for other filters
offset: LayoutVector2D, // only used for DropShadow
color: ColorF, // only used for DropShadow

and then all the existing filters can just use the same c_filter.argument unchanged, and you can use the extra fields in a more strongly-typed fashion for the DropShadow filter. cbindgen should be able to deal with this correctly.
Attachment #8935625 - Flags: review?(bugmail) → review+
Thanks for suggestion. The code becomes much cleaner now.
Comment on attachment 8931585 [details]
Bug 1415034 - Add drop-shadow support.

oops, accidentally sending a review.
Attachment #8931585 - Flags: review?(bugmail)
Attachment #8931585 - Attachment is obsolete: true
The long-chain.html failure is tracking by
https://github.com/servo/webrender/issues/2197

MozReview-Commit-ID: FECidSvTQrY
Attachment #8936415 - Flags: review+
Attachment #8935625 - Attachment is obsolete: true
Fix try failures.

MozReview-Commit-ID: FECidSvTQrY
Attachment #8936437 - Flags: review+
Attachment #8936415 - Attachment is obsolete: true
The long-chain.html failure is tracking by
https://github.com/servo/webrender/issues/2197

MozReview-Commit-ID: FECidSvTQrY
Attachment #8936463 - Flags: review+
Attachment #8936437 - Attachment is obsolete: true
Backed out for build bustage at layout/painting/nsDisplayList.cpp:9992:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcae439efd9ce769f1412b14d2c4ce3205117eb9

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fd2a2e17aef386ad47cfbd2259b7dd791999d194&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=151474795&repo=mozilla-inbound

/builds/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:9992:11: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
/builds/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:9994:11: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
Flags: needinfo?(mtseng)
Sorry for that, submit another patch to fix it.
Flags: needinfo?(mtseng)
https://hg.mozilla.org/mozilla-central/rev/8859d342fd59
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1430789
You need to log in before you can comment on or make changes to this bug.