Editor: images blink when upload is completed

[kean]

When you add images to the post, and the upload completes, the images blink (shows blank canvas until they are downloaded):

https://github.com/wordpress-mobile/WordPress-iOS/assets/1567433/dbc02b55-6b81-4528-9094-b64ed5a15aa9

[kean]

I also wonder why Gutenberg requires the media to have a url right after selection: https://github.com/WordPress/gutenberg/blob/c14792d535c372a045ff4fc262fcaca43c0326a6/packages/block-library/src/image/edit.js#L150. The app actually doesn't have any URLs the picker passes the app the user selection, so it complicates things significantly.

[twstokes]

Thanks for surfacing @kean! I've added it to one of our initiative issues related to media upload improvements and will un-assign myself from this for now.

I also wonder why Gutenberg requires the media to have a url right after selection

I'm actually not entirely sure offhand. A guess would be that the picker passes the editor the image, the image gets uploaded to the backend, and then it's selectable (and configurable) once we know it exists on the site.

[kean]

After removing WPMediaPicker, this is now the last place that uses MediaThumbnailService. So, I was thinking, shouldn't we remove this thumbnail generation? It makes sense not to show the thumbnail until it's uploaded. I'll also reduce the pressure on the resources quite a lot when you upload multiple larger images. And it fixes the animation. @twstokes , thoughts?

https://github.com/wordpress-mobile/WordPress-iOS/assets/1567433/b8c4538b-57e3-482b-882d-d79f1ac5545e

[twstokes]

@twstokes , thoughts?

I think this is less distracting than the blinkenlights effect we have today, but what happens when an upload fails? Does the user know which image they are retrying?

[kean]

It's inefficient in one more way: the high-resolution thumbnails are generated and saved twice during the upload. I've noticed it during my work on 23.2, but I think it's now the right time to address it.

1. MediaImportService creates creates a high-resolution thumbnail and stores it on disk as thumbnailLocalURL. In my case, I see it being resized to a maximum size of 2556x2556 px.
2. MediaImportService also prepares an image for export, resizing it using the new method introduced in https://github.com/wordpress-mobile/WordPress-iOS/pull/21981, and stores as localURL. When I tested it, the maximum size was set to 2000x2000px.

As a result, the app ends up storing two high-resolution images on disk instead of just one. The resizing also puts extra pressure on the system, especially in terms of RAM usage.

The high-resolution preview created during step 1 is used mainly in the Editor. Most of the other parts of the app use localURL to show the original assets.

cc @fluiddot.

[kean]

but what happens when an upload fails? Does the user know which image they are retrying?

Hmm, good point. I think there is a better way of addressing the performance issue (but not the blinking) by exposing the URLs for thumbnails managed by MediaImageService. The .medium size should OK for this purpose. I'll investigate further.

[fluiddot]

For reference, we have a similar GitHub issue in Gutenberg related to this: https://github.com/wordpress-mobile/gutenberg-mobile/issues/6510

[derekblank]

After some initial investigation from the editor side, I believe the blinking issue occurs within the Javascript editor code when the local media file ("thumbnail") is being replaced by the uploaded image from the server. The "blink" timespan may be the time spent downloading the new image from the server. On slower connections, it's more of a small nap than a blink.

The high-resolution preview created during step 1 is used mainly in the Editor. Most of the other parts of the app use localURL to show the original assets.

As @fluiddot mentioned, we can work on improving the blink as part of https://github.com/wordpress-mobile/gutenberg-mobile/issues/6510 by persisting the local media file until the server image is downloaded to create a seamless transition between the two states. If that were the case, we could probably bypass the image generated from MediaThumbnailService as well.

While the performance improvements from the serving the image files are always welcome, the source of the blink itself probably doesn't require further investigation from the WPiOS side at the moment.

[fluiddot]

After some initial investigation from the editor side, I believe the blinking issue occurs within the Javascript editor code when the local media file ("thumbnail") is being replaced by the uploaded image from the server. The "blink" timespan may be the time spent downloading the new image from the server. On slower connections, it's more of a small nap than a blink.

Thanks @derekblank for investigating the issue 🙇 ! We could probably reproduce and confirm that the issue is related to waiting for the remote image download by throttling the network connection. Similarly, we could enforce a failure in the image download to check if the Image block results in a blank image.

[...] by persisting the local media file until the server image is downloaded to create a seamless transition between the two states. If that were the case, we could probably bypass the image generated from MediaThumbnailService as well.

Yep, we should only replace the local image with the remote one when it's fully downloaded. This will most likely address the blink issue.

[derekblank]

@kean I describe some of the details for the blink fix a bit further in p9ugOq-4lR-p2. The fix in https://github.com/WordPress/gutenberg/pull/57869 is mostly focused on the visual transition between the local and network image -- you may note other areas we could improve on the local image side. We only need to generate one local media file, for example.

Could we (or should we) be using localURL instead of localThumbnailURL for displaying the local media file in the Editor?

[kean]

Hey, @derekblank. I appreciate you fixing the issue with thumbnails!

Could we (or should we) be using localURL instead of localThumbnailURL for displaying the local media file in the Editor?

The work for generating the thumbnails is not wasted. It uses MediaImageService to generate a thumbnail of a .large size that could be later used in other places in the app, mainly the media details view and long-press previews.

Having said that, I would agree that the editor should not be using localThumbnailURL for displaying a preview and there is a long-standing comment in the code about that too.

If you look at MediaImportService, it could be significantly simplified if we were to remove .thumbnailReady events. I already removed their usage on the Site Media screen, so it now only the Editor and the PostSettings screens that remain. The later should be a simple refactor.

