Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(gatsby-image): Enhanced Blur-up CSS for placeholderImage #10702

Conversation

polarathene
Copy link
Contributor

@polarathene polarathene commented Dec 28, 2018

Adds CSS filter: blur() and transform: scale() properties. Blur size is configurable via the blurSize prop, defaults to 16px(Not sure if this is a good default value or if there should be some heuristic based on stretched size). Conditionally checks to avoid applying to tracedSVG placeholders where it's unlikely to be desirable?

This improves the "blur-up" effect to be of a higher quality as the placeholder image is a stretched low pixel thumbnail(width of 20px) at the low quality, it can look rather awful especially for jpeg with artifacts and when representing a fairly large image size.

Browser support is pretty good, 90% Global, 80% India, 74% China. Even for those that don't have support, the result is just without the nicer blur effect applied?

Adds CSS filter.blur and transform.scale properties. Blur size is configurable via the `blurSize` prop, defaults to 16px. Conditionally checks to avoid applying to tracedSVG placeholders where it's unlikely to be desirable?

This improves the "blur-up" effect to be of a higher quality as the placeholder image is a stretched low pixel thumbnail(width of 20px) at the low quality, it can look rather awful especially for jpeg with artifacts and when representing a fairly large image size.
@pieh
Copy link
Contributor

pieh commented Dec 28, 2018

blur filter is one of the most expensive ones (both on performance and battery life for mobiles) so I'm not sure I like using it by default. People can opt-in to use it currently by using placeholderStyle, so maybe alternative would be writing docs for it?

@LekoArts
Copy link
Contributor

Agree with @pieh and would also rather have one or two sentences in the doc mentioning that.

@polarathene
Copy link
Contributor Author

blur filter is one of the most expensive ones (both on performance and battery life for mobiles)

@pieh Is it really that bad these days? As we approach 2019, I think we've got very capable devices since this was first introduced into spec late 2012 and many of the articles on using it around 2013-2016(quite a few in 2015).

I tried to take a look for information about impact on battery life or performance, didn't really see anything specifically about battery(that wasn't related to the perf hit). Usually the performance issues were from applying large blur radius values(>50px) on very large images(eg background cover/header). Many were also due to CPU based filters, which afaik modern browsers have supported GPU/hardware acceleration for quite a bit, which should be even more the case on mobile devices to reduce processing time and thus power drawn to perform the task.

Some users also found they could enable GPU acceleration(not just for filter use cases) and get a massive improvement in perf, the additional transform I've added might encourage that behaviour, though afaik if the browser supports doing it on the GPU and deems it better suited for a task like blur it'll offload computation for doing that.

Personally I'm not noticing any difference from using blur on images <500px x 500px on my current project. I've tried in both Firefox and Chrome and used the performance/profiling tools, no noticeable impact from having blur on/off.

And there really shouldn't be if there is no reason to reapply the blur filter to that layer/texture, such as the dimensions or content changing. Gatsby sites are intended to be fast to load, so those blurred placeholders won't be there for long, the blur effect itself is unlikely to be that much of a burden?

If there was good data to suggest a high % of users were experiencing negative impact in performance/battery in exchange for a more visually acceptable image, it'd be a more convincing to be concerned about. If there isn't much of a real world hit, and users can avoid seeing large low quality placeholders with artifacts(which give off a bad impression imo), it seems it'd be a better default on(I'm sure users of Gatsby would chime in if the feature was causing issue).

Another argument for it as a default is Medium does this rather successfully(rather than CSS blur filter, via a canvas and JS, but I'm not sure that's really any better beyond ensuring you only blur and rasterize the input once).

Facebook does it too, though I believe that was native blur for mobile app, they still went with a blur on the placeholder(and that's still going to have some perf hit and battery drain to apply, how much that compares to CSS blur filter in browsers today I'm not sure).

@KyleAMathews seemed to implement the initial blur-up back in 2016? I think it might have been more understandable to not use CSS blur filter back then, the links mention it but #285 doesn't seem to discuss it's use.


That said, I can modify the PR to instead activate blur only when the prop is used instead? Or toggle it via an option property in gatsby-config.js?

I can imagine this is a feature specific and desirable enough that it makes more sense to do one of those vs instructing devs to maintain/implement their own copy(variation or snippet) for Gatsby projects whenever they want to use it(presumably sitewide with a prop to disable it, or blurSize of 0 to not apply?).

I can also update the docs once there is some input for direction on what to go above.

@polarathene
Copy link
Contributor Author

Just for visual comparison, without the blur:

base64_noblur20

With the blur:
base64_blur20

And with placeholder thumbs with double the pixel count:

No blur
base64_noblur

With the blur:
base64_blur

I guess the value varies based on image and how it compresses or if there is much care about the aesthetic for placeholders that aren't intended to last that long on the page.

Looks a bit cheap/tacky without the blur, perf/battery hit is highly likely to be barely anything to worry about for the tradeoff, that minimizes further as time goes by.

The docs also gave me the impression that the effect would actually involve a blur like Medium/Facebook:

On top of that, fluid returns everything else (namely aspectRatio and a base64 image to use as a placeholder) you need to implement the “blur up” technique popularized by Medium and Facebook - Source

or a blurry version of the image. - Source

Blurry pixelated view I guess? The image shared above was one of the ones in the project that made me confused if something was broken with the blur-up effect as I felt uncomfortable with it as a presentational UX element. Looked through Gatsby docs and repo issues to see if I had missed something, then dug into the code and compared to what articles on the topic had to say about it, noticed it was because an actual blur wasn't applied but implied from the low-res stretch...

Updating the docs will definitely help other users avoid the confusion :)

Articles like this one or this one both sold me on the impression of gatsby-image blur-up having a less pixelated blur to what I was getting, though in hindsight I guess that was due to the gifs being a fullscreen scaled-down and recompressed to gif? Neither article mentioned adding a CSS blur filter or similar to improve the outcome, both talked up likeness to Medium. There are more articles like that, which were common ones I came across when looking into what this "Gatsby" thing was that people were talking about.

