Media: Improve average color handling & performance

swissspidy commented 3 years ago

Feature Description

In a few places we use colorthief to get the average/base color of an image or video poster:

When dropping media elements (IDK why it's done here really 🤔 )
In the media library, using useAverageColor, as soon as images are loaded
In the Size & Position design panel, when clicking on the "Set as background" button

This average color is used for:

for skeleton pages in the carousel, to determine the page color based on the background element's average color
for frontend output, to determine background color based on the background element's average color

Some thoughts based on that:

The useAverageColor usage in the media library is extremely wasteful. Right when loading the editor, all images will be run through colorthief. When you perform a search or any other action, they will be run through again
...This will take a loooong time
At the very least this could happen in a requestIdleCallback call instead
There's no reason why this calculation happens for all media items on load, even when they will most likely never be inserted into the story.
There's no reason why this calculation needs to happen during drag & drop.
For frontend output, this background color could be used for all images as a fallback. That would be a nice UX enhancement.
This calculation could simply happen when first inserting a media element onto the canvas. Just once.
The media average color could be generated during upload and then stored permanently in the database. It could be backfilled for existing media items.

Alternatives Considered

Additional Context

swissspidy commented 3 years ago

Pinging @choumx @miina for thoughts

dreamofabear commented 3 years ago

Nice catch. IMO even the background color computation could be done with requestIdleCallback. I'll let Prometheus folks chime in on the original rationale.

miina commented 3 years ago

When dropping media elements (IDK why it's done here really 🤔 )

This is happening since media elements can be inserted via drag and drop, so this is basically for insertion of a media element. It should happen only if the color hasn't been calculated previously already, so it's intended to happen only once.

The useAverageColor usage in the media library is extremely wasteful.

I believe the reasoning here was to speed up the insertion of a media element instead of taking extra time for the calculation while inserting. That said, calculating it just in case for all the media elements when loading the library is wasteful instead.

For frontend output, this background color could be used for all images as a fallback. That would be a nice UX enhancement.

Hm, interesting idea. So when saving output, we would use the background media average color for everything, maybe in this case we'd only need to calculate the average color when setting background media?

swissspidy commented 3 years ago

Hmm that still doesn't explain why that calculation happens so early, when it can be easily done after insertion. Since it's only currently used for the carousel and frontend output.

So when saving output, we would use the background media average color for everything, maybe in this case we'd only need to calculate the average color when setting background media?

No, what I meant is calculating the average color for every image (& not just bg images), and then do img { background-color: <avg color>; for every image to have some colors during load time.

We're already calculating the avg color any way, so might as well make use of it where reasonable.

miina commented 3 years ago

Hmm that still doesn't explain why that calculation happens so early, when it can be easily done after insertion.

True, it would create a new history entry then though. If the user would undo once, visibly nothing would happen but the calculated color would be removed from the element. Perhaps we can start the color calculation when hovering over the media item in the library. Not sure how much time it takes approx per image, to know if it'd be ready in most cases for insertion on time.

swissspidy commented 3 years ago

Can updating an element's resource be done somehow without affecting history?

If not, then calculating on upload + backfilling existing items + storing the color in the database would be preferable over things like hover-detection

miina commented 3 years ago

Can updating an element's resource be done somehow without affecting history?

When the element object changes, a new history entry is created, so unless we store the calculated color outside of the element, it would currently create a new history entry. Not sure if there's a simple way to adjust that. In theory, before creating a new history entry, we could do an additional manual check for change without considering the average color. But this might get a bit hacky + start doing additional comparisons with every change.

Would need to look into this more to see if there's a reasonable way to ignore the average color change.

swissspidy commented 3 years ago

Thanks for freshing up my memory on history entries! I was also asking because we use actions like updateElementsByResourceId quite heavily during media upload or when optimizing/trimming existing videos. Sounds like we probably pollute the history quite a bit there 🤔

swissspidy commented 3 years ago

Let's go with:

Calculate during upload
Lazily backfill for existing items in the library
Use for img background on frontend output in general

This will help to greatly reduce complexity and improve performance.

swissspidy commented 3 years ago

Assigning solely to WP pod because this will involve some PHP changes

spacedmonkey commented 3 years ago

@swissspidy Do you know if it would be possible to add base color to 3P media REST API?

Seems like Unsplash (docs) has this field in it's REST API.

spacedmonkey commented 3 years ago

Lazily backfill for existing items in the library

I have been looking into this. Currently the useAverageColor hook, takes a ref as a param. This ref, points to a image tag that points to either the image or poster image depending resource type. Using useLayoutEffect and the ref, it then adds a event listen when the image is loaded. Once loaded, the refs is passed ColorThief to get the color.

useAverageColor currently relies on the image to be loaded to get the color. Unless I am missing something. When backfilling colors, we will have to generate and force to load, image tags for each media. I would expect the backfill to work similar to generateMissingPosters or backfillHasAudio. Is there a better way to do this? Am I missing something.

CC @miina for her thoughts.

swissspidy commented 3 years ago

Do you know if it would be possible to add base color to 3P media REST API? Seems like Unsplash (docs) has this field in its REST API.

Great question! I haven't thought about 3P media in this context at all so far, as I somehow assumed that was already provided.

I'd have to look into this in detail as the other providers don't provide this information.

So I suppose we could only easily provide it for Unsplash images.

That means for other 3p media we'd need to figure out some other way to get the average color within the editor upon insertion.

Regarding useAverageColor I'd simply not use it. We can directly use getMediaBaseColor instead or create a new function that works more similar to the existing backfill logic.

I would expect the backfill to work similar to generateMissingPosters or backfillHasAudio

generateMissingPosters (via uploadVideoPoster and eventually uploadVideoFrame also just loads a video to extract a poster image. Loading an image to extract the average color is not much different.

GoogleForCreators / web-stories-wp