fix review findings: playlist tar, downloads, player edge cases

server:
- build playlist tar from index-prefixed symlinks, so equal file
  names no longer overwrite each other and the order is kept
- apply the /download file checks to /download-playlist
- check the media type before probing in /transcode
- skip unreadable subdirectories in recursive listings
- fix type error on the first ffmpeg read result

client:
- make the file filter case-insensitive
- do not start playback when adding a single file to a
  non-empty playlist
- clear the loading screen when a playlist import fails
- decrement download progress counters only once
- restore a saved volume of 0
- do not crash on a malformed URL hash
- show the status code when statusText is empty (HTTP/2)
- show the server error when an MSE transcode fails
- do not seek twice after dragging the progress handle
AuthorKonata <konata@posteo.jp>
Date
Commit173507485369f90e5a00f112c25e874f50d2c11e
Parentd78fc9e
8 files changed, 118 insertions(+), 73 deletions(-)
▾Mserver/src/index.ts
@@ -74,11 +74,15 @@ function resolveInRoot(user: User, encodedPath: string): Promise<Resolved | Serv
return resolveRelativeInRoot(user, decodePath(encodedPath));
}
async function resolveMediaFile(
function resolveMediaFile(user: User, encodedPath: string): Promise<(Resolved & { info: PathInfo }) | ServerError> {
return resolveRelativeMediaFile(user, decodePath(encodedPath));
}
async function resolveRelativeMediaFile(
user: User,
encodedPath: string,
relPath: string,
): Promise<(Resolved & { info: PathInfo }) | ServerError> {
const resolved = await resolveInRoot(user, encodedPath);
const resolved = await resolveRelativeInRoot(user, relPath);
if (resolved instanceof ServerError) return resolved;
const info = await getPathInfo(resolved.filePath);
//undefined means it is a directory rather than a file
@@ -250,16 +254,16 @@ const app = setup
return resolved.error;
}
const { filePath, info: fileScan } = resolved;
const probe = await probeFile(filePath);
//don't use higher bitrate than what the file has, use requested bitrate if unknown
const audioBitrate = clampToSource(query.audioBitrate, probe.audioBitrate);
const videoBitrate = clampToSource(query.videoBitrate, probe.videoBitrate);
//before the probe, which throws on non-media files such as images
if (!matchesType(fileScan.mimeType, mediaTypes)) {
set.status = "Temporary Redirect";
set.headers.Location = `/download/${params["*"]}`;
return "Not a media file, redirecting to normal endpoint";
}
const probe = await probeFile(filePath);
//don't use higher bitrate than what the file has, use requested bitrate if unknown
const audioBitrate = clampToSource(query.audioBitrate, probe.audioBitrate);
const videoBitrate = clampToSource(query.videoBitrate, probe.videoBitrate);
if (query.videoCodec && query.videoCodec !== VideoCodec.none && !videoBitrate) {
set.status = "Bad Request";
@@ -319,7 +323,7 @@ const app = setup
//peek ffmpeg to check for failure and return 500
const reader = cmd.stdout.getReader();
let first: ReadableStreamReadResult<Uint8Array>;
let first: Awaited<ReturnType<typeof reader.read>>;
try {
first = await reader.read();
} catch (error) {
@@ -521,7 +525,8 @@ const app = setup
}
const resolvedPaths: string[] = [];
for (const entry of playlist) {
const resolved = await resolveRelativeInRoot(user, entry);
//same file checks as /download, so a playlist cannot fetch directories or excluded files
const resolved = await resolveRelativeMediaFile(user, entry);
if (resolved instanceof ServerError) {
set.status = resolved.status;
return resolved.error;
@@ -536,7 +541,7 @@ const app = setup
}
set.headers["Content-Type"] = "application/x-tar";
set.headers["Content-Disposition"] = `attachment; filename="playlist.tar"`;
return new Response(packWithTar(resolvedPaths).stdout);
return new Response((await packWithTar(resolvedPaths)).stdout);
},
{ params: t.Object({ id: t.String({ minLength: 1 }) }) },
)
▾Mserver/src/utils.ts
@@ -1,4 +1,4 @@
import { readdir, readFile, realpath, rm, stat, writeFile } from "node:fs/promises";
import { mkdtemp, readdir, readFile, realpath, rm, stat, symlink, writeFile } from "node:fs/promises";
import { tmpdir } from "node:os";
import path from "node:path";
import { StatusMap } from "elysia";
@@ -387,7 +387,8 @@ export async function listFiles(
if (!pathInfoResult) {
if (recursive) {
const subListing = await listFiles(rootDir, itemPath, true);
if (subListing instanceof ServerError) throw subListing;
//skip it like an unreadable file, one bad directory (e.g. lost+found) must not fail the whole tree
if (subListing instanceof ServerError) return;
files[fileName] = { files: subListing, status: "Scanned" };
} else {
files[fileName] = { files: {}, status: "Unknown" };
@@ -404,7 +405,6 @@ export async function listFiles(
);
return files;
} catch (error) {
if (error instanceof ServerError) return error;
if (errorCode(error) === "ENOENT") {
return new ServerError(StatusMap["Not Found"], "Directory not found");
}
@@ -413,25 +413,35 @@ export async function listFiles(
}
}
//takes absolute paths the caller has already resolved and validated. the transform flattens every
//entry to its basename, so a playlist spanning several roots needs no grouping
export function packWithTar(absoluteFiles: string[]) {
const tarArgs = [
"--transform=s|.*/||", //only supported in gnu tar TODO: add fallback
"-c", // create archive
...absoluteFiles,
];
return Bun.spawn(["tar", ...tarArgs], {
stdin: "ignore",
stdout: "pipe",
stderr: "pipe",
onExit: async (subprocess: Bun.Subprocess<"ignore", "pipe", "pipe">, exitCode: number | null) => {
if (exitCode !== 0 && exitCode !== null) {
console.error("tar failed with status code", exitCode);
console.error(await new Response(subprocess.stderr).text());
}
},
});
//takes absolute paths the caller has already resolved and validated. each entry is symlinked into a temp
//dir under an index-prefixed name, so equal basenames from different folders cannot overwrite each other
//on extraction, and the archive keeps the playlist order
export async function packWithTar(absoluteFiles: string[]) {
const linkDir = await mkdtemp(path.join(tmpdir(), "playlist-"));
const cleanup = () => rm(linkDir, { recursive: true, force: true });
try {
const digits = Math.max(3, String(absoluteFiles.length).length);
const names = absoluteFiles.map(
(file, index) => `${String(index + 1).padStart(digits, "0")} - ${path.basename(file)}`,
);
await Promise.all(absoluteFiles.map((file, index) => symlink(file, path.join(linkDir, names[index]))));
//-h stores the link targets. names are listed explicitly, "." would also archive the temp dir itself
return Bun.spawn(["tar", "-c", "-h", "-C", linkDir, ...names], {
stdin: "ignore",
stdout: "pipe",
stderr: "pipe",
onExit: async (subprocess: Bun.Subprocess<"ignore", "pipe", "pipe">, exitCode: number | null) => {
await cleanup();
if (exitCode !== 0 && exitCode !== null) {
console.error("tar failed with status code", exitCode);
console.error(await new Response(subprocess.stderr).text());
}
},
});
} catch (error) {
await cleanup();
throw error;
}
}
//copy to a temp file and then use avifenc to convert
▾Mwebclient/src/App.tsx
@@ -46,6 +46,7 @@ import {
LocalStorageValues,
setForbiddenPathHandler,
setUnauthorizedHandler,
statusLabel,
toast,
} from "./utils";
@@ -78,10 +79,18 @@ function bufferedAhead(ranges: TimeRanges, time: number): number {
return 0; //the playhead is starving, so append immediately
}
//a hand-edited hash can hold a stray "%", which makes decodeURIComponent throw
function dirFromHash(): string {
try {
return decodePath(window.location.hash.slice(1));
} catch {
return "";
}
}
//initial directory, resolved synchronously so the very first listing fetch already targets it
function initialDir(): string {
const hash = window.location.hash.slice(1);
if (hash) return decodePath(hash);
if (window.location.hash.length > 1) return dirFromHash();
try {
const savedOptions = localStorage.getItem(LocalStorageValues.options);
const rememberDir = savedOptions ? (JSON.parse(savedOptions) as Partial<AppOptions>).rememberDir : false;
@@ -470,7 +479,9 @@ const App: Component = () => {
const signal = mediaSourceAbortController.signal;
const response = await fetch(srcUrl, { credentials: "same-origin", signal });
if (handleUnauthorized(response)) return;
if (!response.ok || !response.body) throw new Error("Failed to fetch stream");
//the body carries the server's ffmpeg error, which is the only useful part of a failed transcode
if (!response.ok || !response.body)
throw new Error((await response.text().catch(() => "")) || statusLabel(response));
const reader = response.body.getReader();
function waitUntilUpdateDone() {
@@ -604,7 +615,7 @@ const App: Component = () => {
error,
);
} else if (!(error instanceof Error && error.name === "AbortError")) {
console.error(error);
toast("Streaming the transcode failed", "error", error);
}
} finally {
try {
@@ -826,7 +837,7 @@ const App: Component = () => {
});
}
const savedVolume = Number.parseFloat(localStorage.getItem(LocalStorageValues.volume) ?? "");
if (savedVolume && !Number.isNaN(savedVolume)) {
if (!Number.isNaN(savedVolume)) {
setPlayerState("volume", savedVolume);
}
const savedPlaylist = localStorage.getItem(LocalStorageValues.playlist);
@@ -889,7 +900,7 @@ const App: Component = () => {
window.addEventListener("pagehide", stopPlaying);
window.addEventListener("hashchange", () => {
setCurrentDir(decodePath(window.location.hash.slice(1)));
setCurrentDir(dirFromHash());
});
document.addEventListener("keydown", (e) => {
▾Mwebclient/src/components/FileBrowser.tsx
@@ -141,20 +141,12 @@ export default function FileBrowser(props: FileBrowserProps) {
setLoadingActions(loadingActions() + 1);
}
try {
let oldLength = 0;
let newPlaylist: PlaylistItem[];
const oldPlaylist = queue ? props.playlist() : [];
if (item) {
newPlaylist = [fileToPlaylistItem(fullPath, item)];
} else {
const files = await listFiles(fullPath, props.isOffline(), recursive);
newPlaylist = listingToPlaylistItems(fullPath, files);
if (queue) {
oldLength = oldPlaylist.length;
}
}
const newPlaylist = item
? [fileToPlaylistItem(fullPath, item)]
: listingToPlaylistItems(fullPath, await listFiles(fullPath, props.isOffline(), recursive));
const addAction = () => {
if (newPlaylist.length !== 0 && oldLength === 0) props.playTrack(newPlaylist[0]);
if (newPlaylist.length !== 0 && oldPlaylist.length === 0) props.playTrack(newPlaylist[0]);
props.setPlaylist([...oldPlaylist, ...newPlaylist]);
addedToast.hideToast();
addedToast.options.text = `Added ${newPlaylist.length} tracks`;
@@ -262,7 +254,7 @@ export default function FileBrowser(props: FileBrowserProps) {
};
})
.filter((item) => {
return item.displayName.toLowerCase().includes(filter());
return item.displayName.toLowerCase().includes(filter().toLowerCase());
})
.sort(fileSort),
{ key: "key" },
▾Mwebclient/src/components/PlayerControls.tsx
@@ -279,6 +279,8 @@ export default function PlayerControls(props: PlayerControlsProps) {
width: `${props.options.progressBarHeight * 2}px`,
}}
onPointerDown={handlePointerDown}
//pointerup already seeked, the container's click handler would seek a second time
onClick={(e) => e.stopPropagation()}
ref={handleRef}
/>
</div>
▾Mwebclient/src/components/PlaylistManager.tsx
@@ -54,17 +54,18 @@ export default function PlaylistManager(props: PlaylistManagerProps) {
const target = event.target as HTMLInputElement;
const file = target.files?.[0];
if (file) {
toast("Importing playlists...", "info");
setLoadingActions((prev) => prev + 1);
try {
toast("Importing playlists...", "info");
setLoadingActions((prev) => prev + 1);
const imports = JSON.parse(await file.text()) as Playlist[];
for (const imported of imports) {
await savePlaylist(imported, true);
}
setLoadingActions((prev) => prev - 1);
fetchPlaylists.refetch();
} catch (e) {
toast("Importing playlists failed", "error", e);
} finally {
setLoadingActions((prev) => prev - 1);
fetchPlaylists.refetch();
}
}
};
▾Mwebclient/src/offline.ts
@@ -16,7 +16,16 @@ import { batch, from, type ResourceActions, untrack } from "solid-js";
import { createStore, unwrap } from "solid-js/store";
import type { FlatFileListing } from "./App";
import { type AppOptions, type LoadedVideoExtras, type Playlist, StreamingMode } from "./types";
import { basename, dirname, handleForbiddenPath, handleUnauthorized, joinPath, parentPaths, toast } from "./utils";
import {
basename,
dirname,
handleForbiddenPath,
handleUnauthorized,
joinPath,
parentPaths,
statusLabel,
toast,
} from "./utils";
interface StoredSubtitle extends SubtitleTrack {
data: Blob;
@@ -245,7 +254,7 @@ async function listOnlineFiles(dir: string, recursive: boolean): Promise<FileLis
}
if (!response?.ok) {
toast(
`Fetching files for ${dir === "" ? "root" : dir} failed${response ? `: ${response.statusText}` : ""}`,
`Fetching files for ${dir === "" ? "root" : dir} failed${response ? `: ${statusLabel(response)}` : ""}`,
"error",
);
return;
@@ -380,7 +389,9 @@ function videoInfoUrl(path: string): string {
function checkVideoExtrasResponse(response: Response | undefined, path: string): asserts response is Response {
if (handleUnauthorized(response)) throw new Error("Unauthorized while fetching video extras");
if (!response?.ok)
throw new Error(`Fetching video extras for ${path} failed: ${response?.statusText || "server unavailable"}`);
throw new Error(
`Fetching video extras for ${path} failed: ${response ? statusLabel(response) : "server unavailable"}`,
);
}
export async function loadVideoExtras(path: string, offline: boolean): Promise<LoadedVideoExtras> {
@@ -426,7 +437,7 @@ async function downloadVideoExtras(path: string, signal: AbortSignal): Promise<S
});
if (handleUnauthorized(subtitleResponse) || !subtitleResponse.ok) {
throw new Error(
`Fetching subtitle track ${track.streamIndex} for ${path} failed: ${subtitleResponse?.statusText || "server unavailable"}`,
`Fetching subtitle track ${track.streamIndex} for ${path} failed: ${statusLabel(subtitleResponse)}`,
);
}
return { ...track, data: await subtitleResponse.blob() };
@@ -529,10 +540,10 @@ export async function deleteFile(
item: MediaFile | Directory,
fetchFiles: ResourceActions<FlatFileListing>,
) {
//outside the try, the finally below must only undo a startProgress that actually ran
if ("metadata" in item && downloadStatusMap[fullPath] !== "Synced") return;
startProgress(fullPath, { amount: 1 });
try {
if ("metadata" in item && downloadStatusMap[fullPath] !== "Synced") return;
startProgress(fullPath, { amount: 1 });
let toDelete: string[];
if ("files" in item) {
toDelete = collectMediaFilePaths(listOfflineFiles(fullPath), fullPath);
@@ -593,14 +604,22 @@ async function downloadFile(
)
return;
//an abort that lands during db.files.put is followed by the success path, and the parent
//counters must only be decremented once
let progressStopped = false;
const stopOnce = (status: DownloadStatus) => {
if (progressStopped) {
setDownloadStatusMap(path, status);
return;
}
progressStopped = true;
stopProgress(path, status);
};
const controller = new AbortController();
const abortDownload = () => {
controller.abort();
stopProgress(
path,
// if it was already synced, restore it
initialProgress === "Synced" ? "Synced" : { status: "Aborted", retry },
);
// if it was already synced, restore it
stopOnce(initialProgress === "Synced" ? "Synced" : { status: "Aborted", retry });
};
startProgress(path, { status: 0, abort: abortDownload });
@@ -621,7 +640,7 @@ async function downloadFile(
});
handleUnauthorized(res);
if (!res.ok || !res.body) throw res.statusText;
if (!res.ok || !res.body) throw new Error(`${statusLabel(res)}: ${await res.text().catch(() => "")}`);
//read the body ourselves to track progress, and assemble the blob from the same chunks -
//res.clone().blob() would buffer the whole body a second time and reject unobserved on a mid-body error
const reader = res.body.getReader();
@@ -660,7 +679,8 @@ async function downloadFile(
await db.files.put(storedFile);
updateFileTree(path, item, true);
stopProgress(path, "Synced");
//the row is written, so this is synced even if an abort arrived during the put
stopOnce("Synced");
fetchFiles.refetch();
//iterate through all parent directories and download their covers to database
@@ -672,9 +692,8 @@ async function downloadFile(
}
} catch (e) {
if (e instanceof Error && e.name === "AbortError") return;
stopProgress(
path,
// if it was already synced, restore it
// if it was already synced, restore it
stopOnce(
initialProgress === "Synced"
? "Synced"
: {
▾Mwebclient/src/utils.ts
@@ -132,6 +132,11 @@ export function handleForbiddenPath(response?: Response | { status?: number } |
return true;
}
//HTTP/2 has no reason phrase, so statusText alone is empty behind most TLS proxies
export function statusLabel(response: Response): string {
return response.statusText ? `${response.status} ${response.statusText}` : `HTTP ${response.status}`;
}
export function formatBytes(bytes: number): string {
if (bytes === 0) return "Queued...";