@pieh
Copy link
Contributor

pieh commented Dec 28, 2018

@pieh Is it really that bad these days? As we approach 2019, I think we've got very capable devices since this was first introduced into spec late 2012 and many of the articles on using it around 2013-2016(quite a few in 2015).

I don't have hard measures on this and that's also reasonable thing to point out that it might not be the case anymore, but I wouldn't use fact that we are approaching 2019 and devices are more capable now (in general) as deciding factor - there is nuance to this. For example there is this notion about "next billion users" and it's not obvious if those will use performant devices or not.

In any case - please don't take my reluctance on this as "won't merge" attitude - I just want to be careful with defaults, and definitely need much more data and opinions on this.

@polarathene
Copy link
Contributor Author

In any case - please don't take my reluctance on this as "won't merge" attitude - I just want to be careful with defaults, and definitely need much more data and opinions on this.

No problem, I understand the reluctance and it's fair from a user perspective. As I'm unable to get actual metrics about the concern, I have no idea when it'll be viable as a default, but at some point I imagine it will be, Gatsby and it's plugins I believe had made decisions that have been better for the target audience compatibility wise excluding support for some browsers like IE(I think IE 8/11+ is required somewhere?).

For example there is this notion about "next billion users" and it's not obvious if those will use performant devices or not.

Hardware in the future is less likely to be a big regression that would suffer at this, unless we're talking ridiculously cheap devices. I think my flagship Samsung Galaxy S1 from 2010 can easily be beaten performance wise by modern smartphones going for $100 or less. That's due to the much better processor/gpu tech, costs being driven down, much improved OS and software(browsers), etc. I'd be more concerned with legacy devices.

I'm more than happy to make changes as suggested above, let me know what you guys would be happy with :)

  • Activate blur only when the prop is used instead?
  • Set blurSize of 0 to not apply on specific Img elements, other wise use the global default via an option property in gatsby-config.js, if not set, feature is not used. (This one sounds like the way to go to me)

@pieh
Copy link
Contributor

pieh commented Dec 28, 2018

I'd love more feedback, so small cc-spree: @fk @greglobinski @DSchau @m-allanson What's your take on this?

@DSchau
Copy link
Contributor

DSchau commented Dec 28, 2018

Thanks for tagging me in @pieh, I've been watching this one 👀

I just want to be careful with defaults, and definitely need much more data and opinions on this.

The biggest issue I'm seeing right now is that there isn't a ton of data readily available on this. It seems like your assertion that filter: blur can be slow without GPU acceleration is relatively supported, but I'm primarily just seeing some anecdotal evidence, blog posts, etc. so it's hard to know how significant an issue this could be were we to make this change.

We also have to strike a balance here. gatsby-image is one of the most useful and utilized components (and really a technique/optimization with gatsby-plugin-sharp and gatsby-transformer-sharp) that we provide, so we want to balance an incredibly fast experience with smart defaults that won't degrade or provide a slow or sluggish experience out-of-the-box.

Additionally--and this is where it gets a bit more subjective, we have to weigh whether a possible performance hit (on some devices, e.g. "the next billion" as @pieh said) is worth a seemingly small gain in a slightly nicer looking image--and thanks for including the images for comparison, the blurred images are certainly nicer!

So, in a round-about, long winded answer... I'm conflicted on this. I see both points here--I agree that it's pretty much objectively nicer looking with the blur applied, but I'm not necessarily sold whether that slight improvement is worth a (possible) performance issue/sluggish experience for some users.

Some things worth considering are whether we could swap to a different technique that could be more performant while also being slightly more visually pleasing for the blur effect. It appears that Medium uses a canvas element, Stripe uses an svg filter to blur, etc.

screen shot 2018-12-28 at 2 57 10 pm

screen shot 2018-12-28 at 3 01 02 pm

One thing we could do here is utilize something like Browserstack (or just an old device lying around) and experiment with using-gatsby-image or perhaps an example with quite a lot of images and seeing if there's a noticeable performance difference.

To provide some specific feedback:

Activate blur only when the prop is used instead?

I'm not necessarily against this, I just think there's a possibility the gatsby-image component itself can get bloated with each of these additional features. I'd prefer to keep it lightweight and focused, but I can see how this is a fair trade-off here for a nicer looking image.

Set blurSize of 0 to not apply on specific Img elements, other wise use the global default via an option property in gatsby-config.js, if not set, feature is not used. (This one sounds like the way to go to me)

What do you mean "specific Img elements" exactly? Again--there's a trade off here. I don't really like adding an option in gatsby-config.js when we don't have to--and we've already experienced some pains with pathPrefix so I'd really encourage we go a different direction here.

So, TL;DR, I'd largely agree with @pieh here. I see the value of this PR (and thank you so much for adding it!) in that it looks unquestionably nicer than the existing, but I'm not 100% sold it's worth a possible performance regression for some devices.

@polarathene
Copy link
Contributor Author

polarathene commented Dec 29, 2018

Some things worth considering are whether we could swap to a different technique that could be more performant while also being slightly more visually pleasing for the blur effect. It appears that Medium uses a canvas element, Stripe uses an svg filter to blur, etc.

Canvas might be used to ensure that there is no repaints on the blurred content to reapply the blur filter, it just becomes another rasterized image I think?