The only problematic part is the following, which I'm not sure what it does or why:

        case .thumbnailReady(let url) where ReachabilityUtils.isInternetReachable() && media.remoteStatus == .failed:
            gutenberg.mediaUploadUpdate(id: mediaUploadID, state: .failed, progress: 0, url: url, serverID: nil)
        case .thumbnailReady(let url) where !ReachabilityUtils.isInternetReachable() && media.remoteStatus == .failed:
            // The progress value passed is ignored by the editor, allowing the UI to retain the last known progress before pausing
            gutenberg.mediaUploadUpdate(id: mediaUploadID, state: .paused, progress: 0, url: url, serverID: nil)
        case .thumbnailReady(let url):
            gutenberg.mediaUploadUpdate(id: mediaUploadID, state: .uploading, progress: 0.20, url: url, serverID: nil)
            break

[derekblank]

The only problematic part is the following, which I'm not sure what it does or why:

        case .thumbnailReady(let url) where ReachabilityUtils.isInternetReachable() && media.remoteStatus == .failed:
            gutenberg.mediaUploadUpdate(id: mediaUploadID, state: .failed, progress: 0, url: url, serverID: nil)
        case .thumbnailReady(let url) where !ReachabilityUtils.isInternetReachable() && media.remoteStatus == .failed:
            // The progress value passed is ignored by the editor, allowing the UI to retain the last known progress before pausing
            gutenberg.mediaUploadUpdate(id: mediaUploadID, state: .paused, progress: 0, url: url, serverID: nil)
        case .thumbnailReady(let url):
            gutenberg.mediaUploadUpdate(id: mediaUploadID, state: .uploading, progress: 0.20, url: url, serverID: nil)
            break

This code transmits the various media upload events that we send back to the Editor to listen for upload state: uploading, failed, retried, paused, etc.

It seems very plausible that we could remove .thumbnailReady events from MediaImportService and sustain these same Gutenberg media upload events without the explicit need for .thumbnailReady(let url), however. Perhaps by using some other value that indicates the local media is currently "uploading" as a replacement for .thumbnailReady could work?

cc @dcalhoun

[dcalhoun]

The only problematic part is the following [thumbnail media events], which I'm not sure what it does or why:

While I do not completely understand the discussion regarding removing the synchronous thumbnail service or differences between localURL vs localThumbnailURL vs absoluteThumbnailLocalURL, my understanding of the thumbnail media events is that they were introduced in https://github.com/wordpress-mobile/WordPress-iOS/pull/10981 to ensure failed uploads displayed a cached local image behind the "retry" message — in particular the scenario of re-opening a post that contains failed media upload.

The thumbnail events were expanded in https://github.com/wordpress-mobile/WordPress-iOS/pull/14140 and https://github.com/wordpress-mobile/WordPress-iOS/pull/17971 to improve error handling for various, nuanced error circumstances.

Most recently, the events were expanded in https://github.com/wordpress-mobile/WordPress-iOS/pull/22282 to introduce a "paused" state that is utilized to display a different message when a media upload fails due to lack of an internet connection.

It seems very plausible that we could remove .thumbnailReady events from MediaImportService and sustain these same Gutenberg media upload events without the explicit need for .thumbnailReady(let url), however. Perhaps by using some other value that indicates the local media is currently "uploading" as a replacement for .thumbnailReady could work?

From my understanding, it is not incredibly clear to me whether we could remove the events. I'm uncertain as to what would replace its intended communication of "the upload failed, but here is a local thumbnail to display" to the editor.

[kean]

differences between localURL vs localThumbnailURL

• The localURL points to an asset processed by one of the media exporters. It is what gets uploaded to the server.
• The localThumbnailURL points to a .large thumbnail generated by MediaImageService

When the media is created, the localURL value is initially nil. In the updated SiteMediaCollectionCellViewModel, I observe this property and display a thumbnail when it becomes available:

observations.append(media.observe(\.localURL, options: [.new]) { [weak self] media, _ in
    self?.didUpdateLocalThumbnail()
})

I don't think it's ideal that localURL can be nil or that the export can fail, leaving the media entity hanging. It probably has to stay this way, to the very least, because "export" can sometimes involve downloading the asset from the iCloud Photo Library, which can take a while.

The localURL should be enough to display an image preview while it's uploading, at least for images. But the images it points to can be relatively large. It's probably best to stick with the image size .large. What I'm not a fan of is the indirection.

I'm uncertain as to what would replace its intended communication of "the upload failed, but here is a local thumbnail to display" to the editor.

The part about the editor events is what I do not completely understand, but the availability of the thumbnail and the upload status updates should probably be independent events.

[derekblank]

The part about the editor events is what I do not completely understand

In terms of what already exists and what needs to change, I think we all understand 2/3rds of the elements here, and just need to articulate our other third. We'll get there. 😅

the availability of the thumbnail and the upload status updates should probably be independent events

I agree. This is what I was referencing in this comment:

It seems very plausible that we could remove .thumbnailReady events from MediaImportService and sustain these same Gutenberg media upload events without the explicit need for .thumbnailReady(let url), however.

My understanding is that these upload status events don't necessarily need to know that a generated thumbnail is available, but rather that a media upload has been initiated in general. If this understanding is correct, this may the the confusing part for you @kean -- the failed, paused, retried events you referenced shouldn't need to be dependent on .thumbnailReady.

The confusing part for me is why these events were set to be dependent on .thumbnailReady in the first place, and if they could be removed and replaced by some other event that indicates the same "in progress" state (but isn't reliant on actually generating a thumbnail). To clarify, this event is not known to me yet, and may not actually exist.

[kean]

It was fixed in one of the earlier versions, closing.