bug: clip fps/resolution are admin-only operator settings

hasura/metadata/actions.graphql

@@ -899,7 +899,7 @@ input ClipSpecInput {
   segments: [ClipSegmentInput!]!
   overlays: [ClipOverlayInput!]
   audio: ClipAudioInput
-  output: ClipOutputInput!
+  output: ClipOutputInput
   destination: String!
   title: String
 }

hasura/migrations/default/1889000000400_admin_only_clip_settings/down.sql

@@ -0,0 +1,8 @@
+INSERT INTO public.settings (name, value)
+SELECT 'public.' || name, value
+  FROM public.settings
+ WHERE name IN ('clip_fps', 'clip_resolution')
+ON CONFLICT (name) DO UPDATE SET value = EXCLUDED.value;
+
+DELETE FROM public.settings
+ WHERE name IN ('clip_fps', 'clip_resolution');

hasura/migrations/default/1889000000400_admin_only_clip_settings/up.sql

@@ -0,0 +1,9 @@
+-- The `public.` row wins a clash: it is the value the api applied since 1889000000300.
+INSERT INTO public.settings (name, value)
+SELECT substring(name FROM length('public.') + 1), value
+  FROM public.settings
+ WHERE name IN ('public.clip_fps', 'public.clip_resolution')
+ON CONFLICT (name) DO UPDATE SET value = EXCLUDED.value;
+
+DELETE FROM public.settings
+ WHERE name IN ('public.clip_fps', 'public.clip_resolution');

src/hasura/hasura.service.ts

@@ -254,10 +254,10 @@ export class HasuraService {
       "delete from settings where name = 'public.utility_practice_daily_limit'",
     );
 
-    // Renamed to `public.` by migration 1889000000300; an admin still on the
-    // old web app writes the unprefixed names again, which the api ignores.
+    // Admin-only settings: a web app from before migration 1889000000400 still
+    // saves them under `public.`, which every role can read.
     await this.postgresService.query(
-      "delete from settings where name in ('clip_fps', 'clip_resolution')",
+      "delete from settings where name in ('public.clip_fps', 'public.clip_resolution')",
     );
 
     // The light-mode branding palette was retired; only `public.color_dark_*`

src/matches/game-streamer/game-streamer.service.ts

@@ -386,16 +386,12 @@ export class GameStreamerService {
     return value === "720p" ? "720p" : "1080p";
   }
 