Negative would be that unlike CSS filter which would be applied much faster and work directly on the base64 inlined image data, you'll need to wait until JS can run to perform this blur task, and you need to ensure that the blur processing does use the GPU(Canvas seems to be not too different from CSS here in CPU/GPU usage varying, I don't have experience with canvas to know if you have any better control over such?). Personally, waiting on JS for this is a no-no, Gatsby is optimized for speed and I have noticed on slower connections with Medium(I'm in China presently) that their canvas preview is delayed vs the inline base64 stretch that gatsby-image provides that is ready to go immediately. I think this is also important for AMP pages(optimized for mobile devices and speed/performance), going with native CSS blur filter seems a better option.

Positive is, you can ensure that you apply the blur on a smaller size(eg upscale base64 image to 25-50% of the intended full size vs applying blur to 100% of full size), plus you can control what blur algorithm to use, opting for cheaper/lower quality ones. canvasBlurRect is 4 years old claiming 30 FPS real-time(full-screen desktop or iOS device) using such techniques, it should perform much better on todays hardware but gives a good indication of perf on what would be entry level like devices today and in the future.

SVG blur afaik is no different today, CSS blur filter came from the initial implementation of blur filter for SVG(well filters in general). Users don't seem to notice any perf difference(they have a perf issue for a fullscreen blur). I think it can have some different use cases vs CSS blur, Stripe might have just gone with it for compatibility reasons when it was implemented at the time(or the developers were more familiar / confident with it than the state/support of blur filter with CSS at the time in browsers). For this use-case, you're either going to add more weight to the payload or rely on JS to alter the DOM, neither beneficial vs native browser support with CSS blur filter.

One thing we could do here is utilize something like Browserstack (or just an old device lying around) and experiment with using-gatsby-image or perhaps an example with quite a lot of images and seeing if there's a noticeable performance difference.

It's difficult to profile this I think? Chrome had a useful continuous painting setting in it's dev tools a few years back, but it's only able to provide some profiling waterfall/timeline on paints(where I didn't see any noticeable difference in personal tests), or it's performance monitor which gives FPS(with my carousel component disabled and keeping the blur filter applied, there was no degradation on FPS(it only updates when there is painting, so you won't get a useful FPS value if the content is idle, toggling the effect doesn't reflect performance of 60FPS with this tool, regardless of CSS property).

SmashingMagazine did do their own comparison tests(mid 2016), it's not on the blur filter unfortunately, so a reproduction would be required to cater for that. It does conclude that they found the CSS filter to be fastest to paint over both the SVG filter and Canvas API. They use WebPageTest for getting their results rendered/recorded.

I still have my Galaxy S1 and a budget Huawei smartphone from 2014, they're in New Zealand, so I won't be able to test on them for a few months. I really don't see future devices being as limited as the S1 though, that was a single core 1GHz ARM CPU with 512MB RAM plus a rather old version of Android and weak GPU compared to what you can get with todays budget devices... tbh though it's slow at doing anything and battery life isn't too good after it's 5 years of use, browsing the web on it wasn't all that practical to begin with.

Note your link to HTML5 Rocks article, while a nice resource is from 2012(pretty early to when these filters were introduced). It's performance section does mention:

All is not lost with ‘blur’ since on some platforms it’s possible to use the GPU to accelerate it, but that’s not necessarily going to be available in every browser.

We’ve summarized the CSS filter effects available in the various browsers below with rough performance indicators for when they’re implemented in software. Note, that a number of modern browsers are beginning to implement these in hardware (GPU accelerated). When these are built with GPU support, performance will be greatly improved for the slower effects.

7 years later, I think it's fair to say things have changed and browsers used by the majority of users do have the GPU accelerated support for these filters, blur is far more efficient when offloaded to the GPU. Firefox has made quite a big leap in it's own performance for CSS rendering with it's Quantum and Stylo /WebRender projects, Chrome afaik takes performance seriously and has provided GPU acceleration for some time too.

Again, the other main issue with blur besides size of data it's applied on would be the radius set as pointed out in your articles and this StackOverflow answer. I've shown a rather conservative 16px and if a developer was finding certain uses to perform poorly, they could disable the filter effect on those images(eg a fullscreen landing page/hero image).

Other resources to support it as a default:

  • Full-screen modal blur perf when hardware-accelerated(April 2013). Horrible when it was CPU based, fine with the GPU. This is more of an extreme case, although they use a low 5px blur size, it's not only on a full-screen size element, but that element is transparent, so all content/layers beneath it need to be flattened and blurred(not sure how much more expensive that is, but I imagine it's cheaper and optimized on single image elements which are fully opaque, possible concern during the opacity transition to real-size image).
  • Another example of poor blur perf and how GPU accel can make a huge difference

    This moves all the rendering of that specific element over to the gpu. This makes the gallery I linked, at ~200 blurred images, sit at 60fps rather than being an unusable blackhole of cycles.

  • Use responsive design with media queries to avoid hitting mobile users if it's a real concern with them

    you could also make the animations and filters conditional on the size of the viewport, only targeting larger devices that most likely would have additional processing power to do both.
    Although, seeing as there is a a large rise of mobile users for the web these days, degrading their experience when your demographic in most cases is going to be fully capable of handling the effect without issue seems like a negative. I know we want our sites to have the widest reach possible, but when you completely discard certain browser support, you're already alienating a certain kind of user. If you run analytics you should have some data on what kind of users you're getting, if you have a large market reach in third-world countries with dated/unsupported browsers or low-end hardware, then you perhaps want to consider avoiding CSS blur as a default, but many projects aren't at a global scale and have a fair idea of their actual audience that CSS blur is probably clear if it's a good default or not for them, for primarily western first world demographics, I think it's a safe bet the concerns are no longer relevant.

  • BrowserBench - MotionMark, a benchmark by the webkit team includes one targeting CSS blur, but results aren't particularly useful. Their usage of blur is at a larger scale than would be used for placeholders suggested here. They animate a large number of elements with opacity overlaps at fullscreen.

I'm not necessarily against this, I just think there's a possibility the gatsby-image component itself can get bloated with each of these additional features. I'd prefer to keep it lightweight and focused

Understandable, this change isn't that large of an addition, most of it's size is from making it configurable. Currently it checks to avoid usage on tracedSVG placeholders, but I'm not sure if that's the right approach if more alternatives were also added.

Is there a good way to extend gatsby-image if bloat is a concern? I wanted to also contribute some other progressive placeholder alternatives like primitive styled SVG, dominant backgroundColor and gradient styled. You can see examples of these from this article, here's the same image I've shared above but with 100 SVG triangles(weighs 8KB or 2KB gzipped + SVGO)

