From a928676280c35ad1accf2f83b2d4475505c40216 Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Wed, 16 Apr 2025 12:35:08 +0530 Subject: [PATCH 1/4] Outline --- web/packages/gallery/services/video.ts | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/web/packages/gallery/services/video.ts b/web/packages/gallery/services/video.ts index 55a2f74996..511ddd6cc3 100644 --- a/web/packages/gallery/services/video.ts +++ b/web/packages/gallery/services/video.ts @@ -84,6 +84,28 @@ export interface HLSPlaylistData { * the given file. * * See: [Note: Video playlist and preview] + * + * --- + * + * [Note: Caching HLS playlist data] + * + * The playlist data can be cached in an asymmetric manner. + * + * - If a file has a corresponding HLS playlist, then currently there is no + * scenario (apart from file deletion, where the playlist also gets deleted) + * where the playlist is updated or deleted after being created. There is + * still going to be a limit to the validity of the presigned chunk URLs + * within the playlist we create which we do handle (see + * `createHLSPlaylistItemDataValidity`), but the original playlist itself does + * not change. In particular, a positive result ("this file has a playlist") + * can be cached indefinitely. + * + * - If a file does not have a HLS playlist, and it is eligible for being + * streamed (e.g. it is not too small where the streaming overhead is not + * required), then a client (this one, or a different one) can process it at + * any arbitrary time. So the negative result ("this file does not have a + * playlist") cannot be cached. + * */ export const hlsPlaylistDataForFile = async ( file: EnteFile, From 864f0317fa0c6d5eb3ac4fb051ce200de862c9b8 Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Wed, 16 Apr 2025 12:56:31 +0530 Subject: [PATCH 2/4] Outline --- .../gallery/components/viewer/data-source.ts | 30 ++++++++++++------- .../gallery/components/viewer/photoswipe.ts | 13 ++------ web/packages/gallery/services/video.ts | 15 ++++++++++ 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/web/packages/gallery/components/viewer/data-source.ts b/web/packages/gallery/components/viewer/data-source.ts index 933e66d5bc..9c57213049 100644 --- a/web/packages/gallery/components/viewer/data-source.ts +++ b/web/packages/gallery/components/viewer/data-source.ts @@ -251,7 +251,7 @@ export const fileViewerDidClose = () => { resetState(); } else { // Selectively clear. - forgetFailedItems(); + forgetFailedOrTransientItems(); forgetExif(); } }; @@ -357,12 +357,18 @@ export const forgetItemDataForFileID = (fileID: number) => { }; /** - * Forget item data for the given {@link file} if its fetch had failed. + * Forget item data for the given {@link file} if its fetch had failed, or if it + * is caching something that is transient in nature. * - * This is called when the user moves away from a slide so that we attempt a - * full retry when they come back the next time. + * It is called when the user moves away from a slide. In particular, this way + * we can reset failures, if any, for a slide so that the fetch is tried again + * when we come back to it. + * + * See: [Note: File viewer error handling] + * + * See: [Note: Caching HLS playlist data] */ -export const forgetFailedItemDataForFileID = (fileID: number) => { +export const forgetItemDataForFileIDIfNeeded = (fileID: number) => { if (_state.itemDataByFileID.get(fileID)?.fetchFailed) forgetItemDataForFileID(fileID); }; @@ -381,13 +387,17 @@ export const updateItemDataAlt = (updatedFile: EnteFile) => { }; /** - * Forget item data for the all files whose fetch had failed. + * Forget item data for the all files whose fetch had failed, or if the + * corresponding item data is transient and shouldn't be cached. * - * This is called when the user closes the file viewer so that we attempt a full - * retry when they reopen the viewer the next time. + * This is called when the user closes the file viewer; in particular, this way + * we attempt a full retry for previously failed files when the user reopens the + * viewer the next time. */ -const forgetFailedItems = () => - [..._state.itemDataByFileID.keys()].forEach(forgetFailedItemDataForFileID); +const forgetFailedOrTransientItems = () => + [..._state.itemDataByFileID.keys()].forEach( + forgetItemDataForFileIDIfNeeded, + ); const enqueueUpdates = async ( file: EnteFile, diff --git a/web/packages/gallery/components/viewer/photoswipe.ts b/web/packages/gallery/components/viewer/photoswipe.ts index 72d3fc90e3..6402a013d0 100644 --- a/web/packages/gallery/components/viewer/photoswipe.ts +++ b/web/packages/gallery/components/viewer/photoswipe.ts @@ -14,8 +14,8 @@ import { fileViewerDidClose, fileViewerWillOpen, forgetExifForItemData, - forgetFailedItemDataForFileID, forgetItemDataForFileID, + forgetItemDataForFileIDIfNeeded, itemDataForFile, updateFileInfoExifIfNeeded, type ItemData, @@ -710,17 +710,10 @@ export class FileViewerPhotoSwipe { const queryVideoElement = (element: HTMLElement | undefined) => element?.querySelector("video, hls-video"); + // PhotoSwipe invokes this event when moving away from a slide. pswp.on("contentDeactivate", (e) => { - // Reset failures, if any, for this file so that the fetch is tried - // again when we come back to it^. - // - // ^ Note that because of how the preloading works, this will have - // an effect (i.e. the retry will happen) only if the user moves - // more than 2 slides and then back, or if they reopen the viewer. - // - // See: [Note: File viewer error handling] const fileID = asItemData(e.content.data).fileID; - if (fileID) forgetFailedItemDataForFileID(fileID); + if (fileID) forgetItemDataForFileIDIfNeeded(fileID); // Pause the video element, if any, when we move away from the // slide. diff --git a/web/packages/gallery/services/video.ts b/web/packages/gallery/services/video.ts index 511ddd6cc3..e038fb31e7 100644 --- a/web/packages/gallery/services/video.ts +++ b/web/packages/gallery/services/video.ts @@ -106,6 +106,21 @@ export interface HLSPlaylistData { * any arbitrary time. So the negative result ("this file does not have a * playlist") cannot be cached. * + * So while we can easily cache the first case ("this file has a playlist"), we + * need to deal with the second case ("this file does not have a playlist") a + * bit more intricately: + * + * - If running in the context of a logged in user (e.g. photos app), we can use + * the "/files/data/status-diff" API to be notified of any modifications to + * the second case for the user's own files. This status-diff happens during + * the regular "sync", and we can use that as a cue to selectively prune cache + * entries for the second case but can otherwise indefinitely cache it. + * + * - If the file is a shared file, the status-diff will not return it. And if + * we're not running in the context of a logged in user (e.g. the public + * albums app), then there is no status-diff to do. For these two scenarios, + * we thus mark the cached values as "transient" and always recheck for a + * playlist when opening the slide. */ export const hlsPlaylistDataForFile = async ( file: EnteFile, From 7d92b5923b6b5ce464a8466f6770624562fe80e7 Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Wed, 16 Apr 2025 13:38:27 +0530 Subject: [PATCH 3/4] Clear transient --- .../gallery/components/viewer/data-source.ts | 45 ++++++++++++++++++- .../gallery/components/viewer/photoswipe.ts | 6 ++- web/packages/gallery/services/video.ts | 13 +++--- 3 files changed, 54 insertions(+), 10 deletions(-) diff --git a/web/packages/gallery/components/viewer/data-source.ts b/web/packages/gallery/components/viewer/data-source.ts index 9c57213049..125a47686f 100644 --- a/web/packages/gallery/components/viewer/data-source.ts +++ b/web/packages/gallery/components/viewer/data-source.ts @@ -145,6 +145,11 @@ export type ItemData = PhotoSwipeSlideData & { * original file could not be fetched because of an network error). */ fetchFailed?: boolean; + /** + * This will be `true` if the item data for this particular file is + * considered transient, and should not be cached. + */ + isTransient?: boolean; }; /** @@ -364,12 +369,33 @@ export const forgetItemDataForFileID = (fileID: number) => { * we can reset failures, if any, for a slide so that the fetch is tried again * when we come back to it. * + * [Note: File viewer preloading and contentDeactivate] + * + * Note that because of preloading, this will only have a user visible effect if + * the user moves out of the preload range. Let's take an example: + * + * - User opens slide at index `i`. Then the adjacent slides, `i - 1` and `i + + * 1`, also get preloaded. + * + * - User moves away from `i` (in either direction, but let us take the example + * if they move `i - 1`). + * + * - This function will get called for `i` and will clear any failed or + * transient state. + * + * - But since PhotoSwipe already has the slides ready for `i`, if the user then + * moves back to `i` then PhotoSwipe will not attempt to recreate the slide + * and so will not even try to get "itemData". So this will only have an + * effect if they move out of preload range (e.g. `i - 1`, `i - 2`), and then + * back (`i - 1`, `i`). + * * See: [Note: File viewer error handling] * * See: [Note: Caching HLS playlist data] */ export const forgetItemDataForFileIDIfNeeded = (fileID: number) => { - if (_state.itemDataByFileID.get(fileID)?.fetchFailed) + const itemData = _state.itemDataByFileID.get(fileID); + if (itemData?.fetchFailed || itemData?.isTransient) forgetItemDataForFileID(fileID); }; @@ -441,7 +467,22 @@ const enqueueUpdates = async ( createHLSPlaylistItemDataValidity(), ); } else { - update(videoURLD); + if (shouldUsePlayerV2()) { + // See: [Note: Caching HLS playlist data] + // + // TODO(HLS): As an optimization, we can handle the logged in vs + // public albums case separately once we have the status-diff + // state, we don't need to mark status-diff case as transient. + // + // Note that setting the transient flag is not too expensive, + // since the underlying videoURL is still cached by the download + // manager. So effectively, under normal circumstance, it just + // adds one API call (to recheck if an HLS playlist now exists + // for the given file). + update({ ...videoURLD, isTransient: true }); + } else { + update(videoURLD); + } } }; diff --git a/web/packages/gallery/components/viewer/photoswipe.ts b/web/packages/gallery/components/viewer/photoswipe.ts index 6402a013d0..c2caee211e 100644 --- a/web/packages/gallery/components/viewer/photoswipe.ts +++ b/web/packages/gallery/components/viewer/photoswipe.ts @@ -710,8 +710,12 @@ export class FileViewerPhotoSwipe { const queryVideoElement = (element: HTMLElement | undefined) => element?.querySelector("video, hls-video"); - // PhotoSwipe invokes this event when moving away from a slide. pswp.on("contentDeactivate", (e) => { + // PhotoSwipe invokes this event when moving away from a slide. + // + // However it might not have an effect until we move out of preload + // range. See: [Note: File viewer preloading and contentDeactivate]. + const fileID = asItemData(e.content.data).fileID; if (fileID) forgetItemDataForFileIDIfNeeded(fileID); diff --git a/web/packages/gallery/services/video.ts b/web/packages/gallery/services/video.ts index e038fb31e7..ec1cb4d419 100644 --- a/web/packages/gallery/services/video.ts +++ b/web/packages/gallery/services/video.ts @@ -93,12 +93,11 @@ export interface HLSPlaylistData { * * - If a file has a corresponding HLS playlist, then currently there is no * scenario (apart from file deletion, where the playlist also gets deleted) - * where the playlist is updated or deleted after being created. There is - * still going to be a limit to the validity of the presigned chunk URLs - * within the playlist we create which we do handle (see - * `createHLSPlaylistItemDataValidity`), but the original playlist itself does - * not change. In particular, a positive result ("this file has a playlist") - * can be cached indefinitely. + * where the playlist is updated or deleted after being created. There is a + * limit to the validity of the presigned chunk URLs within the playlist we + * create which we do handle (`createHLSPlaylistItemDataValidity`), but the + * original playlist itself does not change. In particular, a positive result + * ("this file has a playlist") can be cached indefinitely. * * - If a file does not have a HLS playlist, and it is eligible for being * streamed (e.g. it is not too small where the streaming overhead is not @@ -114,7 +113,7 @@ export interface HLSPlaylistData { * the "/files/data/status-diff" API to be notified of any modifications to * the second case for the user's own files. This status-diff happens during * the regular "sync", and we can use that as a cue to selectively prune cache - * entries for the second case but can otherwise indefinitely cache it. + * entries for the second case (but can otherwise indefinitely cache it). * * - If the file is a shared file, the status-diff will not return it. And if * we're not running in the context of a logged in user (e.g. the public From 4d3926c1501a5d3588155c101be27809c1b97195 Mon Sep 17 00:00:00 2001 From: Manav Rathi Date: Wed, 16 Apr 2025 14:14:10 +0530 Subject: [PATCH 4/4] Enable for albums app --- web/packages/gallery/components/viewer/photoswipe.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/web/packages/gallery/components/viewer/photoswipe.ts b/web/packages/gallery/components/viewer/photoswipe.ts index c2caee211e..2f953e7e88 100644 --- a/web/packages/gallery/components/viewer/photoswipe.ts +++ b/web/packages/gallery/components/viewer/photoswipe.ts @@ -1,5 +1,6 @@ import { pt } from "ente-base/i18n"; import log from "ente-base/log"; +import { albumsAppOrigin } from "ente-base/origins"; import type { EnteFile } from "ente-media/file"; import { FileType } from "ente-media/file-type"; import { settingsSnapshot } from "ente-new/photos/services/settings"; @@ -166,7 +167,15 @@ export const moreMenuID = "ente-pswp-more-menu"; // TODO(HLS): let _shouldUsePlayerV2: boolean | undefined; export const shouldUsePlayerV2 = () => - (_shouldUsePlayerV2 ??= settingsSnapshot().isInternalUser); + // Enable for internal users and public albums + (_shouldUsePlayerV2 ??= + settingsSnapshot().isInternalUser || isPublicAlbumApp()); + +const isPublicAlbumApp = () => { + const currentURL = new URL(window.location.href); + const albumsURL = new URL(albumsAppOrigin()); + return currentURL.host == albumsURL.host; +}; /** * A wrapper over {@link PhotoSwipe} to tailor its interface for use by our file