Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

timeline fix: loadrange in imavid #4857

Merged
merged 11 commits into from
Sep 27, 2024
114 changes: 78 additions & 36 deletions app/packages/core/src/components/Modal/ImaVidLooker.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { useTheme } from "@fiftyone/components";
import { AbstractLooker, ImaVidLooker } from "@fiftyone/looker";
import { BUFFERING_PAUSE_TIMEOUT } from "@fiftyone/looker/src/lookers/imavid/constants";
import { BaseState } from "@fiftyone/looker/src/state";
import { FoTimelineConfig, useCreateTimeline } from "@fiftyone/playback";
import { useDefaultTimelineNameImperative } from "@fiftyone/playback/src/lib/use-default-timeline-name";
Expand Down Expand Up @@ -54,6 +53,8 @@ export const ImaVidLookerReact = React.memo(
});

const { activeLookerRef, setActiveLookerRef } = useModalContext();
const imaVidLookerRef =
activeLookerRef as unknown as React.MutableRefObject<ImaVidLooker>;
sashankaryal marked this conversation as resolved.
Show resolved Hide resolved

const looker = React.useMemo(
() => createLooker.current(sampleDataWithExtraParams),
Expand Down Expand Up @@ -152,19 +153,61 @@ export const ImaVidLookerReact = React.memo(
);
}, [ref]);

const loadRange = React.useCallback(async (range: BufferRange) => {
// todo: implement
return new Promise<void>((resolve) => {
setTimeout(() => {
resolve();
}, BUFFERING_PAUSE_TIMEOUT);
});
}, []);
const loadRange = React.useCallback(
async (range: Readonly<BufferRange>) => {
if (
imaVidLookerRef.current.frameStoreController.storeBufferManager.containsRange(
range
)
) {
return;
}

const unprocessedBufferRange =
imaVidLookerRef.current.frameStoreController.fetchBufferManager.getUnprocessedBufferRange(
imaVidLookerRef.current.frameStoreController.storeBufferManager.getUnprocessedBufferRange(
range
)
);

if (!unprocessedBufferRange) {
return;
}

imaVidLookerRef.current.frameStoreController.enqueueFetch(
unprocessedBufferRange
);

return new Promise<void>((resolve) => {
const fetchMoreListener = (e: CustomEvent) => {
if (
e.detail.id === imaVidLookerRef.current.frameStoreController.key
) {
if (
imaVidLookerRef.current.frameStoreController.storeBufferManager.containsRange(
unprocessedBufferRange
)
) {
resolve();
window.removeEventListener(
"fetchMore",
fetchMoreListener as EventListener
);
}
}
};

window.addEventListener(
"fetchMore",
fetchMoreListener as EventListener
);
});
},
sashankaryal marked this conversation as resolved.
Show resolved Hide resolved
[]
);
sashankaryal marked this conversation as resolved.
Show resolved Hide resolved

const renderFrame = React.useCallback((frameNumber: number) => {
(
activeLookerRef.current as unknown as ImaVidLooker
)?.element.drawFrameNoAnimation(frameNumber);
imaVidLookerRef.current?.element.drawFrameNoAnimation(frameNumber);
}, []);

