Integrate PHPickerViewController in Media library #21343

kean · 2023-08-15T21:13:23Z

Part of #21190.

The changes affect both the WP and the Jetpack app.

Tests

Important

Enable the "Native Photo Picker" feature flag before testing
The new picker was added only to the Media library screen itself

Permissions

Install a fresh build to reset the permissions
Verify that to upload media, the app no longer asks for permissions

Media Formats

Verify that .heic images are uploaded as .jpeg (matches the existing production behavior of using a high-compatibility format by default)
Verify that .webp images are uploaded as .heic (the existing production version doesn't support them; more info in the code comments)
Verify that .gif are uploaded (with no modifications)
Verify that video files are uploaded (and transcoded to .mp4)

You can add a WebP image from https://developers.google.com/speed/webp/gallery1: open it in the browser, long-tap and select “Save to Photos”.

Image Resizing

Go to Me / App Settings
Set Max Image Upload Size to 200x200
Upload an image
Open the image details and verify that the resolution is lower than 200x200

Removing GPS Data

Enable "Remove Location From Media" in App Settings
Upload an image with GPS data
Verify that the GPS data was removed

You can use test-image-device-photo-gps.jpg for testing and macOS Cmd+I command to view photo EXIF.

iCloud

Open Jetpack on device A with "Optimize iPhone Storage" enabled
Open the new photo picker
Upload a large photo on device B
Verify that the photo is automatically added to the picker on device A
Select the photo before it gets fully downloaded
Verify that the photo got uploaded to Jetpack

Misc

Verify that progress is updated during export and upload
Verify that the selection is unlimited (as much as the system can chew)
There should be no need to test the picker itself because it's fully native, but some of its advantages are outlined here Integrate PHPickerViewController #21190
Verify that the picker uses the app's tint color

Regression Notes

Potential unintended areas of impact: Media uploads
What I did to test those areas of impact (or what existing automated tests I relied on): manual and unit tests
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)

wpmobilebot · 2023-08-15T21:58:06Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr21343-7cc3ee8
	Version	23.0
	Bundle ID	com.jetpack.alpha
	Commit	`7cc3ee8`
	App Center Build	jetpack-installable-builds #5820

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-08-15T21:59:10Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr21343-7cc3ee8
	Version	23.0
	Bundle ID	org.wordpress.alpha
	Commit	`7cc3ee8`
	App Center Build	WPiOS - One-Offs #6780

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

kean · 2023-08-16T14:28:00Z

WordPress/Classes/Utility/Media/ItemProviderMediaExporter.swift