svg_primitive

This can be a nice alternative although much heavier at 100 primitives(see article for lightweight versions with only 10).

What do you mean "specific Img elements" exactly? Again--there's a trade off here. I don't really like adding an option in gatsby-config.js when we don't have to--and we've already experienced some pains with pathPrefix so I'd really encourage we go a different direction here.

I meant if you have a gatsby-image element on a page that you wanted to toggle blur off you could.

The benefit of enabling via global config perhaps isn't that important/useful, you could create a new component that pulls in gatsby-image and applies some generic settings and re-exports it, like shown in the gatsby starter project with the astronaut. It's not too far of a stretch at that point to just include the two extra CSS properties to get the style, which if this wasn't enabled by default or an easy feature toggle in gatsby-config.js, a user might as well discover the same way in the docs.

So treat it similar to backgroundColor? A boolean toggle to apply a default blur(eg with blur size 16px), but allow passing in a value to use a different blur size?


TL;DR:

  • Other alternatives aren't likely to be better, especially for the audience Gatsby targets.
  • The general audience of Gatsby sites are also likely to be using browsers and devices where GPU accelerates greatly what is an expensive CPU effect.
  • In most cases the effect isn't going to cause any actual issues(battery/perf) for the limited time it's in use. In the rare event it does, disable it selectively(or use a lower blur size value, 16px is conservative afaik).
  • If not used as a default, could use as prop with similar behaviour to backgroundColor, else disable default like fadeIn or critical prop.

@KyleAMathews
Copy link
Contributor

I think documenting it would be a good start. I also share concerns about perf. It'd be nice too if you @polarathene could build a sample site w/ blur applied so we could try it directly? Perhaps just fork https://using-gatsby-image.gatsbyjs.org?

@polarathene
Copy link
Contributor Author

polarathene commented Dec 31, 2018