-  // The render dialogs offer a per-clip resolution but no fps choice, so the
-  // operator's fps is final whatever the client sends.
-  public async resolveClipOutput(
-    resolution?: string,
-  ): Promise<{ resolution: "720p" | "1080p"; fps: 30 | 60 }> {
+  public async resolveClipOutput(): Promise<{
+    resolution: "720p" | "1080p";
+    fps: 30 | 60;
+  }> {
     return {
-      resolution:
-        resolution === "720p" || resolution === "1080p"
-          ? resolution
-          : await this.resolveClipResolution(),
+      resolution: await this.resolveClipResolution(),
       fps: await this.resolveClipFps(),
     };
   }

src/matches/matches.controller.spec.ts

@@ -274,10 +274,7 @@ describe("MatchesController", () => {
     const streamer = { role: "streamer", steam_id: "76561198000000001" };
     const admin = { role: "administrator", steam_id: "76561198000000002" };
 
-    const operatorSettings: Record<string, string> = {
-      "public.clip_fps": "30",
-      "public.clip_resolution": "720p",
-    };
+    let operatorSettings: Record<string, string>;
 
     const preset = (extra: Record<string, unknown> = {}) =>
       controller.createClipFromPreset({
@@ -291,6 +288,8 @@ describe("MatchesController", () => {
     const presetOutput = () => clips.buildPresetSpec.mock.calls[0][3];
 
     beforeEach(() => {
+      operatorSettings = { clip_fps: "30", clip_resolution: "720p" };
+
       const hasura = {
         query: jest.fn(
           async (query: { settings_by_pk?: { __args: { name: string } } }) => {
@@ -327,25 +326,32 @@ describe("MatchesController", () => {
       expect(presetOutput()).toEqual({ resolution: "720p", fps: 30 });
     });
 
-    it("keeps the operator's fps when the client sends its own", async () => {
-      await preset({ fps: 60 });
+    it("ignores clip settings saved under the `public.` names", async () => {
+      operatorSettings = {
+        "public.clip_fps": "30",
+        "public.clip_resolution": "720p",
+      };
 
-      expect(presetOutput()).toEqual({ resolution: "720p", fps: 30 });
+      await preset();
+
+      expect(presetOutput()).toEqual({ resolution: "1080p", fps: 60 });
     });
 
-    it("honours a resolution picked in the render dialog", async () => {
+    it("ignores the fps and resolution a client sends with a preset", async () => {
       await preset({ resolution: "1080p", fps: 60 });
 
-      expect(presetOutput()).toEqual({ resolution: "1080p", fps: 30 });
+      expect(presetOutput()).toEqual({ resolution: "720p", fps: 30 });
     });
 
-    it("falls back to the operator's resolution for one the dialog does not offer", async () => {
-      await preset({ resolution: "4k" });
+    it("keeps the operator's 1080p/60 over a client's 720p/30", async () => {
+      operatorSettings = { clip_fps: "60", clip_resolution: "1080p" };
 
-      expect(presetOutput()).toEqual({ resolution: "720p", fps: 30 });
+      await preset({ resolution: "720p", fps: 30 });
+
+      expect(presetOutput()).toEqual({ resolution: "1080p", fps: 60 });
     });
 
-    it("queues a highlight at the operator's fps", async () => {
+    it("queues a highlight at the operator's settings whatever the client sends", async () => {
       await controller.queueClipFromPreset({
         match_map_id: "map-1",
         target_steam_id: "76561198000000009",
@@ -356,25 +362,40 @@ describe("MatchesController", () => {
       } as any);
 
       expect(clips.queueClipFromPreset.mock.calls[0][1].output).toEqual({
-        resolution: "1080p",
+        resolution: "720p",
         fps: 30,
       });
     });
 
-    it("renders an edited clip at the operator's fps", async () => {
-      await controller.createClipRender({
+    const renderEdited = (extra: Record<string, unknown> = {}) =>
+      controller.createClipRender({
         spec: {
           match_map_id: "map-1",
           segments: [{ start_tick: 1, end_tick: 2 }],
-          output: { format: "mp4", resolution: "1080p", fps: 60 },
           destination: "library",
+          ...extra,
         },
         user: streamer as any,
       });
 
+    it("replaces the output a client sends with an edited clip", async () => {
+      await renderEdited({
+        output: { format: "mp4", resolution: "1080p", fps: 60 },
+      });
+
       expect(clips.createClipRender.mock.calls[0][1].output).toEqual({
         format: "mp4",
-        resolution: "1080p",
+        resolution: "720p",
+        fps: 30,
+      });
+    });
+
+    it("renders an edited clip sent without an output at the operator's settings", async () => {
+      await renderEdited();
+
+      expect(clips.createClipRender.mock.calls[0][1].output).toEqual({
+        format: "mp4",
+        resolution: "720p",
         fps: 30,
       });
     });

src/matches/matches.controller.ts

@@ -1945,18 +1945,24 @@ export class MatchesController {
   }
 
   @HasuraAction()
-  public async createClipRender(data: { spec: ClipSpec; user: User }) {
+  public async createClipRender(data: {
+    spec: Omit<ClipSpec, "output">;
+    user: User;
+  }) {
     const { spec, user } = data;
     if (!isRoleAbove(user.role, "streamer")) {
       throw Error("clip rendering requires the streamer role or above");
     }
     if (!spec || !spec.match_map_id) {
       throw Error("invalid clip spec");
     }
-    if (spec.output) {
-      spec.output.fps = await this.gameStreamer.resolveClipFps();
-    }
-    const { jobId } = await this.clips.createClipRender(user.steam_id, spec);
+    const { jobId } = await this.clips.createClipRender(user.steam_id, {
+      ...spec,
+      output: {
+        format: "mp4",
+        ...(await this.gameStreamer.resolveClipOutput()),
+      },
+    });
     return {
       success: true,
       job_id: jobId,
@@ -2081,7 +2087,6 @@ export class MatchesController {
     match_map_id: string;
     target_steam_id: string;
     preset: "knife" | "multikills" | "best_round" | "recap";
-    resolution?: string;
     title?: string;
     target_name?: string;
     user: User;
@@ -2094,7 +2099,7 @@ export class MatchesController {
       data.match_map_id,
       data.target_steam_id,
       data.preset,
-      await this.gameStreamer.resolveClipOutput(data.resolution),
+      await this.gameStreamer.resolveClipOutput(),
       data.title,
       data.target_name,
     );
@@ -2107,7 +2112,6 @@ export class MatchesController {
     match_map_id: string;
     target_steam_id: string;
     preset: "knife" | "multikills" | "best_round" | "recap";
-    resolution?: string;
     title?: string;
     target_name?: string;
     user: User;
@@ -2120,7 +2124,7 @@ export class MatchesController {
       matchMapId: data.match_map_id,
       targetSteamId: data.target_steam_id,
       preset: data.preset,
-      output: await this.gameStreamer.resolveClipOutput(data.resolution),
+      output: await this.gameStreamer.resolveClipOutput(),
       title: data.title,
       targetName: data.target_name,
     });

src/system/enums/SystemSettingName.ts

@@ -72,8 +72,8 @@ export enum SystemSettingName {
   GamePluginRegistryUrl = "game_plugin_registry_url",
   DefaultBroadcastHud = "public.default_broadcast_hud",
   DefaultHudMode = "default_hud_mode",
-  ClipFps = "public.clip_fps",
-  ClipResolution = "public.clip_resolution",
+  ClipFps = "clip_fps",
+  ClipResolution = "clip_resolution",
   // VAPID identifies this panel to the browser push services. The keypair is
   // self-generated -- there is no vendor to register with -- so it is stored
   // here rather than demanding an env var of every operator. The private half

test/public-clip-settings.spec.ts → test/admin-only-clip-settings.spec.ts

@@ -2,15 +2,16 @@ import { readFileSync } from "fs";
 import { join } from "path";
 import { bootMigratedDb, SqlTestDb } from "./utils/sql-test-db";
 
-describe("public clip settings migration", () => {
+describe("admin-only clip settings", () => {
   let db: SqlTestDb;
 
-  const migration = join(
-    __dirname,
-    "../hasura/migrations/default/1889000000300_public_clip_settings",
-  );
-  const up = readFileSync(join(migration, "up.sql"), "utf8");
-  const down = readFileSync(join(migration, "down.sql"), "utf8");
+  const migrations = join(__dirname, "../hasura/migrations/default");
+  const sql = (migration: string, file: "up" | "down") =>
+    readFileSync(join(migrations, migration, `${file}.sql`), "utf8");
+
+  const publicUp = sql("1889000000300_public_clip_settings", "up");
+  const up = sql("1889000000400_admin_only_clip_settings", "up");
+  const down = sql("1889000000400_admin_only_clip_settings", "down");
 
   const NAMES = [
     "clip_fps",
@@ -36,7 +37,7 @@ describe("public clip settings migration", () => {
     );
 
   beforeAll(async () => {
-    db = await bootMigratedDb("PublicClipSettingsTest");
+    db = await bootMigratedDb("AdminOnlyClipSettingsTest");
   }, 600_000);
 
   afterAll(async () => {
@@ -50,61 +51,83 @@ describe("public clip settings migration", () => {
     );
   });
 
-  it("moves the operator's values to the public names", async () => {
-    await set("clip_fps", "30");
-    await set("clip_resolution", "720p");
+  it("moves the operator's values back to the admin-only names", async () => {
+    await set("public.clip_fps", "30");
+    await set("public.clip_resolution", "720p");
 
     await db.postgres.query(up);
 
     expect(await rows()).toEqual({
-      "public.clip_fps": "30",
-      "public.clip_resolution": "720p",
+      clip_fps: "30",
+      clip_resolution: "720p",
     });
   });
 
-  it("keeps the operator's value over form defaults saved under the new name", async () => {
-    await set("clip_fps", "30");
-    await set("public.clip_fps", "60");
+  it("keeps the value the admin page saved under the public name on a clash", async () => {
+    await set("public.clip_fps", "30");
+    await set("clip_fps", "60");
 
     await db.postgres.query(up);
 
-    expect(await rows()).toEqual({ "public.clip_fps": "30" });
+    expect(await rows()).toEqual({ clip_fps: "30" });
   });
 
   it("changes nothing when run again", async () => {
+    await set("public.clip_fps", "30");
+    await set("public.clip_resolution", "720p");
+
+    await db.postgres.query(up);
+    await db.postgres.query(up);
+
+    expect(await rows()).toEqual({
+      clip_fps: "30",
+      clip_resolution: "720p",
+    });
+  });
+
+  it("keeps an install's value when it upgrades past both migrations at once", async () => {
     await set("clip_fps", "30");
     await set("clip_resolution", "720p");
 
+    await db.postgres.query(publicUp);
     await db.postgres.query(up);
-    await db.postgres.query(up);
+
+    expect(await rows()).toEqual({
+      clip_fps: "30",
+      clip_resolution: "720p",
+    });
+  });
+
+  it("restores the public names on down", async () => {
+    await set("clip_fps", "30");
+    await set("clip_resolution", "720p");
+
+    await db.postgres.query(down);
 
     expect(await rows()).toEqual({
       "public.clip_fps": "30",
       "public.clip_resolution": "720p",
     });
   });
 
-  it("drops unprefixed rows an old web app writes after the rename", async () => {
-    await set("public.clip_fps", "30");
-    await set("clip_fps", "60");
-    await set("clip_resolution", "1080p");
+  it("keeps the admin-only value on a clash when run down", async () => {
+    await set("clip_fps", "30");
+    await set("public.clip_fps", "60");
 
-    await (
-      db.hasura as unknown as { updateSettings(): Promise<void> }
-    ).updateSettings();
+    await db.postgres.query(down);
 
     expect(await rows()).toEqual({ "public.clip_fps": "30" });
   });
 
-  it("restores the old names on down", async () => {
-    await set("public.clip_fps", "30");
-    await set("public.clip_resolution", "720p");
+  it("drops public rows an old web app writes after the rename", async () => {
+    await set("clip_fps", "30");
+    await set("public.clip_fps", "60");
+    await set("public.clip_resolution", "1080p");
 
-    await db.postgres.query(down);
+    await (
+      db.hasura as unknown as { updateSettings(): Promise<void> }
+    ).updateSettings();
 
-    expect(await rows()).toEqual({
-      clip_fps: "30",
-      clip_resolution: "720p",
-    });
+    expect(await rows()).toEqual({ clip_fps: "30" });
   });
 });