Skip to content

Commit

Permalink
Fix an issue with deleting frames (#8872)
Browse files Browse the repository at this point in the history
<!-- Raise an issue to propose your change
(https://github.com/cvat-ai/cvat/issues).
It helps to avoid duplication of efforts from multiple independent
contributors.
Discuss your ideas with maintainers to be sure that changes will be
approved and merged.
Read the [Contribution guide](https://docs.cvat.ai/docs/contributing/).
-->

<!-- Provide a general summary of your changes in the Title above -->

### Motivation and context
<!-- Why is this change required? What problem does it solve? If it
fixes an open
issue, please link to the issue here. Describe your changes in detail,
add
screenshots. -->

Previously, in the Honeypots patch, we introduced periodic metadata
updates to allow the UI to reflect chunk changes dynamically. However,
this introduced a couple of issues:

1. The object received from the server was incorrectly utilized by the
UI, preventing frame deletion after requesting updated metadata.
2. The new object overwritten unsaved deleted frame data, leading to the
loss of user data.

### Steps to Reproduce the Frame Deletion Issue  
1. Set `jobMetaDataReloadPeriod` in `config.ts` of `cvat-core` to a
small interval, such as 15 seconds (the default is 1 hour).
2. Open a job and ensure the `/data/meta` request is sent.  
3. Wait for the `jobMetaDataReloadPeriod` to elapse.  
4. Change the frame and confirm that a new `/data/meta` request is sent.
5. Attempt to delete any frame and observe that it is not deleted.  

![deleting-frame](https://github.com/user-attachments/assets/26357b3d-9c9e-4639-bd51-3fa8f771b055)

### Resolution  
To fix this, I removed the secondary storage of `jobMeta` in
`frameDataCache`. Now, it uses an asynchronous getter that fetches data
directly from `frameMetaCache`. Having two caches for the same data
caused synchronization issues, which led to this bug.

To fix second issue I added merge logic inside getFramesMeta function
that will update new object with unsaved data.

### How has this been tested?
<!-- Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc. -->

### Checklist
<!-- Go over all the following points, and put an `x` in all the boxes
that apply.
If an item isn't applicable for some reason, then ~~explicitly
strikethrough~~ the whole
line. If you don't do that, GitHub will show incorrect progress for the
pull request.
If you're unsure about any of these, don't hesitate to ask. We're here
to help! -->
- [x] I submit my changes into the `develop` branch
- [ ] I have created a changelog fragment <!-- see top comment in
CHANGELOG.md -->
- ~~[ ] I have updated the documentation accordingly~~
- [ ] I have added tests to cover my changes
- [ ] I have linked related issues (see [GitHub docs](

https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword))
- [x] I have increased versions of npm packages if it is necessary

([cvat-canvas](https://github.com/cvat-ai/cvat/tree/develop/cvat-canvas#versioning),

[cvat-core](https://github.com/cvat-ai/cvat/tree/develop/cvat-core#versioning),

[cvat-data](https://github.com/cvat-ai/cvat/tree/develop/cvat-data#versioning)
and

[cvat-ui](https://github.com/cvat-ai/cvat/tree/develop/cvat-ui#versioning))

### License

- [x] I submit _my code changes_ under the same [MIT License](
https://github.com/cvat-ai/cvat/blob/develop/LICENSE) that covers the
project.
  Feel free to contact the maintainers if that's a concern.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
- Introduced a new enum to standardize the representation of deleted
frame states.
	- Enhanced metadata retrieval efficiency with updated caching logic.

- **Bug Fixes**
- Improved clarity and maintainability in handling the deletion status
of frames.

- **Refactor**
- Simplified logic for marking frames as deleted or restored using the
new enum.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
klakhov authored Dec 29, 2024
1 parent 9a25291 commit 26f08ce
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 33 deletions.
124 changes: 92 additions & 32 deletions cvat-core/src/frames.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import config from './config';

// frame storage by job id
const frameDataCache: Record<string, {
meta: FramesMetaData;
metaFetchedTimestamp: number;
chunkSize: number;
mode: 'annotation' | 'interpolation';
Expand All @@ -36,11 +35,21 @@ const frameDataCache: Record<string, {
size: number;
}>;
getChunk: (chunkIndex: number, quality: ChunkQuality) => Promise<ArrayBuffer>;
getMeta: () => Promise<FramesMetaData>;
}> = {};

// frame meta data storage by job id
const frameMetaCache: Record<string, Promise<FramesMetaData>> = {};

enum DeletedFrameState {
DELETED = 'deleted',
RESTORED = 'restored',
}

interface FramesMetaDataUpdatedData {
deletedFrames: Record<number, DeletedFrameState>;
}

export class FramesMetaData {
public chunkSize: number;
public deletedFrames: Record<number, boolean>;
Expand Down Expand Up @@ -82,10 +91,13 @@ export class FramesMetaData {
if (Object.prototype.hasOwnProperty.call(data, property) && property in initialData) {
if (property === 'deleted_frames') {
const update = (frame: string, remove: boolean): void => {
if (this.#updateTrigger.get(`deletedFrames:${frame}:${!remove}`)) {
this.#updateTrigger.resetField(`deletedFrames:${frame}:${!remove}`);
const [state, oppositeState] = remove ?
[DeletedFrameState.DELETED, DeletedFrameState.RESTORED] :
[DeletedFrameState.RESTORED, DeletedFrameState.DELETED];
if (this.#updateTrigger.get(`deletedFrames:${frame}:${oppositeState}`)) {
this.#updateTrigger.resetField(`deletedFrames:${frame}:${oppositeState}`);
} else {
this.#updateTrigger.update(`deletedFrames:${frame}:${remove}`);
this.#updateTrigger.update(`deletedFrames:${frame}:${state}`);
}
};

Expand Down Expand Up @@ -178,8 +190,17 @@ export class FramesMetaData {
return (dataFrameNumber - this.startFrame) / this.frameStep;
}

getUpdated(): Record<string, unknown> {
return this.#updateTrigger.getUpdated(this);
getUpdated(): FramesMetaDataUpdatedData {
const updatedFields = this.#updateTrigger.getUpdated(this);
const deletedFrames: FramesMetaDataUpdatedData['deletedFrames'] = {};
for (const key in updatedFields) {
if (Object.hasOwn(updatedFields, key) && key.startsWith('deletedFrames')) {
const [, frame, state] = key.split(':');
deletedFrames[frame] = state;
}
}

return { deletedFrames };
}

resetUpdated(): void {
Expand Down Expand Up @@ -340,17 +361,18 @@ class PrefetchAnalyzer {
}

Object.defineProperty(FrameData.prototype.data, 'implementation', {
value(this: FrameData, onServerRequest) {
async value(this: FrameData, onServerRequest) {
const {
provider, prefetchAnalyzer, chunkSize, jobStartFrame,
decodeForward, forwardStep, decodedBlocksCacheSize,
} = frameDataCache[this.jobID];
const meta = await frameDataCache[this.jobID].getMeta();

return new Promise<{
renderWidth: number;
renderHeight: number;
imageData: ImageBitmap | Blob;
} | Blob>((resolve, reject) => {
const {
meta, provider, prefetchAnalyzer, chunkSize, jobStartFrame,
decodeForward, forwardStep, decodedBlocksCacheSize,
} = frameDataCache[this.jobID];

const requestId = +_.uniqueId();
const requestedDataFrameNumber = meta.getDataFrameNumber(this.number - jobStartFrame);
const chunkIndex = meta.getFrameChunkIndex(requestedDataFrameNumber);
Expand Down Expand Up @@ -536,6 +558,34 @@ Object.defineProperty(FrameData.prototype.data, 'implementation', {
writable: false,
});

function mergeMetaData(
nextData: SerializedFramesMetaData,
previousData?: Promise<FramesMetaData>,
): Promise<FramesMetaData> {
const framesMetaData = new FramesMetaData({
...nextData,
deleted_frames: Object.fromEntries(nextData.deleted_frames.map((_frame) => [_frame, true])),
});

if (previousData instanceof Promise) {
return previousData.then((prevMeta) => {
const updatedFields = prevMeta.getUpdated();
const updatedDeletedFrames = updatedFields.deletedFrames;
for (const [frame, state] of Object.entries(updatedDeletedFrames)) {
if (state === DeletedFrameState.DELETED) {
framesMetaData.deletedFrames[frame] = true;
} else if (state === DeletedFrameState.RESTORED) {
delete framesMetaData.deletedFrames[frame];
}
}

return framesMetaData;
});
}

return Promise.resolve(framesMetaData);
}

export function getFramesMeta(type: 'job' | 'task', id: number, forceReload = false): Promise<FramesMetaData> {
if (type === 'task') {
// we do not cache task meta currently. So, each new call will results to the server request
Expand All @@ -551,11 +601,11 @@ export function getFramesMeta(type: 'job' | 'task', id: number, forceReload = fa
const previousCache = frameMetaCache[id];
frameMetaCache[id] = new Promise((resolve, reject) => {
serverProxy.frames.getMeta('job', id).then((serialized) => {
const framesMetaData = new FramesMetaData({
...serialized,
deleted_frames: Object.fromEntries(serialized.deleted_frames.map((_frame) => [_frame, true])),
// When we get new framesMetaData from server there can be some unsaved data
// here we merge new meta data with cached one
mergeMetaData(serialized, previousCache).then((mergedData) => {
resolve(mergedData);
});
resolve(framesMetaData);
}).catch((error: unknown) => {
delete frameMetaCache[id];
if (previousCache instanceof Promise) {
Expand Down Expand Up @@ -588,8 +638,9 @@ function saveJobMeta(meta: FramesMetaData, jobID: number): Promise<FramesMetaDat
return frameMetaCache[jobID];
}

function getFrameMeta(jobID, frame): SerializedFramesMetaData['frames'][0] {
const { meta, mode, jobStartFrame } = frameDataCache[jobID];
async function getFrameMeta(jobID, frame): Promise<SerializedFramesMetaData['frames'][0]> {
const { mode, jobStartFrame } = frameDataCache[jobID];
const meta = await frameDataCache[jobID].getMeta();
let frameMeta = null;
if (mode === 'interpolation' && meta.frames.length === 1) {
// video tasks have 1 frame info, but image tasks will have many infos
Expand All @@ -616,12 +667,12 @@ async function refreshJobCacheIfOutdated(jobID: number): Promise<void> {

if (isOutdated) {
// get metadata again if outdated
const prevMeta = await cached.getMeta();
const meta = await getFramesMeta('job', jobID, true);
if (new Date(meta.chunksUpdatedDate) > new Date(cached.meta.chunksUpdatedDate)) {
if (new Date(meta.chunksUpdatedDate) > new Date(prevMeta.chunksUpdatedDate)) {
// chunks were re-defined. Existing data not relevant anymore
// currently we only re-write meta, remove all cached frames from provider and clear cached context images
// other parameters (e.g. chunkSize) are not supposed to be changed
cached.meta = meta;
cached.provider.cleanup(Number.MAX_SAFE_INTEGER);
for (const frame of Object.keys(cached.contextCache)) {
for (const image of Object.values(cached.contextCache[+frame].data)) {
Expand All @@ -636,19 +687,19 @@ async function refreshJobCacheIfOutdated(jobID: number): Promise<void> {
}
}

export function getContextImage(jobID: number, frame: number): Promise<Record<string, ImageBitmap>> {
export async function getContextImage(jobID: number, frame: number): Promise<Record<string, ImageBitmap>> {
const frameData = frameDataCache[jobID];
const meta = await frameData.getMeta();
const requestId = frame;
const { jobStartFrame } = frameData;
const { related_files: relatedFiles } = meta.frames[frame - jobStartFrame];
return new Promise<Record<string, ImageBitmap>>((resolve, reject) => {
if (!(jobID in frameDataCache)) {
reject(new Error(
'Frame data was not initialized for this job. Try first requesting any frame.',
));
}

const frameData = frameDataCache[jobID];
const requestId = frame;
const { jobStartFrame } = frameData;
const { related_files: relatedFiles } = frameData.meta.frames[frame - jobStartFrame];

if (relatedFiles === 0) {
resolve({});
} else if (frame in frameData.contextCache) {
Expand Down Expand Up @@ -761,7 +812,6 @@ export async function getFrame(
);

frameDataCache[jobID] = {
meta,
metaFetchedTimestamp: Date.now(),
chunkSize,
mode,
Expand All @@ -784,6 +834,13 @@ export async function getFrame(
latestContextImagesRequest: null,
contextCache: {},
getChunk,
getMeta: () => {
const cached = frameMetaCache[jobID];
if (!(cached instanceof Promise)) {
throw new Error('Frame meta data is not initialized');
}
return cached;
},
};
}

Expand All @@ -803,25 +860,27 @@ export async function getFrame(
// Thus, it is better to only call `refreshJobCacheIfOutdated` from getFrame()
await refreshJobCacheIfOutdated(jobID);

const frameMeta = getFrameMeta(jobID, frame);
const frameMeta = await getFrameMeta(jobID, frame);
frameDataCache[jobID].provider.setRenderSize(frameMeta.width, frameMeta.height);
frameDataCache[jobID].decodeForward = isPlaying;
frameDataCache[jobID].forwardStep = step;

const meta = await frameDataCache[jobID].getMeta();

return new FrameData({
width: frameMeta.width,
height: frameMeta.height,
name: frameMeta.name,
related_files: frameMeta.related_files,
frameNumber: frame,
deleted: frame in frameDataCache[jobID].meta.deletedFrames,
deleted: frame in meta.deletedFrames,
jobID,
});
}

export async function getDeletedFrames(instanceType: 'job' | 'task', id: number): Promise<Record<number, boolean>> {
if (instanceType === 'job') {
const { meta } = frameDataCache[id];
const meta = await frameDataCache[id].getMeta();
return meta.deletedFrames;
}

Expand Down Expand Up @@ -900,12 +959,13 @@ export function getCachedChunks(jobID: number): number[] {
return frameDataCache[jobID].provider.cachedChunks(true);
}

export function getJobFrameNumbers(jobID: number): number[] {
export async function getJobFrameNumbers(jobID: number): Promise<number[]> {
if (!(jobID in frameDataCache)) {
return [];
}

const { meta, jobStartFrame } = frameDataCache[jobID];
const { jobStartFrame } = frameDataCache[jobID];
const meta = await frameDataCache[jobID].getMeta();
return meta.getSegmentFrameNumbers(jobStartFrame);
}

Expand Down
2 changes: 1 addition & 1 deletion cvat-core/src/session-implementation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ export function implementJob(Job: typeof JobClass): typeof JobClass {
value: function includedFramesImplementation(
this: JobClass,
): ReturnType<typeof JobClass.prototype.frames.frameNumbers> {
return Promise.resolve(getJobFrameNumbers(this.id));
return getJobFrameNumbers(this.id);
},
});

Expand Down

0 comments on commit 26f08ce

Please sign in to comment.