@KyleAMathews Sure, I'll get around to that within the next few days :) Any suggestions on how to profile any meaningful data? There are some online services like WebPageTest, or Chrome's dev tools has the Audits tab which can simulate low speed connections and slow CPU down a bit(4x, but I'm currently on a Intel i5 8400 6 core). I could setup a virtual machine with limited resources too,or with a bit more effort via QEMU/KVM run Android on x86 or ARM? Not sure how I'd profile the perf on Android though. sitespeed.io could also be worth looking into? (In particular BrowserTime looks useful)

The concerns for perf/battery, particularly those of mobile users apparently are/were valid for inlining base64 data URIs? Which is popular for gatsby-image and it will do as an optimization for images less than 10KiB by default if I'm not mistaken? A particular piece of information from the linked article that raised a brow was the usage of base64 apparently adds a 814 byte header?? With enough placeholders, it might make external linking(at least with HTTP/2) a bit of a better option, then again the extra weight/overhead can be worth it to ensure instant preview available, does make my 150 byte webp previews not as small as I thought they were :P

Data URI content must be decoded back into it’s original form by the browser. This operation consumes CPU & battery on mobile devices (browser must decode the image each time a page renders).

Data URI
Android 2.2: 1702ms
Android 4.2: 167ms
iPhone iOS 5: 491ms
iPhone iOS 7: 201ms

Binary Image
Android 2.2: 142ms
Android 4.2: 44ms
iPhone iOS 5: 51ms
iPhone iOS 7: 32ms

@KyleAMathews
Copy link
Contributor

webpagetest.org is great as we can compare the CPU traces directly.

@polarathene
Copy link
Contributor Author

@KyleAMathews I have taken the using-contentful(... oops wrong example project!) and set that up with a blur/noblur test which I ran on localhost and Chrome DevTools Lighthouse Audit.

Blur Audit:
blur_lighthouse-audit.pdf

No Blur Audit:
noblur_lighthouse-audit.pdf

Blur:


Metrics
First Contentful Paint
1.5 s
Speed Index
1.9 s
Time to Interactive
2.2 s
First Meaningful Paint
1.5 s
First CPU Idle
1.6 s
Estimated Input Latency
10 ms

No Blur:

Metrics
First Contentful Paint
1.5 s
Speed Index
1.5 s
Time to Interactive
1.9 s
First Meaningful Paint
1.5 s
First CPU Idle
1.6 s
Estimated Input Latency
10 ms

I'll set it up again with the proper example project this time and upload to a repo. Do I need to host the results of a gatsby build somewhere to use webpagetest.org? Or is the github repo sufficient for someone to reproduce if they're interested in comparing themselves?

I also did run a profile in the Chrome DevTools Performance tab, and didn't see any noticeable perf issues with blur enabled. The Lighthouse audits are with mobile device being simulated with slower connection and 6x CPU slowdown(no idea how accurate that compares), CPU/Memory Power difference can be ignored, it varies, the other values small deltas are likely to vary too due to that, these were single test runs rather than collecting a larger sample.

@DSchau
Copy link
Contributor

DSchau commented Jan 7, 2019

@polarathene it might actually be more helpful to use WebPageTest, specifically with an older, mobile device that likely doesn't have GPU acceleration.

But--also thank you so much for doing the due diligence on this already. Seriously means a lot, and we appreciate it!

@polarathene
Copy link
Contributor Author

@DSchau I know with non-monorepo how to use my forked packages to test with git url in my package.json, but I'm not sure how to set that up for monorepo style like gatsby is using that netlify will also be able to use?

@pieh
Copy link
Contributor

pieh commented Jan 8, 2019

@polarathene do you actually need to fork? Using filter on placeholder can be replicated by passing placeholderStyle prop currently I think:

<Image fluid={someImageData} placeholderStyle={{
        filter: `blur(16px)`,
        transform: `scale(1.04)`,
}}/>

@polarathene
Copy link
Contributor Author

@pieh I guess not in this case, I have a few other PRs in progress where if I'm asked to produce any similar tests might require adjusting my package.json?

I've spent my time today dealing with an Alpine Linux issue with the https://using-gatsby-image.gatsbyjs.org/ example project, libvips 8.7.0 doesn't work well on Alpine when writing certain image/file sizes to /tmp, it got fixed in 8.7.1 but the npm sharp package needs to push out a new build for it.

@polarathene
Copy link
Contributor Author

Sorry for the delay. I have the requested example up on netlify, I have both the original version and the one with blur added as @pieh suggested vs trying to get my PR to be used.

With blur
Without blur

Test conditions:
From: Sydney, Australia - EC2 - Chrome - Emulated Motorola G (gen 1) - 3GFast - Mobile

Hopefully that contains the data you're wanting or you're able to run your own tests on it? I'm not sure why this project was suggested for testing with, as you can see in the filmstrip/video, some images will fade in from solid backgroundColor vs displaying a blur. It's due to the layering order(I have a local branch with changes that also include fixing that layering order, I just haven't got around to finishing and pushing a PR).

Sidenote: using-gatsby-image surprised me. It sources 130MB of image data from unsplash to build?(and build disregards the cache), on my China + VPN connection that took over 11 minutes to retrieve the remote images :(

@polarathene
Copy link
Contributor Author

polarathene commented Jan 13, 2019

I put together an example site on netlify(noblur, blur) to test. That should test the blur on a full screen amount(most perf hit you'd get?).

Mobile - 1 cell wide:
From: Hong Kong, China - Chrome - Emulated Motorola G (gen 1) - 3GFast

Desktop - 3 cells wide:
From: Hong Kong, China - Chrome - 3G

I can provide a link to the repo if anyone is interested? These results should show that the impact seems rather minimal? I'd be interested if you can get results that say otherwise? I think this is the best I can do for reviewers. If there is interest in going forward with this PR I will add improvements as discussed earlier, along with making changes for documentation.

@DSchau @pieh @KyleAMathews @sidharthachatterjee @LekoArts

Sorry for spam, I know you're all very busy, I'm not sure how to go about pinging/requesting for a review(if I'm meant to do it another way please let me know)

@polarathene
Copy link
Contributor Author

polarathene commented Feb 18, 2019

Could some feedback be given please? I have provided what was asked for to progress this PR to being merged or not.

You do not need to read all the prior issue history. @KyleAMathews requested a project for WebPageTest results, as did @DSchau .

My comment before this one, provides what is needed. The project is different from what was requested but more applicable imo, still, the requested project to be tested was also provided one more comment back.

@wardpeet

@wardpeet wardpeet added type: feature or enhancement status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response labels Feb 18, 2019
@wardpeet
Copy link
Contributor

wardpeet commented Feb 18, 2019

I'll ask some perf engineers at google they might have a better insight on this one 👍

from the traces at least it doesn't seem to harm but I can see this being a problem when you have like a gallery with tons of photos. Could we maybe make it opt-in 🤷‍♂️ ?

@polarathene
Copy link
Contributor Author

polarathene commented Feb 18, 2019

I'll ask some perf engineers at google they might have a better insight on this one

Awesome, looking forward to what they have to say! :)

but I can see this being a problem when you have like a gallery with tons of photos.

Like the last linked live example site and webpagetest results? It was 20+ images at a reasonably large size iirc, where you could render many images or a big single one on mobile with the blur effect effectively being fullscreen.

According to profiling data the paint events were painting the entire document height?, though it's not clear if blur was being applied to placeholders far from the viewport bounds. I'm not familiar with how the internal rendering happens.

Could we maybe make it opt-in ?

Yes. See below for discussion on that in this thread.


Summary of discussion (regarding changes and concerns)

I mentioned some alternatives in an earlier comment and and re-iterated it in another comment:

That said, I can modify the PR to activate blur only when the prop is used instead? Or toggle it via an option property in gatsby-config.js?

@DSchau provided some feedback on this

On using a prop to apply:

I'm not necessarily against this, I just think there's a possibility the gatsby-image component itself can get bloated with each of these additional features. I'd prefer to keep it lightweight and focused, but I can see how this is a fair trade-off here for a nicer looking image.

and for the gatsby.config.js approach:

I don't really like adding an option in gatsby-config.js when we don't have to--and we've already experienced some pains with pathPrefix so I'd really encourage we go a different direction here.

and expressed concerns:

I'm not 100% sold it's worth a possible performance regression for some devices.

@pieh shared a similar stance earlier that @LekoArts agreed with:

blur filter is one of the most expensive ones (both on performance and battery life for mobiles) so I'm not sure I like using it by default.

I dig up a lot of information to about the blur filter effect being costly as probably outdated in this comment, which at the end mentions the usage/options behaviour too:

If not used as a default, perhaps a prop with similar behaviour to backgroundColor, else disable the default like the components fadeIn or critical prop allows.

And that arguably, if battery/perf concerns were actually valid, then why is base64 support default?(which had notable perf difference):

Data URI content must be decoded back into it’s original form by the browser. This operation consumes CPU & battery on mobile devices (browser must decode the image each time a page renders).


You're more than likely wanting to globally opt-in/out and possibly opt-in/out for select gatsby-image elements, or adjust the blur size(globally, or per image where a different value suits better or lower if there was perf issues but retaining some blur was still better than the pixelated blur up example above).

@polarathene
Copy link
Contributor Author

@wardpeet Did you manage to hear back from anyone at Google?

from the traces at least it doesn't seem to harm but I can see this being a problem when you have like a gallery with tons of photos.

Let me know what the definition of "a gallery with tons of photos" is and I'd be happy to put together another demo project or update my existing one(20+ large pics), if that'll help at all.

Would love to have progress on a decision with this PR. So far there is no indication of any performance issues or noticeable battery drain correct?

Could we maybe make it opt-in?

It's a much better aesthetic(link to prior comment with comparison pics and quotes/links of gatsby docs citing blur-up effect like Medium and others that have actual blur effect applied). Wouldn't it make sense to have as a default with opt-out if there are no major negatives anymore?

I'm happy to do what I can to get the metrics you want, but I'm unable to find anything that shows the effect to be as bad as it was over half a decade ago. Surely by having it as a default, should anyone in the community find out otherwise, it'd be reported here and could be easily reverted to opt-in?


There is a related PR(allows larger placeholder image like facebook uses, allows webp(firefox/chrome both support) for placeholder with proper layer fallback to bgcolor) that I'd love to get feedback on as well if anyone has the time.

@wardpeet
Copy link
Contributor

wardpeet commented Feb 27, 2019

Sadly I have to block this PR from being merged.

I had a chat with @paullewis & @addyosmani about this. This is a summary of at least what I understood. filter: blur is not as bad as it used to be if a GPU is present but still something to consider harmful. For a few small images, it's not a big issue as the work needs to be done is small by the GPU. In the article Animating a Blur the process of filter: blur on images is being explained.

In the article they talk about Convolution filters:

Convolution filters are fairly expensive as for every output pixel a number of input pixels have to be considered. The bigger the image or the bigger the blur radius, the more costly the effect is.

blur 1000 x 1000 at 16px (blur size) you're convolving a lot of pixels
whereas 10 x 10 at 2px (blur size) is fine.

from the traces at least it doesn't seem to harm but I can see this being a problem when you have like a gallery with tons of photos.

After learning about these filter expenses I looked at your traces again and saw you were running WPT on emulated mode and not GPU info was collected. So I ran it again (thanks for the demo pages!)

current behaviour no filter:
https://www.webpagetest.org/result/190227_ZZ_180bdd1c27a2a8a87c3cde17aa10e222/
image

enhanced blur with filter:
https://www.webpagetest.org/result/190227_NP_438db9d4aee48f5747e139659bd7a565/
image

If you look at the trace you can see that the GPU is spiking on the mobile phone. It even spikes on my surface book 2. So it's definitely an opt-in. I think I have to agree with @pieh because it's so heavy we shouldn't make people life easy to add this kind of behaviour.

blur filter is one of the most expensive ones (both on performance and battery life for mobiles) so I'm not sure I like using it by default. People can opt-in to use it currently by using placeholderStyle, so maybe alternative would be writing docs for it?

For your use case, you might want to create a HOC that actually sets placeholderStyle on all your images with the blur filter. Or are there other issues you're running into?

@m-allanson
Copy link
Contributor

Thanks for your work on this @polarathene! To follow up on @wardpeet's comment, I think documenting how to do this manually is a great next step.

That means we'll have a 'recommended' approach, which will allow people to experiment with the feature on their sites. If this turns out to be really popular and our performance concerns prove to have minimal real-world impact, then we could take another look at bringing this directly into gatsby-image.

Approaching this problem from another direction - maybe we can improve the default look of the thumbnails that are created by sharp? This would move the processing cost over to the build stage instead of individual clients, which makes it much easier to measure the pros and cons.

@wardpeet wardpeet added status: awaiting author response Additional information has been requested from the author and removed status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response labels Feb 27, 2019
@KyleAMathews
Copy link
Contributor

One thing we could look at is what happens if we increase the size of the small placeholders. It's 20px which might be too small sometimes (leading to excessive pixelization).

@polarathene
Copy link
Contributor Author

@wardpeet

After learning about these filter expenses I looked at your traces again and saw you were running WPT on emulated mode and not GPU info was collected. So I ran it again (thanks for the demo pages!)

The first half is heavier GPU activity yes, but that is applying the blur filter to all 20+ images in that gallery/layer. IIRC the profiler showed the entire height being rendered, not just the viewport, perhaps there is an optimization around that like a virtualized list. <1 sec of GPU time spent on blurring several thousand pixels height by n width of device/viewport. (1k x 5k for your mobile test). Although such an optimization might be a bit misleading if the blur is toggled/reapplied based on visibility.

The 2nd half is animation of opacity. According to the article you linked(thanks for that), it might be reapplying blur during this transition, when it should ideally only be applying opacity to the rasterized blurred texture. I'll look into this, afaik there shouldn't be any need for increased GPU activity here.

If you look at the trace you can see that the GPU is spiking on the mobile phone. It even spikes on my surface book 2.

The GPU is being used effectively to compute the blur for all images and compute it as fast as possible, which I'd assume a user wants. There is not much point of a GPU not utilizing it's full compute resources and delaying the processing, unless it's due to some form of power save mode to conserve battery and the processing delay is acceptable UX.

Did you notice a noticeable/measurable dip in battery life on your device? Was there a noticeable perf hit delaying the rest of the site render visually or similar issues?(I don't think this effect was blocking anything?)

I think I have to agree with @pieh because it's so heavy we shouldn't make people life easy to add this kind of behaviour.

Yes... a taxing demo for the blur filter usage as asked for is a bit heavier than desirable, but not exactly a proper representation of a common Gatsby site?

Do note that it's a one off computation during page load. Blur isn't being reapplied (probably will be if a layout is triggered again, but that's less likely on mobile devices where the concern is.), and is only used for placeholder images, so subsequent visits where the proper image is cached shouldn't be causing that initial blur computation either?(I haven't checked, but imagine the lazy-load approach allows for skipping the blur effect when placeholder isn't needed).

In the article Animating a Blur the process of filter: blur on images is being explained.

In the context of that article, it's about how blur was a perf problem from animating the blur size value. This feature is not animating the blur size, it applies blur once to placeholder images.

The article indicates animating blur with opacity can be an issue because the attempt to optimize by cross-fading several blur instances(instead of animating a single blur value), reapplied the blur per frame due to the element being blurred and promoted, instead they fix this with a wrapper element: "If the element is blurred but not promoted, the blur is rasterized to the nearest parent texture instead... The blurred image is now the texture of the parent element and can be re-used for all future frames.", this caches the blurred result.

blur 1000 x 1000 at 16px (blur size) you're convolving a lot of pixels

whereas 10 x 10 at 2px (blur size) is fine.

This is parallelized on the GPU distributing the work across the cores. Yes a larger blur size is more intensive to compute, I've linked about this in a past comment several times, it was often due to much larger blur size values such as 64px+.

I felt 16px was an alright default, but if you'd rather a 8px or lower default blur size, that can be done. The code allows for a user to adjust per image or global default should they want or need to adjust for perf/aesthetic reasons.

The size of the image was less of an issue vs blur size value which increases the amount of pixel area to compare/compute, which afaik the larger the value, the more overlap for a pixel there is being compared by it's neighbours(like this)

For your use case, you might want to create a HOC that actually sets placeholderStyle on all your images with the blur filter. Or are there other issues you're running into?

I don't have much of a choice if the PR is to be rejected :P

I can take this approach, it's not a major issue for me personally, but I figured it'd be better to contribute into gatsby-image as it's closely tied to that and everyone else could benefit from it by default(along with the docs stating you had everything to get a Medium like blur-up effect, but I've noticed it can be quite ugly on large images such as hero content without proper blur applied)

@wardpeet
Copy link
Contributor

wardpeet commented Mar 4, 2019

In the context of that article, it's about how blur was a perf problem from animating the blur size value. This feature is not animating the blur size, it applies blur once to placeholder images.

They talk about animating blur indeed but they explain how blur is applied on a single image and it's more taxing on animating it than a single value. That doesn't mean it's not "expensive" for an initial blur. A good video about why you should be careful about CPU/GPU speed and it's not getting better. https://www.youtube.com/watch?v=4bZvq3nodf4

Yes... a taxing demo for the blur filter usage as asked for is a bit heavier than desirable, but not exactly a proper representation of a common Gatsby site?

That's the hard part, what is a common Gatsby site? We're a "framework" to build websites so we should be fast by default and this potentially adds slowness and extra work to all users which is what we don't want.

Did you notice a noticeable/measurable dip in battery life on your device? Was there a noticeable perf hit delaying the rest of the site render visually or similar issues?(I don't think this effect was blocking anything?)

It depends on how you look at it. The more work you do, the more battery is being used but I can say that while you're not animating blur it's not an issue.

@KyleAMathews: One thing we could look at is what happens if we increase the size of the small placeholders. It's 20px which might be too small sometimes (leading to excessive pixelization).

We probably want to get #10944 merged and see if we can improve blur with making our placeholder images larger. For the css filter part I would suggest going for filter:blur as placeholderStyle.

@LekoArts
Copy link
Contributor

LekoArts commented Mar 4, 2019

btw, there’s is currently this bug:
#12254

So the blur calculation would happen every time

@polarathene
Copy link
Contributor Author

@m-allanson

I think documenting how to do this manually is a great next step.

Sure if that's the decision, I'll look into updating the docs to mention an example of the blur CSS used here, along with a note/warning about performance considerations and link to this PR? (It's become a pretty good resource dump on the topic ha)

If this turns out to be really popular and our performance concerns prove to have minimal real-world impact, then we could take another look at bringing this directly into gatsby-image.

Ok, should I leave this PR open in case of that as a reminder; or should this PR be rejected and closed? (open a new PR in future, if using blur css is popular and without perf problems)

Alternatively, what is the stance of merging the feature as opt-in instead? It becomes easier to adopt(not that asking each user interested to keep their own copy of the CSS and/or HOC is that much to ask for, should be rather low maintenance wise).

Approaching this problem from another direction - maybe we can improve the default look of the thumbnails that are created by sharp? This would move the processing cost over to the build stage instead of individual clients, which makes it much easier to measure the pros and cons.

There's not much that can be done. Look at the preview images in my earlier comment, the placeholders are reduced to to 20 x 20 px previews for the current blur up effect(where the pixels are stretched/interpolated to the intended width/height container of the real image).

Increasing pixel density will tend to have a negative visual imo, you can see in the shared previews from earlier that the doubled resolution(4x the pixel density) doesn't look much better. It does when complimented with the blur filter however. This only gets worse, the larger the image is.

You could adapt the placeholder dimensions based on the intended display width/height I guess, but that makes them less likely to be embedded into the initial html payload(although base64 inlining is also a perf/battery issue in it's own, like I linked earlier) due to increased data size. You end up with stretched, low quality/blocky images that are somewhat decipherable but bad UX imo.

Blur effect on the other hand, doesn't have the pixel source limitation, it can take the new dimensions and blend the pixel input data to make use of the added pixel density, which as my preview images show, are much nicer aesthetically.

Medium is a big user of this effect, and if I'm not mistaken have plenty of users on mobile devices, with each article having a large cover image and potentially several other large images throughout the article. They've not removed the blur feature(they do handle it via canvas instead though). I imagine, since they're for-profit, they'd be very concerned about losing their user base to competitors? Choice for canvas might be due to it being more suitable at the time of implementation, afaik CSS filter is more optimal, provided the GPU isn't being used to reapply the blur unnecessarily.

I completely understand the concerns, I just don't see them being all that valid anymore. The data we have so far, seems to support that fact? It's been available in the spec since 2012, and majority of articles about perf issues was 2013-2016 (sometimes due to browser support, hardware, or incorrect usage). These perf issues were noticeable(690ms vs 63ms paint times based on CPU or GPU handling the blur), but I don't see them being that noticeable today?


Out of curiousity, at what point would such a feature be considered safe to integrate/support?

AFAIK, the current decision for rejecting this PR (or accepting it without requiring opt-in) is the comparison @wardpeet last shared, where a Moto G4(budget android phone released mid 2016) rendered a 1080px wide, 5536px tall layer, taking less than 1 second to compute the blur on the GPU for initial load (slightly heavier GPU activity for the opacity transition too), but otherwise should not utilize the GPU any further.

Is ~500ms of GPU time on a budget android phone close to 3 years old with a taxing example site for the feature proposed, that detrimental to battery life or any other area of the site performance or user experience?


Just to be clear, in case it comes across the wrong way via text; I'm happy with whatever direction the team decides on(although, it'd be nice to at least see this accepted with opt-in instead).

The reasoning for still being against this as a new default with the given data is just a tad confusing. It's clearly not exhibiting the major issues cited in articles years ago - when used for the purpose proposed by this PR. It's always going to have some overhead over not having blur applied.

@polarathene
Copy link
Contributor Author

They talk about animating blur indeed but they explain how blur is applied on a single image and it's more taxing on animating it than a single value.

The article introduces an expensive blur size animation.

They then tackle that problem with 4 images each with a different blur size applied, cross-fading through them to achieve the same blur size transition effect. Yes, it turns out to be more taxing due to the elements being both promoted and blurred.

The final iteration, resolves that problem by promoting a parent wrapper element to the blurred children elements. This turns out to perform much better.

That doesn't mean it's not "expensive" for an initial blur.

The initial blur does take a fair amount of compute time to apply in my demo page, I don't disagree with that. But as it's intended to only be applied once at page load, it's rather inexpensive, it's not blocking anything else afaik.

A good video about why you should be careful about CPU/GPU speed and it's not getting better.

Thanks, I'll give that a watch! 😃

That's the hard part, what is a common Gatsby site? We're a "framework" to build websites so we should be fast by default and this potentially adds slowness and extra work to all users which is what we don't want.

I'd assume the majority of Gatsby sites that use gatsby-image with base64 placeholders for such an effect aren't doing full page galleries like the demo page I shared for testing this PR with? If the site is not presenting a 1k x 5k px layer on mobile to render blur on the GPU with, then the actual perf hit would be much smaller no?(I could look into the impact on other gatsby sites?)

For those that do, and feel that the perf impact is worse than the aesthetic gain for their visually focused sites, they'd be able to opt-out or reduce the blur size? (I'd be happy to push out some more variations as netlify makes this simple with branches, for example how much a difference 2px and 8px blur sizes make).

Fast by default is a fair argument though. It doesn't appear to be a big tradeoff but there is some overhead, if that's worth avoiding by default and requiring users to opt-in, I understand that.

It depends on how you look at it. The more work you do, the more battery is being used but I can say that while you're not animating blur it's not an issue.

Is it common to see users animate gatsby-image placeholders elements in their sites? In regards to the opacity transition, that looks like it could possibly be avoided(seems to recalculate blur per image rather than layer, but shouldn't need to recalculate the blur in the first place).

We probably want to get #10944 merged and see if we can improve blur with making our placeholder images larger. For the css filter part I would suggest going for filter:blur as placeholderStyle.

I'm not sure what you're suggesting about the css filter part?

I didn't get time to reply with that PR link earlier :) I was going to link to my earlier comment which shows some previews at larger placeholder size(double the resolution, 4x the pixels, with and without/blur). As it's my PR, I'm also happy to put together a demo of it on Netlify if anyone would like.

If you meant improving the blur without the CSS blur filter, but just the increased pixels... well the linked comment should also clarify that doesn't really improve things, I elaborated more on that in my reply to @m-allanson . Facebook apparently uses placeholders at 42px wide(linked to source earlier in this thread).

@m-allanson
Copy link
Contributor

Sure if that's the decision, I'll look into updating the docs to mention an example of the blur CSS used here, along with a note/warning about performance considerations and link to this PR? (It's become a pretty good resource dump on the topic ha)

Yep that sounds excellent, and yeah I agree this thread has become a great set of resources :) This would be great to document in the gatsby-image readme.

Updating one of the images in the using-gatsby-image example site could be helpful too.

And if you had more time, maybe checking out other docs that mention gatsby-image, and pointing them to a new section in the gatsby-image readme would be another good pointer.

Ok, should I leave this PR open in case of that as a reminder; or should this PR be rejected and closed? (open a new PR in future, if using blur css is popular and without perf problems)

Let's close this PR for now, we can re-open when needed.

Alternatively, what is the stance of merging the feature as opt-in instead? It becomes easier to adopt(not that asking each user interested to keep their own copy of the CSS and/or HOC is that much to ask for, should be rather low maintenance wise).

On the subject of opt-in vs documentation... Gatsby's image component doesn't need to solve all problems for all people, rather it should be easy to use and easy to extend.

Ultimately this is a pretty subjective decision, particularly in a case like this where there's benefits to either approach. Given that, I'd err on the side of caution and prefer additional documentation over additional code.

Out of curiousity, at what point would such a feature be considered safe to integrate/support?

There's not really a clear line here - maybe this would be a case of encouraging people to use this pattern (via documentation) and then looking for feedback. Did it work well? What were the cases where it didn't?

Is ~500ms of GPU time on a budget android phone close to 3 years old with a taxing example site for the feature proposed, that detrimental to battery life or any other area of the site performance or user experience?

I agree with you on battery - there are so many factors that go into the battery life of a mobile device that I don't think we can clearly measure what impact this change would make. My gut says it wouldn't be that significant.

However I think there are cases where using this feature could cause juddery animation or other side effects - e.g. if you have an older device, are using large images, have many images, or if you want to simultaneously run other animations on your page.

The reasoning for still being against this as a new default with the given data is just a tad confusing.

Adding this could have a negative impact in some cases. There's no clear way to measure how often or how bad that impact could be, as the code will be used to build different sites that will run on different devices and browsers.

This change can be done in userland using the placeholderStyle prop so adding it as a prop doesn't enable any impossible functionality, and it's not much upfront (or ongoing) effort to document that pattern.

While adding it as an opt-in feature is a reasonably small change, it's adding another prop to the already lengthy list of props for gatsby-image, and it means we're committed to maintaining, testing and documenting that feature for a long time.

I hope that answers your questions? This decision doesn't need to be forever-final, but I think it's the best approach for right now. Maybe in the future we'll revisit this discussion, I'll wonder what nonsense I was going on about, and all Gatsby sites will use a CSS filter-based blur up. But let's start with the documentation approach.

And thank you for opening the PR, it has felt very useful to properly go through the pros and cons here! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants