Fix paired stereo review in the web interface (#1984)

* Fix web stereo review selection and camera configuration

* Review complete stereo sequences when selecting web camera folders

* prevent showing sub cameras as their own datasets in the picker

---------

Co-authored-by: Bryon Lewis <Bryon.Lewis@kitware.com>

Author: mattdawkins

client/dive-common/apispec.ts

@@ -480,6 +480,11 @@ interface Api {
    * dataset list the review page offers.
    */
   listScoringDatasets?(): Promise<ScoringDatasetSummary[]>;
+  /** Resolve a selected camera to its whole sequence before loading review. */
+  resolveReviewDatasetId?(datasetId: string): Promise<string>;
+  /** Review includes whole stereo/multicamera sequences, unlike scoring. */
+  listReviewDatasets?(): Promise<ScoringDatasetSummary[]>;
+  pickReviewDataset?(excludeIds: string[]): Promise<ScoringDatasetSummary | null>;
   /**
    * Open a platform dataset picker; returns null when the user cancels.
    * Shared by the scoring and review pages.

client/dive-common/components/Review/ReviewDatasetsPanel.vue

@@ -16,7 +16,8 @@ export default defineComponent({
     const api = useApi();
     const review = useReview();
     const picking = ref(false);
-    const usePicker = computed(() => typeof api.pickScoringDataset === 'function');
+    const pickDataset = api.pickReviewDataset ?? api.pickScoringDataset;
+    const usePicker = computed(() => typeof pickDataset === 'function');
 
     const selectedIds = computed(() => review.datasets.value.map((d) => d.id));
 
@@ -35,10 +36,10 @@ export default defineComponent({
     }
 
     async function openPicker() {
-      if (!api.pickScoringDataset || picking.value) return;
+      if (!pickDataset || picking.value) return;
       picking.value = true;
       try {
-        const picked = await api.pickScoringDataset(selectedIds.value);
+        const picked = await pickDataset(selectedIds.value);
         if (picked) await review.addDataset(picked.id, picked, { defer: true });
       } finally {
         picking.value = false;

client/dive-common/use/useReview.spec.ts

@@ -53,6 +53,16 @@ function makeApi(tracksById: Record<string, TrackData[]>, overrides: Partial<Rev
 }
 
 describe('createReviewService', () => {
+  it('uses the review-specific dataset list when a platform separates it from scoring', async () => {
+    const listReviewDatasets = vi.fn(async () => [{ id: 'rig', name: 'Stereo', type: 'multi' }]);
+    const api = makeApi({}, { listReviewDatasets });
+    const service = createReviewService({ api });
+    await service.refreshAvailable();
+    expect(service.available.value).toEqual([{ id: 'rig', name: 'Stereo', type: 'multi' }]);
+    expect(api.listScoringDatasets).not.toHaveBeenCalled();
+    service.dispose();
+  });
+
   it('uses the web tracks-only reader without loading unused annotation data', async () => {
     const loadReviewTracks = vi.fn(async () => [track(1, [['fish', 0.9]], [0])]);
     const api = makeApi({}, { loadReviewTracks });
@@ -91,6 +101,40 @@ describe('createReviewService', () => {
     expect(api.loadDetections).toHaveBeenCalledTimes(1);
   });
 
+  it('queues the resolved parent for a deferred camera pick and skips duplicates', async () => {
+    const resolveReviewDatasetId = vi.fn(async (id: string) => (id === 'leftFolder' ? 'rig' : id));
+    const api = makeApi({
+      'rig/left': [track(1, [['fish', 1]], [0])],
+      'rig/right': [track(1, [['fish', 1]], [0])],
+    }, {
+      resolveReviewDatasetId,
+      loadConfig: vi.fn(async (id: string) => (id === 'rig'
+        ? config('rig', {
+          type: 'multi',
+          name: 'Stereo',
+          multiCamMedia: {
+            defaultDisplay: 'left',
+            cameras: {
+              left: { type: 'image-sequence', imageData: [{ url: 'l.jpg', filename: 'l.jpg' }], videoUrl: '' },
+              right: { type: 'image-sequence', imageData: [{ url: 'r.jpg', filename: 'r.jpg' }], videoUrl: '' },
+            },
+          },
+        })
+        : config(id))),
+    });
+    const service = createReviewService({ api });
+    await service.addDataset('leftFolder', { id: 'leftFolder', name: 'left' }, { defer: true });
+    expect(resolveReviewDatasetId).toHaveBeenCalledWith('leftFolder');
+    expect(service.datasets.value).toMatchObject([{ id: 'rig', status: 'queued' }]);
+    expect(api.loadConfig).not.toHaveBeenCalled();
+    await service.loadQueued();
+    expect(service.datasets.value).toMatchObject([{ id: 'rig', status: 'ready' }]);
+    // Deferred browse of a camera folder must not sit beside the loaded rig.
+    await service.addDataset('leftFolder', { id: 'leftFolder', name: 'left' }, { defer: true });
+    expect(service.datasets.value).toEqual([expect.objectContaining({ id: 'rig', status: 'ready' })]);
+    service.dispose();
+  });
+
   it('loads datasets, prefers peekConfig, and builds items for a query', async () => {
     const peekConfig = vi.fn(async (id: string) => config(id));
     const api = makeApi({

client/dive-common/use/useReview.ts

@@ -1,3 +1,4 @@
+import { orderedMultiCamCameraNames } from 'dive-common/multicamDisplay';
 import { orderedHeadTail } from 'vue-media-annotator/headTail';
 /**
  * State behind the Review page: the datasets under review (with their
@@ -47,7 +48,8 @@ export interface ReviewGeometryEdit {
 
 export type ReviewApi = Pick<Api,
   'loadConfig' | 'peekConfig' | 'loadDetections' | 'loadReviewTracks' | 'saveDetections'
-  | 'listScoringDatasets' | 'pickScoringDataset'>;
+  | 'listScoringDatasets' | 'pickScoringDataset' | 'listReviewDatasets' | 'pickReviewDataset'
+  | 'resolveReviewDatasetId'>;
 
 export interface ReviewServiceDeps {
   api: ReviewApi;
@@ -322,9 +324,10 @@ function createScopedReviewService(deps: ReviewServiceDeps): ReviewService {
   }
 
   async function refreshAvailable() {
-    if (!api.listScoringDatasets) return;
+    const listDatasets = api.listReviewDatasets ?? api.listScoringDatasets;
+    if (!listDatasets) return;
     try {
-      const result = await requests.run(() => api.listScoringDatasets!());
+      const result = await requests.run(() => listDatasets());
       if (!disposed) available.value = result;
     } catch (err) {
       fail(err, 'Could not list datasets');
@@ -365,14 +368,28 @@ function createScopedReviewService(deps: ReviewServiceDeps): ReviewService {
     const isCurrent = () => loadTokens.get(id) === token && !!entry(id);
     loading.value = true;
     try {
+      // Normalize user selections, but keep expanded cameras separate internally
+      // so media, annotations, and writes continue using their own folders.
+      if (api.resolveReviewDatasetId && !memberships.has(id)) {
+        const resolvedId = await requests.run(() => api.resolveReviewDatasetId!(id));
+        if (!isCurrent()) return;
+        if (resolvedId !== id) {
+          datasets.value = datasets.value.filter((d) => d.id !== id);
+          await addDataset(resolvedId);
+          return;
+        }
+      }
       const config = await requests.run(() => {
         if (!isCurrent()) throw new Error('Dataset removed');
         return loadConfig(id);
       });
       if (!isCurrent()) return;
       if (config.type === 'multi') {
         // Load each camera separately while exposing the parent as one selected sequence.
-        const cameras = Object.keys(config.multiCamMedia?.cameras || {});
+        const cameras = [...new Set([
+          ...orderedMultiCamCameraNames(config.multiCamMedia),
+          ...Object.keys(config.multiCamMedia?.cameras || {}),
+        ])];
         const parentName = entry(id)?.name || config.name;
         if (!cameras.length) throw new Error('This sequence has no cameras');
         parentNames.set(id, parentName);
@@ -429,20 +446,36 @@ function createScopedReviewService(deps: ReviewServiceDeps): ReviewService {
   /**
    * Add a dataset; with `defer` it only joins the list and loads on the
    * next `loadQueued`, so picking many datasets costs nothing until the
-   * results are actually wanted.
+   * results are actually wanted. Deferred picks still resolve camera folders
+   * to their sequence so a browse pick cannot sit beside an already-loaded rig.
    */
   async function addDataset(id: string, summary?: ScoringDatasetSummary, options: { defer?: boolean } = {}) {
     if (disposed || !id || entry(id) || selectedDatasets.value.some((dataset) => dataset.id === id)) return;
+    let selectedId = id;
+    let selectedSummary = summary;
+    if (options.defer && api.resolveReviewDatasetId) {
+      try {
+        const resolvedId = await api.resolveReviewDatasetId(id);
+        if (resolvedId !== id) {
+          selectedId = resolvedId;
+          // Drop the camera-folder summary; the parent owns the sequence name.
+          selectedSummary = undefined;
+        }
+      } catch {
+        // Keep the original id; load() will surface the error.
+      }
+    }
+    if (entry(selectedId) || selectedDatasets.value.some((dataset) => dataset.id === selectedId)) return;
     datasets.value = [...datasets.value, {
-      id,
-      name: summary?.name || datasetName(id),
-      type: summary?.type,
+      id: selectedId,
+      name: selectedSummary?.name || datasetName(selectedId),
+      type: selectedSummary?.type,
       status: options.defer ? 'queued' : 'loading',
       trackCount: 0,
       croppable: false,
     }];
     if (options.defer) return;
-    await load(id);
+    await load(selectedId);
   }
 
   /** Load every queued dataset; annotations are read and the query rerun as each arrives. */

client/platform/web-girder/App.vue

@@ -50,9 +50,12 @@ import {
   deleteScoringResult,
   listScoringSources,
   listScoringDatasets,
+  listReviewDatasets,
+  resolveReviewDatasetId,
   saveScoringExport,
   exportScoringPdf,
   pickScoringDataset,
+  pickReviewDataset,
 } from './api';
 import ScoringDatasetPickerDialog from './components/ScoringDatasetPickerDialog.vue';
 import {
@@ -127,7 +130,10 @@ export default defineComponent({
       deleteScoringResult,
       listScoringSources: unwrap(listScoringSources),
       listScoringDatasets,
+      listReviewDatasets,
+      resolveReviewDatasetId,
       pickScoringDataset,
+      pickReviewDataset,
       saveScoringExport,
       exportScoringPdf,
     });

client/platform/web-girder/api/dataset.service.ts

@@ -107,11 +107,23 @@ function mergeDatasetConfig(
  */
 async function loadDatasetConfig(datasetId: string): Promise<GirderConfig> {
   const { compositeId } = await resolveDatasetFolderId(datasetId);
-  const [metaStatic, media] = await Promise.all([
+  const [metaStatic, media, parentConfig] = await Promise.all([
     getDataset(datasetId),
     getDatasetMedia(datasetId),
+    compositeId
+      ? girderRest.get<DatasetConfigMutable>(`dive_dataset/${parentDatasetId(datasetId)}/configuration`)
+      : Promise.resolve(null),
   ]);
-  return mergeDatasetConfig(metaStatic.data, media.data, compositeId);
+  const config = mergeDatasetConfig(metaStatic.data, media.data, compositeId);
+  if (parentConfig) {
+    // The parent owns the shared hierarchy. In particular, an absent parent
+    // hierarchy must not revive obsolete edges stored on a camera folder.
+    config.typeHierarchy = parentConfig.data.typeHierarchy;
+    config.customTypeStyling = {
+      ...config.customTypeStyling, ...parentConfig.data.customTypeStyling,
+    };
+  }
+  return config;
 }
 
 function clone({

client/platform/web-girder/api/index.ts

@@ -13,7 +13,7 @@ export * from './girder.service';
 export * from './multicamResolve';
 export * from './rpc.service';
 export * from './scoring.service';
-export { pickScoringDataset } from './scoringDatasetPicker';
+export { pickScoringDataset, pickReviewDataset } from './scoringDatasetPicker';
 export * from './waitForFolderDatasetReady';
 export { default as watchPipelineJob, watchScoringJob } from './watchPipelineJob';
 export * from './largeImage.service';

client/platform/web-girder/api/multicamResolve.spec.ts

@@ -11,6 +11,7 @@ import {
   clearMultiCamMetaCache,
   parseCompositeDatasetId,
   resolveDatasetFolderId,
+  resolveReviewDatasetId,
 } from './multicamResolve';
 
 describe('multicamResolve', () => {
@@ -75,4 +76,32 @@ describe('multicamResolve', () => {
       'Unknown camera "missing"',
     );
   });
+
+  it('keeps a standalone dataset under an ordinary folder', async () => {
+    vi.spyOn(girderRest, 'get')
+      .mockResolvedValueOnce({ data: { parentId: 'ordinary', parentCollection: 'folder' } } as never)
+      .mockResolvedValueOnce({ data: { meta: {} } } as never);
+    expect(await resolveReviewDatasetId('standalone')).toBe('standalone');
+  });
+
+  it('does not include unrelated datasets nested beneath a stereo parent', async () => {
+    vi.spyOn(girderRest, 'get')
+      .mockResolvedValueOnce({ data: { parentId: 'rig', parentCollection: 'folder' } } as never)
+      .mockResolvedValueOnce({
+        data: {
+          meta: {
+            type: 'multi', multiCam: { cameras: { left: { folderId: 'left' } } },
+          },
+        },
+      } as never);
+    expect(await resolveReviewDatasetId('unrelated')).toBe('unrelated');
+  });
+
+  it('does not resolve a collection id as a folder', async () => {
+    const get = vi.spyOn(girderRest, 'get').mockResolvedValueOnce({
+      data: { parentId: 'collection', parentCollection: 'collection' },
+    } as never);
+    expect(await resolveReviewDatasetId('standalone')).toBe('standalone');
+    expect(get).toHaveBeenCalledTimes(1);
+  });
 });

client/platform/web-girder/api/multicamResolve.ts

@@ -56,3 +56,25 @@ export function clearMultiCamMetaCache(parentId?: string): void {
     multiCamMetaCache.clear();
   }
 }
+
+/** Review a whole rig even when entered from a camera folder or viewer link. */
+export async function resolveReviewDatasetId(datasetId: string): Promise<string> {
+  const { parentId, cameraName } = parseCompositeDatasetId(datasetId);
+  if (cameraName) return parentId;
+  const { data: folder } = await girderRest.get<{
+    parentId?: string;
+    parentCollection?: string;
+    meta?: { type?: string };
+  }>(`folder/${datasetId}`);
+  if (folder.meta?.type === 'multi' || folder.parentCollection !== 'folder' || !folder.parentId) {
+    return datasetId;
+  }
+  const { data: parent } = await girderRest.get<{
+    meta?: { type?: string; multiCam?: MultiCamStorageMeta };
+  }>(`folder/${folder.parentId}`);
+  const cameras = parent.meta?.multiCam?.cameras;
+  // Only registered cameras belong to the sequence.
+  return parent.meta?.type === 'multi'
+    && Object.values(cameras ?? {}).some((camera) => camera.folderId === datasetId)
+    ? folder.parentId : datasetId;
+}