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(media): Blurhash #1381

Merged
merged 26 commits into from Aug 17, 2019
Merged

feat(media): Blurhash #1381

merged 26 commits into from Aug 17, 2019

Conversation

sorin-davidoi
Copy link
Contributor

@sorin-davidoi sorin-davidoi commented Aug 8, 2019

Todo:

  • Handle medias without blurhash
  • Look into performance (maybe cache blurhash -> base64 computation)
  • Implement for videos
  • Fix integration tests
  • Option to disable usage

image

Closes #1179.

const pixels = decodeBlurHash(blurhash, 320, 320)

if (pixels) {
canvas = canvas || document.createElement('canvas')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mastodon renders the canvases directly but I though that it's too much overhead to create and destroy that many canvases so I went with this approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Profile sample with 6x CPU throttling.
image

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if toBlob() with a Blob URL may actually be more performant here than a data URL. I'll play around with it. :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so the problem with toBlob() is it's asynchronous, meaning that there might be a flash of unhidden sensistive media unless we're careful about doing the blurhashing in advance. I might keep noodling on this after merging the PR.

@@ -48,6 +48,7 @@
"@webcomponents/custom-elements": "^1.2.4",
"babel-loader": "^8.0.6",
"babel-plugin-transform-react-remove-prop-types": "^0.4.24",
"blurhash": "^1.1.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could try https://github.com/fpapado/blurhash-rust-wasm if performance turns out to be an issue.

import { decode as decodeBlurHash } from 'blurhash'
import { mark, stop } from './marks'

const RESOLUTION = 32
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could maybe lower this based on some heuristics (e.g. navigator.hardwareConcurrency)? Setting it to 1 will basically extract the average color, so maybe we can do that initially and repeat with 32 inside a requestIdleCallback?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the limiting factor is for blurhash, but yeah for now a hard-coded value is probably fine.

@sorin-davidoi sorin-davidoi changed the title [WIP] feat(media): Blurhash feat(media): Blurhash Aug 9, 2019
@nolanlawson
Copy link
Owner

Thanks very much for this PR!! I made some minor refactoring changes. At this point, my only major concerns are:

  1. This doesn't work with "show large inline images." Although honestly maybe it's time to just remove that option; I'm not sure how many people are using it, given that @sgenoud's new image system is so much better than the old one.
  2. In the settings, "Use blurhash for previews" is now the only option that is checked by default. Up to now I have tried to prefer "false by default" to reduce cognitive load on users (i.e. so they can remember what the defaults are). I admit though that it's hard to think of a negation for this feature that wouldn't be awkward. ("Hide blurhash"?)
  3. Yeah the performance of blurhashing is a bit concerning, especially when I turn on 6x slowdown in Chrome (up to 100ms, dominates the time spent rendering a status overall). I think long term though, we could move toward a webworker-based solution or toBlob() solution, but that would probably require doing some initial asynchronous work before rendering (e.g. in VirtualListLazyItem.html). I've been meaning to do that for some other stuff anyway. But that can wait for a future PR.

For now I'm mostly concerned about # 1; maybe it's time for me to retire the "show large inline images system." Or we could implement blurhash for it.

@nolanlawson
Copy link
Owner

Created a poll to see if anyone is using this setting: https://mastodon.technology/@pinafore/102593964013067092

@sorin-davidoi
Copy link
Contributor Author

Pushed a tentative fix for the overflow issue when "show large inline images" is enabled.

image

@nolanlawson
Copy link
Owner

Awesome! In that case I'd say remaining tasks are:

  • Negate the setting somehow... maybe "Show a plain gray color for sensitive media" ?
  • Fix the performance issues... maybe indeed blurhash-rust-wasm is the best fix here

@sorin-davidoi
Copy link
Contributor Author

Can't seem to get blurhash-rust-wasm to play along (getting Error: Cannot find module 'blurhash-wasm').

@sorin-davidoi
Copy link
Contributor Author

Pushed a commit where the computations are moved to a Web Worker:

  • if OffscreenCanvas is supported the main thread sends the encoded string and gets back an URL
  • otherwise it sends the encoded string and gets back an ImageData, which it then renders using the canvas

Some things still need to be addresses though:

  • the Web Worker is created lazily (when the first blurhash needs to be computed) - this introduces some delay, we should call init as soon as we know that blurhashes are not disabled (when the store is created?)
  • some caching for URLs should probably be added, especially since we currently don't ever call URL.revokeObjectURL

@@ -141,6 +166,7 @@
},
data: () => ({
oneTransparentPixel: ONE_TRANSPARENT_PIXEL,
decodedBlurhash: ONE_TRANSPARENT_PIXEL,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ONE_TRANSPARENT_PIXEL is used while the async computation is pending.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good workaround!

@@ -0,0 +1,49 @@
import BlurhashWorker from 'worker-loader!../_workers/blurhash' // eslint-disable-line
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get worker-loader working when added to client.config.js for some reason. Standard complains about the inline use of the loader, disabled it for now.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine; I don't really care whether the worker-loader is referenced in the Webpack config or in the import.

@sorin-davidoi
Copy link
Contributor Author

Added caching, but we are still leaking memory due to never calling revokeObjectURL - not sure when would be the best time to call it.

@sorin-davidoi
Copy link
Contributor Author

There are still some overflow issues caused by padding-bottom: 56.25%:

image

@nolanlawson
Copy link
Owner

Fantastic work, this looks great to me. I only have some minor issues remaining:

  • The eye icon and "sensitive media" text are a bit hard to read; I think the font should be lighter when blurhash is enabled
  • It would be nice to do the worker operations at the same time that the content is fetched asynchronously from IDB so that there isn't a "pop" when the color changes from gray to the blurhash (i.e. delay rendering the toot until the blurhash is ready... it's a tradeoff I'd prefer to make in favor of less visually jarring rendering).

Both of these things can be fixed later, though; overall I think this is a fantastic PR and we should just get it in master and fix the minor things later. Thanks for the excellent work!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement blurhash
2 participants