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

Fixed propagate in ground truth tasks with sparsed frames #8550

Merged
merged 15 commits into from
Nov 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
### Fixed

- Propagation creates copies on non-existing frames in a ground truth job
(<https://github.com/cvat-ai/cvat/pull/8550>)
8 changes: 5 additions & 3 deletions cvat-core/src/annotations-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// SPDX-License-Identifier: MIT

import { omit, throttle } from 'lodash';
import { omit, range, throttle } from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Import only necessary functions to optimize bundle size

While importing range from lodash, consider importing only the specific functions you need to reduce the bundle size. This can be done by importing from lodash/range instead:

-import { omit, range, throttle } from 'lodash';
+import omit from 'lodash/omit';
+import range from 'lodash/range';
+import throttle from 'lodash/throttle';

This ensures that only the required modules are included, improving the performance of the application.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import { omit, range, throttle } from 'lodash';
import omit from 'lodash/omit';
import range from 'lodash/range';
import throttle from 'lodash/throttle';

import { ArgumentError } from './exceptions';
import { SerializedCollection, SerializedShape } from './server-response-types';
import { Job, Task } from './session';
Expand Down Expand Up @@ -107,13 +107,15 @@ class PropagateShapes extends BaseSingleFrameAction {
}

public async run(
instance,
instance: Job | Task,
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure consistent parameter naming with the base class

In the run method of PropagateShapes, consider renaming the parameter instance to sessionInstance to maintain consistency with the base class BaseSingleFrameAction. This enhances readability and makes it easier for developers to follow the codebase.

{ collection: { shapes }, frameData: { number } },
): Promise<SingleFrameActionOutput> {
if (number === this.#targetFrame) {
return { collection: { shapes } };
}
const propagatedShapes = propagateShapes<SerializedShape>(shapes, number, this.#targetFrame);

const frameNumbers = instance instanceof Job ? await instance.frames.frameNumbers() : range(0, instance.size);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using native JavaScript methods for generating ranges

Instead of using lodash's range function, you might use native JavaScript array methods to generate the range of frame numbers. This can reduce external dependencies and improve performance.

Example:

-const frameNumbers = instance instanceof Job ? await instance.frames.frameNumbers() : range(0, instance.size);
+const frameNumbers = instance instanceof Job 
+    ? await instance.frames.frameNumbers() 
+    : Array.from({ length: instance.size }, (_, i) => i);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const frameNumbers = instance instanceof Job ? await instance.frames.frameNumbers() : range(0, instance.size);
const frameNumbers = instance instanceof Job
? await instance.frames.frameNumbers()
: Array.from({ length: instance.size }, (_, i) => i);

const propagatedShapes = propagateShapes<SerializedShape>(shapes, number, this.#targetFrame, frameNumbers);
return { collection: { shapes: [...shapes, ...propagatedShapes] } };
}

Expand Down
13 changes: 7 additions & 6 deletions cvat-core/src/frames.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,11 @@ export class FramesMetaData {
return Math.floor(this.getFrameIndex(dataFrameNumber) / this.chunkSize);
}

getSegmentFrameNumbers(jobStartFrame: number): number[] {
const frames = this.getDataFrameNumbers();
return frames.map((frame) => this.getJobRelativeFrameNumber(frame) + jobStartFrame);
}

getDataFrameNumbers(): number[] {
if (this.includedFrames) {
return [...this.includedFrames];
Expand Down Expand Up @@ -348,9 +353,7 @@ Object.defineProperty(FrameData.prototype.data, 'implementation', {
const requestId = +_.uniqueId();
const requestedDataFrameNumber = meta.getDataFrameNumber(this.number - jobStartFrame);
const chunkIndex = meta.getFrameChunkIndex(requestedDataFrameNumber);
const segmentFrameNumbers = meta.getDataFrameNumbers().map((dataFrameNumber: number) => (
meta.getJobRelativeFrameNumber(dataFrameNumber) + jobStartFrame
));
const segmentFrameNumbers = meta.getSegmentFrameNumbers(jobStartFrame);
const frame = provider.frame(this.number);

function findTheNextNotDecodedChunk(currentFrameIndex: number): number | null {
Expand Down Expand Up @@ -889,9 +892,7 @@ export function getJobFrameNumbers(jobID: number): number[] {
}

const { meta, jobStartFrame } = frameDataCache[jobID];
return meta.getDataFrameNumbers().map((dataFrameNumber: number): number => (
meta.getJobRelativeFrameNumber(dataFrameNumber) + jobStartFrame
));
return meta.getSegmentFrameNumbers(jobStartFrame);
}

export function clear(jobID: number): void {
Expand Down
15 changes: 12 additions & 3 deletions cvat-core/src/object-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ export function rle2Mask(rle: number[], width: number, height: number): number[]
}

export function propagateShapes<T extends SerializedShape | ObjectState>(
shapes: T[], from: number, to: number,
shapes: T[], from: number, to: number, frameNumbers: number[],
): T[] {
const getCopy = (shape: T): SerializedShape | SerializedData => {
if (shape instanceof ObjectState) {
Expand Down Expand Up @@ -397,9 +397,18 @@ export function propagateShapes<T extends SerializedShape | ObjectState>(
};
};

const targetFrameNumbers = frameNumbers.filter(
(frameNumber: number) => frameNumber >= Math.min(from, to) &&
frameNumber <= Math.max(from, to) &&
frameNumber !== from,
);

const states: T[] = [];
const sign = Math.sign(to - from);
for (let frame = from + sign; sign > 0 ? frame <= to : frame >= to; frame += sign) {
for (const frame of targetFrameNumbers) {
if (frame === from) {
continue;
}

for (const shape of shapes) {
const copy = getCopy(shape);

Expand Down
8 changes: 8 additions & 0 deletions cvat-core/src/session-implementation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -879,6 +879,14 @@ export function implementTask(Task: typeof TaskClass): typeof TaskClass {
},
});

Object.defineProperty(Task.prototype.frames.frameNumbers, 'implementation', {
value: function includedFramesImplementation(
this: TaskClass,
): ReturnType<typeof TaskClass.prototype.frames.frameNumbers> {
throw new Error('Not implemented for Task');
},
});
bsekachev marked this conversation as resolved.
Show resolved Hide resolved

Object.defineProperty(Task.prototype.frames.preview, 'implementation', {
value: function previewImplementation(
this: TaskClass,
Expand Down
8 changes: 4 additions & 4 deletions cvat-core/src/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -373,8 +373,8 @@ export class Session {
};

public actions: {
undo: (count: number) => Promise<number[]>;
redo: (count: number) => Promise<number[]>;
undo: (count?: number) => Promise<number[]>;
redo: (count?: number) => Promise<number[]>;
freeze: (frozen: boolean) => Promise<void>;
clear: () => Promise<void>;
get: () => Promise<{ undo: [HistoryActions, number][], redo: [HistoryActions, number][] }>;
Expand Down Expand Up @@ -403,8 +403,8 @@ export class Session {
public logger: {
log: (
scope: Parameters<typeof logger.log>[0],
payload: Parameters<typeof logger.log>[1],
wait: Parameters<typeof logger.log>[2],
payload?: Parameters<typeof logger.log>[1],
wait?: Parameters<typeof logger.log>[2],
) => ReturnType<typeof logger.log>;
};

Expand Down
22 changes: 15 additions & 7 deletions cvat-ui/src/actions/annotation-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,7 @@ export function propagateObjectAsync(from: number, to: number): ThunkAction {
const {
job: {
instance: sessionInstance,
frameNumbers,
},
annotations: {
activatedStateID,
Expand All @@ -465,12 +466,17 @@ export function propagateObjectAsync(from: number, to: number): ThunkAction {
throw new Error('There is not an activated object state to be propagated');
}

await sessionInstance.logger.log(EventScope.propagateObject, { count: Math.abs(to - from) });
const states = cvat.utils.propagateShapes<ObjectState>([objectState], from, to);
if (!sessionInstance) {
throw new Error('SessionInstance is not defined, propagation is not possible');
}

await sessionInstance.annotations.put(states);
const history = await sessionInstance.actions.get();
const states = cvat.utils.propagateShapes<ObjectState>([objectState], from, to, frameNumbers);
if (states.length) {
await sessionInstance.logger.log(EventScope.propagateObject, { count: states.length });
await sessionInstance.annotations.put(states);
}

const history = await sessionInstance.actions.get();
dispatch({
type: AnnotationActionTypes.PROPAGATE_OBJECT_SUCCESS,
payload: { history },
Expand Down Expand Up @@ -596,10 +602,10 @@ export function confirmCanvasReadyAsync(): ThunkAction {
return async (dispatch: ThunkDispatch, getState: () => CombinedState): Promise<void> => {
try {
const state: CombinedState = getState();
const { instance: job } = state.annotation.job;
const job = state.annotation.job.instance as Job;
const includedFrames = state.annotation.job.frameNumbers;
bsekachev marked this conversation as resolved.
Show resolved Hide resolved
const { changeFrameEvent } = state.annotation.player.frame;
const chunks = await job.frames.cachedChunks() as number[];
const includedFrames = await job.frames.frameNumbers() as number[];
const { frameCount, dataChunkSize } = job;

const ranges = chunks.map((chunk) => (
Expand Down Expand Up @@ -916,7 +922,6 @@ export function getJobAsync({
}
}

const jobMeta = await cvat.frames.getMeta('job', job.id);
// frame query parameter does not work for GT job
const frameNumber = Number.isInteger(initialFrame) && gtJob?.id !== job.id ?
initialFrame as number :
Expand All @@ -925,6 +930,8 @@ export function getJobAsync({
)) || job.startFrame;

const frameData = await job.frames.get(frameNumber);
const jobMeta = await cvat.frames.getMeta('job', job.id);
const frameNumbers = await job.frames.frameNumbers();
Comment on lines +933 to +934
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for external calls to getMeta and frameNumbers.

The calls to cvat.frames.getMeta('job', job.id) and job.frames.frameNumbers() may fail due to network issues or server errors, potentially causing unhandled exceptions. Consider wrapping these calls in try-catch blocks to handle possible errors gracefully and provide informative error messages.

Apply this diff to add error handling:

     const [job] = await cvat.jobs.get({ jobID });
+    let jobMeta;
+    let frameNumbers;
+    try {
+        jobMeta = await cvat.frames.getMeta('job', job.id);
+        frameNumbers = await job.frames.frameNumbers();
+    } catch (error) {
+        throw new Error(`Failed to retrieve job metadata or frame numbers: ${error.message}`);
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const jobMeta = await cvat.frames.getMeta('job', job.id);
const frameNumbers = await job.frames.frameNumbers();
let jobMeta;
let frameNumbers;
try {
jobMeta = await cvat.frames.getMeta('job', job.id);
frameNumbers = await job.frames.frameNumbers();
} catch (error) {
throw new Error(`Failed to retrieve job metadata or frame numbers: ${error.message}`);
}

try {
// call first getting of frame data before rendering interface
// to load and decode first chunk
Expand Down Expand Up @@ -962,6 +969,7 @@ export function getJobAsync({
payload: {
openTime,
job,
frameNumbers,
jobMeta,
queryParameters,
groundTruthInstance: gtJob || null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// SPDX-License-Identifier: MIT

import React, { useEffect, useState } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import { shallowEqual, useDispatch, useSelector } from 'react-redux';
import Modal from 'antd/lib/modal';
import InputNumber from 'antd/lib/input-number';
import Text from 'antd/lib/typography/Text';
Expand All @@ -23,12 +23,19 @@ export enum PropagateDirection {

function PropagateConfirmComponent(): JSX.Element {
const dispatch = useDispatch();
const visible = useSelector((state: CombinedState) => state.annotation.propagate.visible);
const frameNumber = useSelector((state: CombinedState) => state.annotation.player.frame.number);
const startFrame = useSelector((state: CombinedState) => state.annotation.job.instance.startFrame);
const stopFrame = useSelector((state: CombinedState) => state.annotation.job.instance.stopFrame);
const [targetFrame, setTargetFrame] = useState<number>(frameNumber);
const {
visible,
frameNumber,
frameNumbers,
} = useSelector((state: CombinedState) => ({
visible: state.annotation.propagate.visible,
frameNumber: state.annotation.player.frame.number,
frameNumbers: state.annotation.job.frameNumbers,
}), shallowEqual);

const [targetFrame, setTargetFrame] = useState<number>(frameNumber);
const startFrame = frameNumbers[0];
const stopFrame = frameNumbers[frameNumbers.length - 1];
const propagateFrames = Math.abs(targetFrame - frameNumber);
const propagateDirection = targetFrame >= frameNumber ? PropagateDirection.FORWARD : PropagateDirection.BACKWARD;

Expand Down Expand Up @@ -93,9 +100,9 @@ function PropagateConfirmComponent(): JSX.Element {
size='small'
min={0}
value={propagateFrames}
onChange={(value: number) => {
if (typeof value !== 'undefined') {
updateTargetFrame(propagateDirection, +value);
onChange={(value: number | null) => {
if (typeof value === 'number') {
updateTargetFrame(propagateDirection, value);
}
}}
/>
Expand All @@ -115,7 +122,7 @@ function PropagateConfirmComponent(): JSX.Element {
[frameNumber]: 'FROM',
[targetFrame]: 'TO',
} : undefined}
onChange={([value1, value2]: [number, number]) => {
onChange={([value1, value2]: number[]) => {
const value = value1 === frameNumber || value1 === targetFrame ? value2 : value1;
if (value < frameNumber) {
setTargetFrame(clamp(value, startFrame, frameNumber));
Expand All @@ -133,8 +140,8 @@ function PropagateConfirmComponent(): JSX.Element {
min={startFrame}
max={stopFrame}
value={targetFrame}
onChange={(value: number) => {
if (typeof value !== 'undefined') {
onChange={(value: number | null) => {
if (typeof value === 'number') {
if (value > frameNumber) {
setTargetFrame(clamp(+value, frameNumber, stopFrame));
} else if (value < frameNumber) {
Expand Down
3 changes: 3 additions & 0 deletions cvat-ui/src/reducers/annotation-reducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ const defaultState: AnnotationState = {
groundTruthJobFramesMeta: null,
groundTruthInstance: null,
},
frameNumbers: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure TypeScript interfaces include frameNumbers in job state

The addition of frameNumbers to the job state should be reflected in the TypeScript interfaces (AnnotationState and any related interfaces) to maintain type safety and prevent potential type errors.

instance: null,
meta: null,
attributes: {},
Expand Down Expand Up @@ -165,6 +166,7 @@ export default (state = defaultState, action: AnyAction): AnnotationState => {
job,
jobMeta,
openTime,
frameNumbers,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle possible undefined frameNumbers in action payload

When extracting frameNumbers from action.payload, ensure that frameNumbers is always defined or provide a default value to prevent potential undefined errors during runtime.

frameNumber: number,
frameFilename: filename,
relatedFiles,
Expand Down Expand Up @@ -209,6 +211,7 @@ export default (state = defaultState, action: AnyAction): AnnotationState => {
job: {
...state.job,
openTime,
frameNumbers,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate frameNumbers before updating the state

Before assigning frameNumbers to the job state, consider validating that it is properly defined and contains the expected array of frame numbers to avoid any unexpected behavior.

fetching: false,
instance: job,
meta: jobMeta,
Expand Down
1 change: 1 addition & 0 deletions cvat-ui/src/reducers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,7 @@ export interface AnnotationState {
requestedId: number | null;
meta: FramesMetaData | null;
instance: Job | null | undefined;
frameNumbers: number[];
queryParameters: {
initialOpenGuide: boolean;
defaultLabel: string | null;
Expand Down
Loading