const { getName } = useDefaultTimelineNameImperative();
Expand All @@ -189,7 +232,7 @@ export const ImaVidLookerReact = React.memo(

const readyWhen = useCallback(async () => {
return new Promise<void>((resolve) => {
// wait for total frame count to be resolved
// hack: wait for total frame count to be resolved
let intervalId;
intervalId = setInterval(() => {
if (totalFrameCountRef.current) {
Expand All @@ -200,6 +243,10 @@ export const ImaVidLookerReact = React.memo(
});
}, []);

const onAnimationStutter = useCallback(() => {
imaVidLookerRef.current?.element.checkFetchBufferManager();
}, []);

const {
isTimelineInitialized,
registerOnPauseCallback,
Expand All @@ -210,6 +257,10 @@ export const ImaVidLookerReact = React.memo(
name: timelineName,
config: timelineCreationConfig,
waitUntilInitialized: readyWhen,
// using this mechanism to resume fetch if it was paused
// ideally we have control of fetch in this component but can't do that yet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the grid moving to react? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely haven't thought about that one way or the other. 😅

// since imavid is part of the grid too
onAnimationStutter,
});

/**
Expand All @@ -224,35 +275,27 @@ export const ImaVidLookerReact = React.memo(
});

registerOnPlayCallback(() => {
(activeLookerRef.current as unknown as ImaVidLooker).element.update(
() => ({
playing: true,
})
);
imaVidLookerRef.current.element.update(() => ({
playing: true,
}));
});

registerOnPauseCallback(() => {
(activeLookerRef.current as unknown as ImaVidLooker).element.update(
() => ({
playing: false,
})
);
imaVidLookerRef.current.element.update(() => ({
playing: false,
}));
});

registerOnSeekCallbacks({
start: () => {
(activeLookerRef.current as unknown as ImaVidLooker).element.update(
() => ({
seeking: true,
})
);
imaVidLookerRef.current.element.update(() => ({
seeking: true,
}));
},
end: () => {
(activeLookerRef.current as unknown as ImaVidLooker).element.update(
() => ({
seeking: false,
})
);
imaVidLookerRef.current.element.update(() => ({
seeking: false,
}));
sashankaryal marked this conversation as resolved.
Show resolved Hide resolved
},
});
}
Expand All @@ -265,9 +308,8 @@ export const ImaVidLookerReact = React.memo(
// hack: poll every 10ms for total frame count
// replace with event listener or callback
let intervalId = setInterval(() => {
const totalFrameCount = (
activeLookerRef.current as unknown as ImaVidLooker
).frameStoreController.totalFrameCount;
const totalFrameCount =
imaVidLookerRef.current.frameStoreController.totalFrameCount;
if (totalFrameCount) {
setTotalFrameCount(totalFrameCount);
clearInterval(intervalId);
Expand Down
21 changes: 20 additions & 1 deletion app/packages/looker/src/elements/imavid/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,8 @@ export class ImaVidElement extends BaseElement<ImaVidState, HTMLImageElement> {
/**
* Queue up frames to be fetched if necessary.
* This method is not blocking, it merely enqueues a fetch job.
*
* This is for legacy imavid, which is used for thumbnail imavid.
*/
private ensureBuffers(state: Readonly<ImaVidState>) {
if (!this.framesController.totalFrameCount) {
Expand Down Expand Up @@ -407,6 +409,19 @@ export class ImaVidElement extends BaseElement<ImaVidState, HTMLImageElement> {
}
}

/**
* Starts fetch if there are buffers in the fetch buffer manager
*/
public checkFetchBufferManager() {
if (!this.framesController.totalFrameCount) {
return;
}

if (this.framesController.fetchBufferManager.buffers.length > 0) {
this.framesController.resumeFetch();
}
}

renderSelf(state: Readonly<ImaVidState>) {
const {
options: { playbackRate, loop },
Expand Down Expand Up @@ -443,7 +458,11 @@ export class ImaVidElement extends BaseElement<ImaVidState, HTMLImageElement> {
this.framesController.destroy();
}

this.ensureBuffers(state);
if (this.isThumbnail) {
this.ensureBuffers(state);
} else {
this.checkFetchBufferManager();
}

if (!playing && this.isAnimationActive) {
// this flag will be picked up in `drawFrame`, that in turn will call `pause`
Expand Down
45 changes: 29 additions & 16 deletions app/packages/looker/src/lookers/imavid/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ export class ImaVidFramesController {
BUFFER_METADATA_FETCHING
);

// subtract by two because 1) cursor is one based and 2) cursor here translates to "after" the cursor
return this.fetchMore(range[0] - 2, range[1] - range[0] || 2).finally(
// subtract/add by two because 1) cursor is one based and 2) cursor here translates to "after" the cursor
return this.fetchMore(range[0] - 2, range[1] - range[0] + 2 || 2).finally(
sashankaryal marked this conversation as resolved.
Show resolved Hide resolved
() => {
this.fetchBufferManager.removeMetadataFromBufferRange(index);
}
Expand Down Expand Up @@ -165,7 +165,7 @@ export class ImaVidFramesController {
return this.config.page;
}

private get key() {
public get key() {
return this.config.key;
}

Expand Down Expand Up @@ -257,23 +257,36 @@ export class ImaVidFramesController {
const frameIndices = imageFetchPromisesMap.keys();
const imageFetchPromises = imageFetchPromisesMap.values();

Promise.all(imageFetchPromises).then((sampleIds) => {
for (let i = 0; i < sampleIds.length; i++) {
const frameIndex = frameIndices.next().value;
const sampleId = sampleIds[i];
this.store.frameIndex.set(frameIndex, sampleId);
this.store.reverseFrameIndex.set(sampleId, frameIndex);

this.storeBufferManager.addNewRange([
Promise.all(imageFetchPromises)
.then((sampleIds) => {
for (let i = 0; i < sampleIds.length; i++) {
const frameIndex = frameIndices.next().value;
const sampleId = sampleIds[i];
this.store.frameIndex.set(frameIndex, sampleId);
this.store.reverseFrameIndex.set(sampleId, frameIndex);

resolve();
}
})
.then(() => {
const newRange = [
Number(data.samples.edges[0].cursor) + 1,
Number(
data.samples.edges[data.samples.edges.length - 1].cursor
) + 1,
]);

resolve();
}
});
] as BufferRange;

this.storeBufferManager.addNewRange(newRange);

window.dispatchEvent(
new CustomEvent("fetchMore", {
detail: {
id: this.key,
},
bubbles: false,
})
);
});
sashankaryal marked this conversation as resolved.
Show resolved Hide resolved
}
},
});
Expand Down
15 changes: 11 additions & 4 deletions app/packages/playback/src/lib/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@ export type CreateFoTimeline = {
* If true, the creator will be responsible for managing the animation loop.
*/
optOutOfAnimation?: boolean;

/**
* Callback to be called when the animation stutters.
*/
onAnimationStutter?: () => void;
};

const _frameNumbers = atomFamily((_timelineName: TimelineName) =>
Expand Down Expand Up @@ -311,6 +316,8 @@ export const setFrameNumberAtom = atom(
newFrameNumber: FrameNumber;
}
) => {
console.log(">>>SFNATOM SUGGESTION", newFrameNumber);

const subscribers = get(_subscribers(name));

if (!subscribers) {
Expand Down Expand Up @@ -339,7 +346,7 @@ export const setFrameNumberAtom = atom(
set(_currentBufferingRange(name), newLoadRange);

try {
await Promise.all(rangeLoadPromises);
await Promise.allSettled(rangeLoadPromises);
bufferManager.addNewRange(newLoadRange);
} catch (e) {
// todo: handle error better, maybe retry
Expand All @@ -358,9 +365,9 @@ export const setFrameNumberAtom = atom(
renderPromises.push(subscriber.renderFrame(newFrameNumber));
});

Promise.all(renderPromises).then(() => {
set(_frameNumbers(name), newFrameNumber);
});
await Promise.allSettled(renderPromises);
console.log(">>>SFNATOM FINAL", newFrameNumber);
sashankaryal marked this conversation as resolved.
Show resolved Hide resolved
set(_frameNumbers(name), newFrameNumber);
}
);

Expand Down
16 changes: 15 additions & 1 deletion app/packages/playback/src/lib/use-create-timeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ export const useCreateTimeline = (
const isAnimationActiveRef = useRef(false);
const isLastDrawFinishedRef = useRef(true);
const frameNumberRef = useRef(frameNumber);
const onAnimationStutterRef = useRef(newTimelineProps.onAnimationStutter);
sashankaryal marked this conversation as resolved.
Show resolved Hide resolved
const onPlayListenerRef = useRef<() => void>();
const onPauseListenerRef = useRef<() => void>();
const onSeekCallbackRefs = useRef<{ start: () => void; end: () => void }>();
Expand All @@ -145,11 +146,15 @@ export const useCreateTimeline = (
const updateFreqRef = useRef(updateFreq);

const play = useCallback(() => {
if (!isTimelineInitialized) {
return;
}

sashankaryal marked this conversation as resolved.
Show resolved Hide resolved
setPlayHeadState({ name: timelineName, state: "playing" });
if (onPlayListenerRef.current) {
onPlayListenerRef.current();
}
}, [timelineName]);
}, [timelineName, isTimelineInitialized]);

const pause = useCallback(() => {
setPlayHeadState({ name: timelineName, state: "paused" });
Expand Down Expand Up @@ -258,20 +263,28 @@ export const useCreateTimeline = (
// queue next animation before draw
animationId.current = requestAnimationFrame(animate);

// usually happens when we're out of frames in store
if (!isLastDrawFinishedRef.current) {
queueMicrotask(() => {
onAnimationStutterRef.current?.();
});
return;
}

// drawing logic is owned by subscribers and invoked by setFrameNumber
// we don't increase frame number until the draw is complete
isLastDrawFinishedRef.current = false;

setFrameNumber({
name: timelineName,
newFrameNumber: targetFrameNumber,
})
.then(() => {
frameNumberRef.current = targetFrameNumber;
})
.catch((e) => {
console.error("error setting frame number", e);
})
sashankaryal marked this conversation as resolved.
Show resolved Hide resolved
.finally(() => {
isLastDrawFinishedRef.current = true;
});
Expand All @@ -292,6 +305,7 @@ export const useCreateTimeline = (
const cancelAnimation = useCallback(() => {
cancelAnimationFrame(animationId.current);
isAnimationActiveRef.current = false;
lastDrawTime.current = -1;
}, []);

useEventHandler(window, "play", onPlayEvent);
Expand Down
Loading
Loading