[WIP] Add client-side media processing #64278
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
swissspidy
force-pushed
the
add/client-side-media-v1
branch
3 times, most recently
from
9b31a7d
to
1affded
Compare
|
Size Change: +55.3 kB (+3.12%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
swissspidy
force-pushed
the
add/client-side-media-v1
branch
6 times, most recently
from
15e88c3
to
903c0d2
Compare
swissspidy
changed the title
swissspidy
force-pushed
the
add/client-side-media-v1
branch
from
903c0d2
to
0e754c0
Compare
youknowriad
reviewed
Aug 9, 2024
There was a problem hiding this comment.
Huge PR thanks for starting this work. I know it's early and I was not able to review this PR properly, it's too big. But I left some small initial comments.
I was wondering how we can split this PR into smaller chunks. For instance, there are a lot of things that seems like enhancement / code quality / types for existing packages. Maybe we can extract this kind of things easily. Obviously it's a small part of the PR but then we can think about the rest.
| @@ -0,0 +1,86 @@ | |||
| # `@wordpress/mime` | |||
|
|
|||
| A set of mime type-related helper utilities, wrapping [`mime/lite`](https://www.npmjs.com/package/mime). | |||
There was a problem hiding this comment.
Do we really need a wrapper package, why not use mime directly to avoid a new public API / package and maintenance burden.
Mostly asking because the content of the package seems very small.
There was a problem hiding this comment.
Sure I can reuse it directly 👍
|
|
||
| return $response; | ||
| } | ||
| } |
There was a problem hiding this comment.
Do you think all the endpoint changes can be extracted to a separate PR to ease reviews... We can ping folks working on the REST API to help.
There was a problem hiding this comment.
Sure yeah that's possible. It won't be useful on its own, but makes reviewing easier for sure.
| $image_size_threshold = (int) apply_filters( 'big_image_size_threshold', 2560, array( 0, 0 ), '', 0 ); // phpcs:ignore WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound | ||
|
|
||
| $settings['__experimentalAvailableImageSizes'] = gutenberg_get_all_image_sizes(); | ||
| $settings['__experimentalBigImageSizeThreshold'] = $image_size_threshold; |
There was a problem hiding this comment.
Can you expand a bit on these settings?
I have two thoughs:
- We're trying to move away slowly from block editor settings passed from the server to the client and instead relying more on the endpoints (The base endpoint has been used for similar things)
There was a problem hiding this comment.
I can look into moving those to the REST API endpoint.
| @@ -0,0 +1,138 @@ | |||
| /** | |||
There was a problem hiding this comment.
I see that you're using camelCase for file names, we tend to use kebab-case instead.
youknowriad
added
[Type] Enhancement
|
Anything specific you'd like some visual input on? As a couple of quick tests, I saw very little user-facing changes in this PR. What should we be on the lookout for? |
|
Note that this PR is still in early stages. The one single user-facing change is in the image block's Advanced panel, where you can compress an existing image. It looks a bit like this: optimize-existing-media.mov |
|
Nice, thanks. We've some past designs by @jameskoster that are likely relevant for when that piece is ready to be polished: 
|
|
Yes I am aware of those (I am also in that thread), so I suppose they're still up-to-date? I have partially implemented it in my plugin, but it works best for multiple attachments (e.g. in a gallery block) and not a single image IMHO (a single image block doesn't have multiple "attachments"). Also, the "Compress original" option though, it has downsides. Anyway, I'll probably iterate on that as part of https://github.com/swissspidy/media-experiments first and remove it from this initial PR for now. So it can be added in a follow-up PR. |
|
The mockups are up-to-date, yes. I agree the current status is optimized for multiple images, but the same pieces can easily be tweaked to work on a per-image basis, happy to help, feel free to ping @WordPress/gutenberg-design if it becomes more urgent. |
|
The only design adjustment I'd consider is to use the 
|
swissspidy
changed the title
swissspidy
changed the title
This was referenced Aug 20, 2024
ntsekouras
reviewed
Aug 31, 2024
| ) | ||
| : sprintf( | ||
| /* translators: %s: file size increase in percent. */ | ||
| __( 'The new version is <b>%s bigger</b> :(' ), |
There was a problem hiding this comment.
I was testing the PR and this happened to me (33% bigger in 4MB image). When this can happen? It feels weird to click an optimize button and end up with a bigger file.
Can we handle this differently? At least in the level of wording - ex: see if can be optimized.
There was a problem hiding this comment.
That's impossible to say without knowing the image and the settings used. In this PR things like image quality or preferred file format aren't configurable yet either, so depending on the source image a size increase is definitely possible.
I'd suggest trying it again with https://github.com/swissspidy/media-experiments and opening an issue there if it happens, ideally uploading the unmodified original image somewhere too for inspection.
ntsekouras
reviewed
Aug 31, 2024
ntsekouras
reviewed
Aug 31, 2024
| /** | ||
| * An isolated orchestrator of store registrations. | ||
| */ | ||
| export interface WPDataRegistry { |
There was a problem hiding this comment.
Why we need both an interface and a typedef?
There was a problem hiding this comment.
IIRC I removed the typedef in favor of the interface here
ntsekouras
reviewed
Aug 31, 2024
| * | ||
| * @param file File object. | ||
| */ | ||
| export function validateFileSize( file: File ) { |
There was a problem hiding this comment.
Can we make this util to get maxUploadFileSize as a param to avoid the store subscription?
Same question for validateMimeType function below.
|
Thanks for starting this! I left just a few comments and will catch up with your other related PRs. It's good that you've started splitting this in smaller chunks to review them easier. |
|
Thanks @ntsekouras, much appreciated! Once the smaller PRs are reviewed and merged I'll rebase this one here to incorporate all the latest changes from the other repo as well. There's been some great progress all over. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WORK IN PROGRESS
What?
See #61447 for full context.
This PR lays the foundation for client-side media processing in Gutenberg—meaning we can do things like thumbnail generation, cropping, compression, and format conversion (transcoding) all in the browser. It is heavily inspired by my https://github.com/swissspidy/media-experiments project.
As mentioned in the roadmap at #61447, this first iteration includes the following features:
TODO (for this PR)
block-editor<->upload-mediaintegration a bit nicerfetchbe available in unit tests, either in jsdom or native environment 🤔TODO (for later)
Why?
In short:
Check out #61447 and https://github.com/swissspidy/media-experiments for full context.
How?
In a nutshell:
New packages:
@wordpress/media-utils- is enhanced with new functions, TypeScript, and so on@wordpress/upload-media- is a new upload queue system powered by@wordpress/data@wordpress/block-editor- the exposedmediaUploadfunction now adds files to the queue rather than uploading straight to the server.@wordpress/vips- new dedicated package for interacting with wasm-vips library for all the image processing in a web worker@wordpress/mime- new dedicated package for dealing with mime types (ext -> mime type and mime type -> ext conversion)Implementation:
wp/v2/media/<id>/sideloadTesting Instructions
There is existing e2e test coverage already for most media-related functionality. However, I plan on porting over more tests from https://github.com/swissspidy/media-experiments which has more extensive coverage, of course also for the new UI elements.
Things to test:
Testing Instructions for Keyboard
The only change relevant for keyboard is the UI for compressing an existing image. Everything else is behind the scenes.
Screenshots or screencast