Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upImplementing Blur filter #5546
Implementing Blur filter #5546
Conversation
hoppipolla-critic-bot
commented
Apr 6, 2015
|
Critic review: https://critic.hoppipolla.co.uk/r/4533 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
|
@SimonSapin and @pcwalton I have one question about paint_context + StackingContext. As was commented, it might be needed to adjust the overflow regions or (quote) “else display list optimization might clip out the edges of a blurred object”. But reading the code, I noticed that the filters are only draw (e.g. http://mxr.mozilla.org/servo/source/components/gfx/paint_context.rs#923) way past the display list optimizer has executed (i.e. http://mxr.mozilla.org/servo/source/components/gfx/display_list/mod.rs#383). If so, that would require changes on optimize_and_draw_into_context() i.ie to take into account the fact that there are filters before performing the display list optimization? |
|
Testing with the following test case (used 512px as IIRC, that is the default tile size): https://gist.github.com/Adenilson/ae28080d9fc25bb64ff1 Got attached result. The clipping probably is responsible for the smaller size, but what about the white lines in the green rectangle?
|
|
@Adenilson That indicates that your intermediate draw target sizes are incorrect. The white lines are on tile boundaries (the sizes of which can be changed with the |
|
@Adenilson Looking at the |
|
WIP. |
maxhoffmann
commented
Apr 14, 2015
|
Some time ago I saw this video about color blurring being broken. Not sure if this is the right thread to mention it, but maybe Servo could be the first rendering engine to blur colors correctly? :) |
|
@maxhoffmann We (Servo) will not unilaterally implement a behavior incompatible with other browsers, however more correct it might be. This is a proposal for W3C. That said, is the |
| impl ToCss for SpecifiedFilter { | ||
| fn to_css<W>(&self, dest: &mut W) -> text_writer::Result where W: TextWriter { | ||
| match *self { | ||
| SpecifiedFilter::Blur(value) => try!(write!(dest, "blur({:?})", value)), |
This comment has been minimized.
This comment has been minimized.
SimonSapin
Apr 14, 2015
Member
{:?} is not necessarily doing the right formatting here. Use value.to_css(dest) instead, like hue-rotate(). I made a PR into your PR’s branch Adenilson#1. Merging it should automatically update this one.
|
r=me on the |
maxhoffmann
commented
Apr 14, 2015
|
@SimonSapin As far as I understand the video, the calculations of the blended colors are calculated wrongly independent of the color space, so the mentioned property wouldn’t fix that. Thanks for the quick answer. |
|
“Square root everything” sounds a lot like a color space, but I don’t know. Regardless, rather than Servo just deviating from the spec, the right way to make this happen is to first discuss at W3C to change the spec. See “Feedback” at the top of http://dev.w3.org/fxtf/filters/ |
|
Related #5668. |
| // Pre-calculate if there is a blur expansion need. | ||
| let accum_blur = filters::calculate_accumulated_blur(filters); | ||
| let mut matrix = Matrix2D::identity(); | ||
| if accum_blur > Au::new(0) { |
This comment has been minimized.
This comment has been minimized.
|
|
||
| let temporary_draw_target = | ||
| self.draw_target.create_similar_draw_target(&size, self.draw_target.get_format()); | ||
| temporary_draw_target.set_transform(&self.draw_target.get_transform()); | ||
|
|
||
| if accum_blur > Au::new(0) { |
This comment has been minimized.
This comment has been minimized.
|
|
||
| // Pre-calculate if there is a blur expansion need. | ||
| let accum_blur = filters::calculate_accumulated_blur(filters); | ||
| let mut matrix = Matrix2D::identity(); |
This comment has been minimized.
This comment has been minimized.
pcwalton
Apr 15, 2015
Contributor
Why not just use let mut matrix = self.draw_target.get_transform();? Then you wouldn't need the second if accum_blur > Au(0) { ... below.
| let (filter_node, opacity) = filters::create_filters(&self.draw_target, | ||
| temporary_draw_target, | ||
| filters); | ||
| filters, &mut accum_blur); |
This comment has been minimized.
This comment has been minimized.
|
|
||
| // Create the Azure filter pipeline. | ||
| let mut accum_blur = Au::new(0); |
This comment has been minimized.
This comment has been minimized.
| self.draw_target.draw_filter(&filter_node, &rect, &rect.origin, draw_options); | ||
|
|
||
| // If there is a blur expansion, shift the transform and update the size. | ||
| if accum_blur > Au::new(0) { |
This comment has been minimized.
This comment has been minimized.
| let rect = Rect(Point2D(0, 0), size); | ||
| let side_inflation = accum_blur * BLUR_INFLATION_FACTOR; | ||
| let inflated_size = rect.inflate(side_inflation.to_nearest_px() as i32, side_inflation.to_nearest_px() as i32); | ||
| size = inflated_size.size; |
This comment has been minimized.
This comment has been minimized.
pcwalton
Apr 15, 2015
Contributor
If you're just using the size part of the rect, why use a rect and inflate() at all? Isn't size = Size2D((side_inflation.to_nearest_px() * 2) as i32, (side_inflation.to_nearest_px() * 2) as i32 enough?
|
Looks good to me. Squash away! |
|
Done. |
|
@bors-servo: r+ |
|
|
See discussion on #5190 and #5496. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5546) <!-- Reviewable:end -->
Adenilson commentedApr 6, 2015
See discussion on #5190 and #5496.