From 9b38f3e4ff812ec56e7336b7640352c076afacff Mon Sep 17 00:00:00 2001 From: Kirill Sizov Date: Fri, 26 May 2023 15:00:37 +0300 Subject: [PATCH] Fix skeleton tracks (#6075) Currently, we don't have validation of incoming annotations, as a result, there is exist some cases when annotations successfully saved in DB, but it's impossible to export them. In order to successfully export a dataset with a skeleton track it's required that each track satisfy the following condition: ` {frame number of track} == {frame number of parent track} == {frame number of the first shape of the track}` This PR adds an additional step during saving annotation in DB. This additional step check that all these there "frame numbers" are equal and try to automatically fix it's not true. --- .github/workflows/helm.yml | 2 +- CHANGELOG.md | 1 + cvat/apps/dataset_manager/task.py | 162 +++++++++++++------ cvat/apps/engine/tests/test_rest_api.py | 2 +- tests/python/rest_api/test_tasks.py | 98 +++++++++++ tests/python/shared/assets/cvat_db/data.json | 54 ++++++- tests/python/shared/assets/jobs.json | 2 +- tests/python/shared/assets/labels.json | 31 +++- tests/python/shared/assets/tasks.json | 4 +- 9 files changed, 300 insertions(+), 56 deletions(-) diff --git a/.github/workflows/helm.yml b/.github/workflows/helm.yml index 5624ba248..2e4ab0cb2 100644 --- a/.github/workflows/helm.yml +++ b/.github/workflows/helm.yml @@ -82,4 +82,4 @@ jobs: # They are still tested without Helm run: | kubectl cp tests/mounted_file_share/images $(kubectl get pods -l component=server -o jsonpath='{.items[0].metadata.name}'):/home/django/share - pytest --timeout 30 --platform=kube -m "not with_external_services" tests/python + pytest --timeout 30 --platform=kube -m "not with_external_services" tests/python --log-cli-level DEBUG diff --git a/CHANGELOG.md b/CHANGELOG.md index a70f21c0f..14e2deb1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ without use_cache option () ### Fixed - Skeletons dumping on created tasks/projects () +- Fix saving annotations for skeleton tracks () ### Security - TDB diff --git a/cvat/apps/dataset_manager/task.py b/cvat/apps/dataset_manager/task.py index 08674082c..c596db992 100644 --- a/cvat/apps/dataset_manager/task.py +++ b/cvat/apps/dataset_manager/task.py @@ -3,21 +3,23 @@ # # SPDX-License-Identifier: MIT +import os from collections import OrderedDict +from copy import deepcopy from enum import Enum -import os from tempfile import TemporaryDirectory from django.db import transaction from django.db.models.query import Prefetch from django.utils import timezone +from rest_framework.exceptions import ValidationError from cvat.apps.engine import models, serializers from cvat.apps.engine.plugins import plugin_decorator from cvat.apps.profiler import silk_profile from .annotation import AnnotationIR, AnnotationManager -from .bindings import TaskData, JobData +from .bindings import JobData, TaskData from .formats.registry import make_exporter, make_importer from .util import bulk_create @@ -115,47 +117,108 @@ class JobAnnotation: def reset(self): self.ir_data.reset() + def _validate_attribute_for_existence(self, db_attr_val, label_id, attr_type): + if db_attr_val.spec_id not in self.db_attributes[label_id][attr_type]: + raise ValidationError("spec_id `{}` is invalid".format(db_attr_val.spec_id)) + + def _validate_label_for_existence(self, label_id): + if label_id not in self.db_labels: + raise ValidationError("label_id `{}` is invalid".format(label_id)) + + def _add_missing_shape(self, track, first_shape): + if first_shape["type"] == "skeleton": + # in case with skeleton track we always expect to see one shape in track + first_shape["frame"] = track["frame"] + else: + missing_shape = deepcopy(first_shape) + missing_shape["frame"] = track["frame"] + missing_shape["outside"] = True + track["shapes"].append(missing_shape) + + def _correct_frame_of_tracked_shapes(self, track): + shapes = sorted(track["shapes"], key=lambda a: a["frame"]) + first_shape = shapes[0] if shapes else None + + if first_shape and track["frame"] < first_shape["frame"]: + self._add_missing_shape(track, first_shape) + elif first_shape and first_shape["frame"] < track["frame"]: + track["frame"] = first_shape["frame"] + + def _sync_frames(self, tracks, parent_track): + if not tracks: + return + + min_frame = tracks[0]["frame"] + + for track in tracks: + if parent_track and parent_track.frame < track["frame"]: + track["frame"] = parent_track.frame + + # track and its first shape must have the same frame + self._correct_frame_of_tracked_shapes(track) + + if track["frame"] < min_frame: + min_frame = track["frame"] + + if not parent_track: + return + + if min_frame < parent_track.frame: + # parent track cannot have a frame greater than the frame of the child track + parent_tracked_shape = parent_track.trackedshape_set.first() + parent_track.frame = min_frame + parent_tracked_shape.frame = min_frame + + parent_tracked_shape.save() + parent_track.save() + + for track in tracks: + if parent_track.frame < track["frame"]: + track["frame"] = parent_track.frame + + self._correct_frame_of_tracked_shapes(track) + def _save_tracks_to_db(self, tracks): def create_tracks(tracks, parent_track=None): db_tracks = [] - db_track_attrvals = [] + db_track_attr_vals = [] db_shapes = [] - db_shape_attrvals = [] + db_shape_attr_vals = [] + + self._sync_frames(tracks, parent_track) for track in tracks: track_attributes = track.pop("attributes", []) shapes = track.pop("shapes") elements = track.pop("elements", []) db_track = models.LabeledTrack(job=self.db_job, parent=parent_track, **track) - if db_track.label_id not in self.db_labels: - raise AttributeError("label_id `{}` is invalid".format(db_track.label_id)) + + self._validate_label_for_existence(db_track.label_id) for attr in track_attributes: - db_attrval = models.LabeledTrackAttributeVal(**attr) - if db_attrval.spec_id not in self.db_attributes[db_track.label_id]["immutable"]: - raise AttributeError("spec_id `{}` is invalid".format(db_attrval.spec_id)) - db_attrval.track_id = len(db_tracks) - db_track_attrvals.append(db_attrval) + db_attr_val = models.LabeledTrackAttributeVal(**attr, track_id=len(db_tracks)) + + self._validate_attribute_for_existence(db_attr_val, db_track.label_id, "immutable") - for shape in shapes: + db_track_attr_vals.append(db_attr_val) + + for shape_idx, shape in enumerate(shapes): shape_attributes = shape.pop("attributes", []) - # FIXME: need to clamp points (be sure that all of them inside the image) - # Should we check here or implement a validator? - db_shape = models.TrackedShape(**shape) - db_shape.track_id = len(db_tracks) + db_shape = models.TrackedShape(**shape, track_id=len(db_tracks)) for attr in shape_attributes: - db_attrval = models.TrackedShapeAttributeVal(**attr) - if db_attrval.spec_id not in self.db_attributes[db_track.label_id]["mutable"]: - raise AttributeError("spec_id `{}` is invalid".format(db_attrval.spec_id)) - db_attrval.shape_id = len(db_shapes) - db_shape_attrvals.append(db_attrval) + db_attr_val = models.TrackedShapeAttributeVal(**attr, shape_id=len(db_shapes)) + + self._validate_attribute_for_existence(db_attr_val, db_track.label_id, "mutable") + + db_shape_attr_vals.append(db_attr_val) db_shapes.append(db_shape) shape["attributes"] = shape_attributes db_tracks.append(db_track) + track["attributes"] = track_attributes track["shapes"] = shapes if elements or parent_track is None: @@ -167,11 +230,12 @@ class JobAnnotation: flt_param={"job_id": self.db_job.id} ) - for db_attrval in db_track_attrvals: - db_attrval.track_id = db_tracks[db_attrval.track_id].id + for db_attr_val in db_track_attr_vals: + db_attr_val.track_id = db_tracks[db_attr_val.track_id].id + bulk_create( db_model=models.LabeledTrackAttributeVal, - objects=db_track_attrvals, + objects=db_track_attr_vals, flt_param={} ) @@ -184,12 +248,12 @@ class JobAnnotation: flt_param={"track__job_id": self.db_job.id} ) - for db_attrval in db_shape_attrvals: - db_attrval.shape_id = db_shapes[db_attrval.shape_id].id + for db_attr_val in db_shape_attr_vals: + db_attr_val.shape_id = db_shapes[db_attr_val.shape_id].id bulk_create( db_model=models.TrackedShapeAttributeVal, - objects=db_shape_attrvals, + objects=db_shape_attr_vals, flt_param={} ) @@ -208,7 +272,7 @@ class JobAnnotation: def _save_shapes_to_db(self, shapes): def create_shapes(shapes, parent_shape=None): db_shapes = [] - db_attrvals = [] + db_attr_vals = [] for shape in shapes: attributes = shape.pop("attributes", []) @@ -216,16 +280,15 @@ class JobAnnotation: # FIXME: need to clamp points (be sure that all of them inside the image) # Should we check here or implement a validator? db_shape = models.LabeledShape(job=self.db_job, parent=parent_shape, **shape) - if db_shape.label_id not in self.db_labels: - raise AttributeError("label_id `{}` is invalid".format(db_shape.label_id)) + + self._validate_label_for_existence(db_shape.label_id) for attr in attributes: - db_attrval = models.LabeledShapeAttributeVal(**attr) - if db_attrval.spec_id not in self.db_attributes[db_shape.label_id]["all"]: - raise AttributeError("spec_id `{}` is invalid".format(db_attrval.spec_id)) + db_attr_val = models.LabeledShapeAttributeVal(**attr, shape_id=len(db_shapes)) - db_attrval.shape_id = len(db_shapes) - db_attrvals.append(db_attrval) + self._validate_attribute_for_existence(db_attr_val, db_shape.label_id, "all") + + db_attr_vals.append(db_attr_val) db_shapes.append(db_shape) shape["attributes"] = attributes @@ -238,12 +301,12 @@ class JobAnnotation: flt_param={"job_id": self.db_job.id} ) - for db_attrval in db_attrvals: - db_attrval.shape_id = db_shapes[db_attrval.shape_id].id + for db_attr_val in db_attr_vals: + db_attr_val.shape_id = db_shapes[db_attr_val.shape_id].id bulk_create( db_model=models.LabeledShapeAttributeVal, - objects=db_attrvals, + objects=db_attr_vals, flt_param={} ) @@ -257,20 +320,21 @@ class JobAnnotation: def _save_tags_to_db(self, tags): db_tags = [] - db_attrvals = [] + db_attr_vals = [] for tag in tags: attributes = tag.pop("attributes", []) db_tag = models.LabeledImage(job=self.db_job, **tag) - if db_tag.label_id not in self.db_labels: - raise AttributeError("label_id `{}` is invalid".format(db_tag.label_id)) + + self._validate_label_for_existence(db_tag.label_id) for attr in attributes: - db_attrval = models.LabeledImageAttributeVal(**attr) - if db_attrval.spec_id not in self.db_attributes[db_tag.label_id]["all"]: - raise AttributeError("spec_id `{}` is invalid".format(db_attrval.spec_id)) - db_attrval.tag_id = len(db_tags) - db_attrvals.append(db_attrval) + db_attr_val = models.LabeledImageAttributeVal(**attr) + + self._validate_attribute_for_existence(db_attr_val, db_tag.label_id, "all") + + db_attr_val.tag_id = len(db_tags) + db_attr_vals.append(db_attr_val) db_tags.append(db_tag) tag["attributes"] = attributes @@ -281,12 +345,12 @@ class JobAnnotation: flt_param={"job_id": self.db_job.id} ) - for db_attrval in db_attrvals: - db_attrval.image_id = db_tags[db_attrval.tag_id].id + for db_attr_val in db_attr_vals: + db_attr_val.image_id = db_tags[db_attr_val.tag_id].id bulk_create( db_model=models.LabeledImageAttributeVal, - objects=db_attrvals, + objects=db_attr_vals, flt_param={} ) diff --git a/cvat/apps/engine/tests/test_rest_api.py b/cvat/apps/engine/tests/test_rest_api.py index d248e40da..ef91c3444 100644 --- a/cvat/apps/engine/tests/test_rest_api.py +++ b/cvat/apps/engine/tests/test_rest_api.py @@ -4538,7 +4538,7 @@ class JobAnnotationAPITestCase(ApiTestBase): ] }, { - "frame": 1, + "frame": 2, "label_id": task["labels"][1]["id"], "group": None, "source": "manual", diff --git a/tests/python/rest_api/test_tasks.py b/tests/python/rest_api/test_tasks.py index b03d47403..0b9332c86 100644 --- a/tests/python/rest_api/test_tasks.py +++ b/tests/python/rest_api/test_tasks.py @@ -470,6 +470,104 @@ class TestGetTaskDataset: response = self._test_export_task(admin_user, tid, format=format_name) assert response.data + @pytest.mark.parametrize("tid", [8]) + def test_can_export_task_to_coco_format(self, admin_user, tid): + # these annotations contains incorrect frame numbers + # in order to check that server handle such cases + annotations = { + "version": 0, + "tags": [], + "shapes": [], + "tracks": [ + { + "label_id": 63, + "frame": 1, + "group": 0, + "source": "manual", + "shapes": [ + { + "type": "skeleton", + "frame": 1, + "occluded": False, + "outside": False, + "z_order": 0, + "rotation": 0, + "points": [], + "attributes": [], + } + ], + "attributes": [], + "elements": [ + { + "label_id": 64, + "frame": 0, + "group": 0, + "source": "manual", + "shapes": [ + { + "type": "points", + "frame": 1, + "occluded": False, + "outside": True, + "z_order": 0, + "rotation": 0, + "points": [74.14935096036425, 79.09960455479086], + "attributes": [], + }, + { + "type": "points", + "frame": 7, + "occluded": False, + "outside": False, + "z_order": 0, + "rotation": 0, + "points": [74.14935096036425, 79.09960455479086], + "attributes": [], + }, + ], + "attributes": [], + }, + { + "label_id": 65, + "frame": 0, + "group": 0, + "source": "manual", + "shapes": [ + { + "type": "points", + "frame": 0, + "occluded": False, + "outside": False, + "z_order": 0, + "rotation": 0, + "points": [285.07319976630424, 353.51583641966175], + "attributes": [], + } + ], + "attributes": [], + }, + ], + } + ], + } + response = patch_method( + admin_user, f"tasks/{tid}/annotations", annotations, action="update" + ) + assert response.status_code == HTTPStatus.OK + + # check that we can export task + response = self._test_export_task(admin_user, tid, format="COCO Keypoints 1.0") + assert response.status == HTTPStatus.OK + + # check that server saved track annotations correctly + response = get_method(admin_user, f"tasks/{tid}/annotations") + assert response.status_code == HTTPStatus.OK + + annotations = response.json() + assert annotations["tracks"][0]["frame"] == 0 + assert annotations["tracks"][0]["shapes"][0]["frame"] == 0 + assert annotations["tracks"][0]["elements"][0]["shapes"][0]["frame"] == 0 + @pytest.mark.usefixtures("restore_db_per_function") @pytest.mark.usefixtures("restore_cvat_data") diff --git a/tests/python/shared/assets/cvat_db/data.json b/tests/python/shared/assets/cvat_db/data.json index 5fa2c342b..4f5566860 100644 --- a/tests/python/shared/assets/cvat_db/data.json +++ b/tests/python/shared/assets/cvat_db/data.json @@ -653,6 +653,14 @@ "expire_date": "2023-03-24T11:54:54.631Z" } }, +{ + "model": "sessions.session", + "pk": "ttnnwlz4b1z28uiy24l4gqjgotwhpw1j", + "fields": { + "session_data": ".eJxVjEEOwiAQRe_C2hALFKYu3fcMZJhhpGpoUtqV8e7apAvd_vfef6mI21ri1vISJ1YX1anT75aQHrnugO9Yb7Omua7LlPSu6IM2Pc6cn9fD_Tso2Mq3zpzAUyay6EIHThLQQEZEAgdGkJ4hGeOCDBbOvrcYvMOQ2Fp2GYx6fwAZFjiv:1ptmJ8:Ac_26k4kTSoRBplBjwsTvpb0oukXBgpVgB230iP706c", + "expire_date": "2023-05-16T09:28:26.391Z" + } +}, { "model": "sessions.session", "pk": "v28l0efbrv9x06z97ilwcf7lwtuf4ctc", @@ -2983,7 +2991,7 @@ ], "bug_tracker": "", "created_date": "2022-03-05T08:30:48.612Z", - "updated_date": "2022-03-05T08:52:34.908Z", + "updated_date": "2023-05-02T09:28:57.638Z", "overlap": 0, "segment_size": 14, "status": "annotation", @@ -5244,6 +5252,42 @@ "parent": 58 } }, +{ + "model": "engine.label", + "pk": 63, + "fields": { + "task": 8, + "project": null, + "name": "skeleton", + "color": "#91becf", + "type": "skeleton", + "parent": null + } +}, +{ + "model": "engine.label", + "pk": 64, + "fields": { + "task": 8, + "project": null, + "name": "1", + "color": "#d12345", + "type": "points", + "parent": 63 + } +}, +{ + "model": "engine.label", + "pk": 65, + "fields": { + "task": 8, + "project": null, + "name": "2", + "color": "#350dea", + "type": "points", + "parent": 63 + } +}, { "model": "engine.skeleton", "pk": 1, @@ -5284,6 +5328,14 @@ "svg": "" } }, +{ + "model": "engine.skeleton", + "pk": 6, + "fields": { + "root": 63, + "svg": "" + } +}, { "model": "engine.attributespec", "pk": 1, diff --git a/tests/python/shared/assets/jobs.json b/tests/python/shared/assets/jobs.json index c72e0665b..ecee087bc 100644 --- a/tests/python/shared/assets/jobs.json +++ b/tests/python/shared/assets/jobs.json @@ -397,7 +397,7 @@ "url": "http://localhost:8080/api/issues?job_id=10" }, "labels": { - "count": 2, + "count": 3, "url": "http://localhost:8080/api/labels?job_id=10" }, "mode": "annotation", diff --git a/tests/python/shared/assets/labels.json b/tests/python/shared/assets/labels.json index 204d1111b..e22e8aa87 100644 --- a/tests/python/shared/assets/labels.json +++ b/tests/python/shared/assets/labels.json @@ -1,5 +1,5 @@ { - "count": 35, + "count": 36, "next": null, "previous": null, "results": [ @@ -688,6 +688,35 @@ "svg": "", "task_id": 21, "type": "skeleton" + }, + { + "attributes": [], + "color": "#91becf", + "has_parent": false, + "id": 63, + "name": "skeleton", + "parent_id": null, + "sublabels": [ + { + "attributes": [], + "color": "#d12345", + "has_parent": true, + "id": 64, + "name": "1", + "type": "points" + }, + { + "attributes": [], + "color": "#350dea", + "has_parent": true, + "id": 65, + "name": "2", + "type": "points" + } + ], + "svg": "", + "task_id": 8, + "type": "skeleton" } ] } \ No newline at end of file diff --git a/tests/python/shared/assets/tasks.json b/tests/python/shared/assets/tasks.json index 756e5d491..c86e9f2bd 100644 --- a/tests/python/shared/assets/tasks.json +++ b/tests/python/shared/assets/tasks.json @@ -551,7 +551,7 @@ "url": "http://localhost:8080/api/jobs?task_id=8" }, "labels": { - "count": 2, + "count": 3, "url": "http://localhost:8080/api/labels?task_id=8" }, "mode": "annotation", @@ -572,7 +572,7 @@ "status": "annotation", "subset": "", "target_storage": null, - "updated_date": "2022-03-05T08:52:34.908000Z", + "updated_date": "2023-05-02T09:28:57.638000Z", "url": "http://localhost:8080/api/tasks/8" }, { -- GitLab