+        self.provider = provider
+    }
+
+    func export(onCompletion: @escaping (MediaExport) -> Void, onError: @escaping (MediaExportError) -> Void) -> Progress {


It works nearly identical to the existing MediaAssetExporter (PHAsset support) and shares some of the code with it.

Great post on NSItemProvider: https://www.humancode.us/2023/07/08/all-about-nsitemprovider.html.

kean · 2023-08-16T14:28:18Z

WordPress/Classes/Utility/Media/ItemProviderMediaExporter.swift

+    ].map(\.identifier))
+
+    private func hasConformingType(_ type: UTType) -> Bool {
+        provider.hasItemConformingToTypeIdentifier(type.identifier)


There is a nice UTType-based API but it’s available only in iOS 16 :(

kean · 2023-08-16T14:28:58Z

WordPress/WordPressTest/ItemProviderMediaExporterTests.swift

+import XCTest
+@testable import WordPress
+
+final class ItemProviderMediaExporterTests: XCTestCase {


MediaImageExporterTests that’s used internally already has a bunch of tests, so we mostly rely on them and just test the integration.

kean · 2023-08-18T18:35:36Z

I've been running some additional tests and encountered a limitation with RAW files. I added a RAW file to the Photos library (6000×4000px) and tried uploading it, but the file provider serves the downscaled version:

(lldb) po CGImageSourceCreateImageAtIndex(source, 0, nil)
▿ Optional<CGImageRef>
  - some : <CGImage 0x10ba415d0> (IP)
	<<CGColorSpace 0x10a384460> (kCGColorSpaceICCBased; kCGColorSpaceModelRGB; sRGB IEC61966-2.1)>
		width = 1616, height = 1080, bpc = 8, bpp = 32, row bytes = 6464 
		kCGImageAlphaNoneSkipLast | 0 (default byte order)  | kCGImagePixelFormatPacked 
		is mask? No, has masking color? No, has soft mask? No, has matte? No, should interpolate? Yes

I tried uploading an even larger JPEG file (7200 × 5400px), and it worked perfectly and uploaded the full-resolution image.

I have yet to find any documentation covering RAW support, so I'm asking around. There is a workaround: PHPickerResult contains an asset identifier, which you can always you to fetch the original data, but the problem is it requires access to the Photos library, which we are trying to avoid.

I'm not sure how important that scenario is, but it is still worth mentioning. WordPress doesn't support RAW files, so the app still has to transcode the image to JPEG. But the whole point of RAW files is to allow the users to pull out as many details from the photo as possible and create the JPEG they want. It's just not a thing that someone would want to upload a RAW file to WordPress and use the default iOS processing with no adjustments to the original photo.

guarani · 2023-08-18T21:41:51Z

Finished manual testing and this is looking great! I have a couple of questions I left below.

I'll go through the code separately and leave a review after that.

.heic image not uploading on the sim

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-08-18.at.16.25.05.mp4

Picker saying the location is included

This branch	23.0 beta

Picker navigation bar transparency color

kean · 2023-08-18T23:43:51Z

On the iPhone simulator, the sample .heic image isn't being added to the WP.com media library

It's a known issue with the flower image on the simulator. The item provider fails to export it on sims; nothing we can do. It's shame it's the very first one in the list 😃

kean · 2023-08-19T00:04:41Z

Note that the native picker says "location included" which could be confusing given that the app's setting for this is OFF.

The export options are a new feature added in iOS 17. Is that what you are testing with?

There is no way to remove these options, but I think it's a great opportunity for us to remove some of the work that MediaImageExporter does. I kept it as is, so it fully matched the production version, but it seems problematic in a few ways:

Why is it using CGImageSourceCreateThumbnailAtIndex and not CGImageSourceCreateImageAtIndex?
Why is it changing the compression ratio? Does it mean we lose signal when re-exporting the photos?

I think, by default, the app should upload the image as is with no modifications whatsoever. Suppose we remove the "remove GPS data" feature from the Jetpack app in favor of the built-in feature on iOS 17. In that case, we can re-evaluate the decisions in MediaImageExporter too. It'll be great to simplify it to reduce the amount of work the app does and also ensure we don't lose signal.

Btw, there is a screenshot attached to the message that says 23.0 beta, but the new photo picker isn't included in it.

What does export refer to?

Exporting the item from NSItemProvider and processing it using MediaImageExporter. It takes a little under a second per a reasonably large photo on modern devices (which is way too much).

guarani · 2023-08-19T00:54:12Z

It's a known issue with the flower image on the simulator. The item provider fails to export it on sims; nothing we can do. It's shame it's the very first one in the list 😃

Aha good catch, I didn't know that – it used to work at some point in the past.

The export options are a new feature added in iOS 17. Is that what you are testing with?

Yep, I'm on iOS 17 on my iPhone.

There is no way to remove these options, but I think it's a great opportunity for us to remove some of the work that MediaImageExporter does. I kept it as is, so it fully matched the production version, but it seems problematic in a few ways:

Why is it using CGImageSourceCreateThumbnailAtIndex and not CGImageSourceCreateImageAtIndex?

Why is it changing the compression ratio? Does it mean we lose signal when re-exporting the photos?

I think, by default, the app should upload the image as is with no modifications whatsoever. Suppose we remove the "remove GPS data" feature from the Jetpack app in favor of the built-in feature on iOS 17. In that case, we can re-evaluate the decisions in MediaImageExporter too. It'll be great to simplify it to reduce the amount of work the app does and also ensure we don't lose signal.

I'm in favor of relying on the system "remove GPS data" if there's a way to ensure users don't accidentally upload GPS data by not realizing they need to manually turn it off at the system level.

Btw, there is a screenshot attached to the message that says 23.0 beta, but the new photo picker isn't included in it.

The screenshot shows the system picker that's nested inside MediaPicker-iOS when the user selects more photos to grant the app access to. Here's a video showing it:

done.mov

What does export refer to?

Exporting the item from NSItemProvider and processing it using MediaImageExporter. It takes a little under a second per a reasonably large photo on modern devices (which is way too much).

Gotcha 👍

kean · 2023-08-19T01:49:53Z

I'm in favor of relying on the system "remove GPS data" if there's a way to ensure users don't accidentally upload GPS data by not realizing they need to manually turn it off at the system level

We can keep it in the app but avoid doing any extra processing if it's not there. Reading EXIF and verifying if it's there is a
nearly instantaneous operation. Then we'd only need to check if the image size is greater than the preferred upload size and, again, avoid doing any extra processing if it's OK.

The screenshot shows the system picker that's nested inside MediaPicker-iOS when the user selects more photos to grant the app access to.

Ah, got it. Yes, it makes sense it doesn't have the export options: it doesn't export the photos.

mokagio · 2023-08-21T01:58:19Z

I'm going to bump this to the next release because we'll be code freezing 23.1 today and this hasn't been approved yet.

If this cannot wait two weeks and it's important that it makes it into this release, let me know and we'll organize a new beta once ready.

guarani

This looks good @kean! Tested in my last review 👍 The typos aren't blockers if you can fix them in a follow-up PR.

I'm excited to see the new image picker and I know this will fix a bunch of issues like #19576 once it's integrated across the rest of the app.

WordPress/Classes/ViewRelated/Media/NSIterProvider+Exportable.swift

WordPress/Classes/ViewRelated/Media/PHPickerContoller+Extensions.swift

kean added 2 commits August 15, 2023 10:49

Add .nativePhotoPicker feature flag

67a22de

Integrate PHPickerController on the Media screen

29b10fc

kean added Media [Type] Task UI Modernization labels Aug 15, 2023

kean added this to the 23.1 milestone Aug 15, 2023

kean added 3 commits August 15, 2023 21:34

Add WebP support and cleanup

7274402

Add more unit tests

b336e4c

Update error handling

c317cef

kean marked this pull request as ready for review August 16, 2023 14:12

Remove selection limit

2a19f6a

kean commented Aug 16, 2023

View reviewed changes

kean requested review from momo-ozawa and guarani August 16, 2023 14:29

This was referenced Aug 16, 2023

Integrate PHPickerViewController #21190

Closed

Integrate PHPickerViewController in Page Editor #21360

Merged

mokagio modified the milestones: 23.1, 23.2 Aug 21, 2023

Merge branch 'trunk' into task/phpicker-media-screen

0a680ab

kean removed the request for review from momo-ozawa August 21, 2023 15:13

guarani approved these changes Aug 21, 2023

View reviewed changes

WordPress/Classes/ViewRelated/Media/NSIterProvider+Exportable.swift Outdated Show resolved Hide resolved

WordPress/Classes/ViewRelated/Media/PHPickerContoller+Extensions.swift Outdated Show resolved Hide resolved

Fix typos

2e23696

kean enabled auto-merge August 21, 2023 15:41

Fix media export unit tests by isolating them

7cc3ee8

kean merged commit d264d82 into trunk Aug 21, 2023

kean deleted the task/phpicker-media-screen branch August 21, 2023 19:15

This was referenced Aug 23, 2023

Disable one of the new video export test cases #21400

Merged

Fix media uploads error handling #21411

Merged

momo-ozawa assigned kean Sep 1, 2023

kean mentioned this pull request Sep 13, 2023

Media: Implement media uploads #21562

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate PHPickerViewController in Media library #21343

Integrate PHPickerViewController in Media library #21343

kean commented Aug 15, 2023 •

edited

Loading

wpmobilebot commented Aug 15, 2023 •

edited

Loading

wpmobilebot commented Aug 15, 2023 •

edited

Loading

kean Aug 16, 2023

kean Aug 16, 2023

kean Aug 16, 2023

kean commented Aug 18, 2023 •

edited

Loading

guarani commented Aug 18, 2023 •

edited

Loading

kean commented Aug 18, 2023 •

edited

Loading

kean commented Aug 19, 2023 •

edited

Loading

guarani commented Aug 19, 2023

kean commented Aug 19, 2023 •

edited

Loading

mokagio commented Aug 21, 2023

guarani left a comment

Integrate PHPickerViewController in Media library #21343

Integrate PHPickerViewController in Media library #21343

Conversation

kean commented Aug 15, 2023 • edited Loading

Tests

Permissions

Media Formats

Image Resizing

Removing GPS Data

iCloud

Misc

Regression Notes

wpmobilebot commented Aug 15, 2023 • edited Loading

wpmobilebot commented Aug 15, 2023 • edited Loading

kean Aug 16, 2023

Choose a reason for hiding this comment

kean Aug 16, 2023

Choose a reason for hiding this comment

kean Aug 16, 2023

Choose a reason for hiding this comment

kean commented Aug 18, 2023 • edited Loading

guarani commented Aug 18, 2023 • edited Loading

.heic image not uploading on the sim

Picker saying the location is included

Picker navigation bar transparency color

kean commented Aug 18, 2023 • edited Loading

kean commented Aug 19, 2023 • edited Loading

guarani commented Aug 19, 2023

kean commented Aug 19, 2023 • edited Loading

mokagio commented Aug 21, 2023

guarani left a comment

Choose a reason for hiding this comment

kean commented Aug 15, 2023 •

edited

Loading

wpmobilebot commented Aug 15, 2023 •

edited

Loading

wpmobilebot commented Aug 15, 2023 •

edited

Loading

kean commented Aug 18, 2023 •

edited

Loading

guarani commented Aug 18, 2023 •

edited

Loading

kean commented Aug 18, 2023 •

edited

Loading

kean commented Aug 19, 2023 •

edited

Loading

kean commented Aug 19, 2023 •

edited

Loading