Closed Bug 1438017 Opened 2 years ago Closed 2 years ago

Update wr bindings for filters and add filters to command builder

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file, 1 obsolete file)

This is a followup to https://github.com/servo/webrender/pull/2323.

Also, I added API for defining filters in WebRenderCommandBuilder::BuildWebRenderCommands to make it useful in bug 1431466.
Assignee: nobody → eitan
Blocks: 1431466
Comment on attachment 8950746 [details] [diff] [review]
Update wr bindings with ColorMatrix filter and add filters to command builder. r?kats

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

Minus for now but only because regenerating the FFI header (see below) will probably remove the operator== on that struct and I'm not sure if that'll cause build failures or not. If the comments are addressed without problems I'm happy to r+ this.

::: gfx/layers/wr/WebRenderCommandBuilder.h
@@ +51,5 @@
>                                nsDisplayList* aDisplayList,
>                                nsDisplayListBuilder* aDisplayListBuilder,
>                                WebRenderScrollData& aScrollData,
> +                              wr::LayoutSize& aContentSize,
> +                              const nsTArray<wr::WrFilterOp>& aFilters = nsTArray<wr::WrFilterOp>());

Drop the default value here, you've already provided a value at the only call site of this function

::: gfx/webrender_bindings/webrender_ffi_generated.h
@@ +676,5 @@
>    WrFilterOpType filter_type;
>    float argument;
>    LayoutVector2D offset;
>    ColorF color;
> +  float matrix[20];

Looks like you hand-modified this? This file is meant to be autogenerated, although right now the step to do that is slightly broken. You have to:
(1) apply https://hg.mozilla.org/try/rev/9d8d9b8971961e87116ca5a1684aa9f5889d9e82 locally
(2) follow the instructions at the top/bottom of this file to regenerate it
(3) unapply the thing from step (1)
Attachment #8950746 - Flags: review?(bugmail) → review-
This still builds without the operator==, but will it introduce regressions? Running a try on this.
Attachment #8951004 - Flags: review?(bugmail)
Attachment #8950746 - Attachment is obsolete: true
Comment on attachment 8951004 [details] [diff] [review]
Update wr bindings with ColorMatrix filter and add filters to command builder. r?kats

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

LGTM. If it compiles it's probably fine - the operator is autogenerated but probably not being used anywhere.
Attachment #8951004 - Flags: review?(bugmail) → review+
Keywords: checkin-needed
Nope, need to rebase this manually first.
Keywords: checkin-needed
Pushed by eisaacson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae79bc0bbd31
Update wr bindings with ColorMatrix filter and add filters to command builder. r=kats
https://hg.mozilla.org/mozilla-central/rev/ae79bc0bbd31
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.