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

Media: Thumbnails #21615

Merged
merged 10 commits into from
Sep 25, 2023
Merged

Media: Thumbnails #21615

merged 10 commits into from
Sep 25, 2023

Conversation

kean
Copy link
Contributor

@kean kean commented Sep 20, 2023

Part of #21457

I made a couple of misguided changes in #21562 when it comes to thumbnails. This PR reverts some of them but keeps the performance improvements and adds more. These changes are hard to test manually, so I added unit tests. I'll add more as I add "official" support for GIFs, videos, and other formats.

In #21562, I tried switching to media.absoluteThumbnailLocalURL for all thumbnails, but it turned out it's used for large thumbnails that can be used fullscreen. Using small thumbnails breaks layout in the editor: recording (under the feature flag only).

This PR has the following changes:

  • ⚠️ This is the only (small) change that affects the production code. Set the maximum scale of large thumbnails mentioned earlier to 2x (3x is way too much for large thumbnails, and it looks like this code hasn't been updated for 3x phones). Example: 2796×1857 px (1.4 MB) → 1864 × 1238 px (542 KB). In addition to that, I changed the JPEG quality to an optional 80%, which reduces the size even further without any noticeable loss in quality: 542 KB → 406 KB. So, overall, it's a 3.5x improvement.
  • Introduces a targetSize(forMediaSize,targetSize) method to make sure this doesn't happen: recording. It's also a prerequisite for adding an "aspect" mode to the photos, like in the native Photos app.
  • Use 1x scale for GIFs. I'll make them animated in the future PRs, and it'll look good. There is a precedent in the app for doing that. It results in a massive size reduction, example, 2.4 MB → 373 KB (6.5x improvement), and with more than acceptable quality for an animated GIF.
  • Switch to magic numbers to identify image format and determine when decompression is needed (more reliable than file extensions)
  • Add a migration step that clears all previously cached thumbnails, as most of them will need to be re-fetched anyway with a more efficient format and/or size when the feature goes out

I'm not sure I like the use of @MainActor, but other than that, I think it's almost there now. Maybe a few minor tweaks. I'll likely move all of this work to the background in the next PR, but I need to figure out how to do it efficiently with Core Data.

To Test

It's hard to test it manually, so I ended up writing a set of unit tests. Instead of duplicating all the test scenarios here, please follow the unit tests.

I asked not to test in the previous PRs because of the imminent upcoming changes. Now is the time to do some testing, preferably with a debugger.

Regression Notes

  1. Potential unintended areas of impact: Post Editor (add photo)
  2. What I did to test those areas of impact (or what existing automated tests I relied on): manual
  3. What automated tests I added (or what prevented me from doing so): yes

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn't complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@kean kean added this to the Pending milestone Sep 20, 2023
@kean kean mentioned this pull request Sep 20, 2023
21 tasks
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Sep 20, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr21615-8e553ec
Version23.3
Bundle IDorg.wordpress.alpha
Commit8e553ec
App Center BuildWPiOS - One-Offs #7177
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Sep 20, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr21615-8e553ec
Version23.3
Bundle IDcom.jetpack.alpha
Commit8e553ec
App Center Buildjetpack-installable-builds #6218
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@kean kean force-pushed the task/media-foundation-thubmnails branch from 5567de8 to 702b0a0 Compare September 21, 2023 15:52
@kean kean requested a review from momo-ozawa September 21, 2023 18:33
@kean kean marked this pull request as ready for review September 21, 2023 18:33
@kean kean force-pushed the task/media-foundation-thubmnails branch from 3b956ea to 8e553ec Compare September 21, 2023 19:02
Copy link
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

Confirmed reported regressions are resolved. Followed unit tests - works as described!

@kean kean merged commit 879a9cf into trunk Sep 25, 2023
@kean kean deleted the task/media-foundation-thubmnails branch September 25, 2023 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants