diff --git a/mmif/serialize/annotation.py b/mmif/serialize/annotation.py index e910fd68..1e5e905b 100644 --- a/mmif/serialize/annotation.py +++ b/mmif/serialize/annotation.py @@ -414,22 +414,56 @@ def add_property(self, name: str, else: super().add_property(name, value) - def get(self, prop_name, default=None): + def __getitem__(self, prop_name: str): """ - Safe property access with optional default value for Document objects. + Property access for ``Document`` objects, following the document + property hierarchy (highest to lowest priority): - Searches for a document property by name and returns its value, or a - default value if not found. Documents have a more complex property - hierarchy than regular annotations: + #. Special fields (``id``, ``location``) + #. Pending properties (added via :meth:`add_property` after creation, + to be serialized as capital ``Annotation`` objects) + #. Original properties (in ``document.properties``) + #. Ephemeral properties (from existing ``Annotation`` objects or view + ``contains`` metadata) - Priority order (highest to lowest): - 1. Special fields ('id', 'location') - 2. Pending properties (added after creation, to be serialized as ``Annotation`` objects) - 3. Ephemeral properties (from existing ``Annotation`` annotations or view metadata) - 4. Original properties (in ``document.properties``) + ``Document`` overrides ``__getitem__`` (not only :meth:`get`) so that + the ``in`` operator, subscripting, and :meth:`get` all agree. + ``__contains__`` is implemented on top of ``__getitem__``, so without + this override a property added via :meth:`add_property` (which lands in + ``_props_pending``) is readable through :meth:`get` yet invisible to + ``in`` until a serialize/deserialize round trip. See issue #390. - This allows convenient access to all document properties regardless of - where they're stored internally. + :param prop_name: the name of the property to retrieve + :raises KeyError: if the property is not found in any of the stores + """ + if prop_name == 'id': + # because all three dicts have `id` key as required field, we need + # this special case to return the correct value from the correct dict + return self.id + elif prop_name == 'location': + # location is internally stored as `location_`, so the regular + # lookup below never finds it. Treat an unset location as absent + # (raise) rather than returning None, so the inherited + # `__contains__` does not report `'location'` as always present. + if self.location is None: + raise KeyError(prop_name) + return self.location + elif prop_name in self._props_pending: + return self._props_pending[prop_name] + else: + # Delegates to Annotation.__getitem__(), which checks + # self.properties (_props_original) before _props_ephemeral. + # This gives annotation-level properties precedence over + # view-level contains defaults (MMIF spec >= 1.1.1). + return super().__getitem__(prop_name) + + def get(self, prop_name, default=None): + """ + Safe property access with optional default value for Document objects. + + Searches for a document property by name and returns its value, or + ``default`` if not found, following the priority order documented on + :meth:`__getitem__`. :param prop_name: The name of the property to retrieve :param default: The value to return if the property is not found (default: None) @@ -455,22 +489,10 @@ def get(self, prop_name, default=None): add_property : Add a new property to the document mmif.serialize.mmif.Mmif.generate_capital_annotations : How pending properties are serialized """ - if prop_name == 'id': - # because all three dicts have `id` key as required field, we need - # this special case to return the correct value from the correct dict - return self.id - elif prop_name == 'location': - # because location is internally stored in self.location_, - # it doesn't work with regular __getitem__ method - return self.location - elif prop_name in self._props_pending: - return self._props_pending[prop_name] - else: - # Delegates to Annotation.__getitem__(), which checks - # self.properties (_props_original) before _props_ephemeral. - # This gives annotation-level properties precedence over - # view-level contains defaults (MMIF spec >= 1.1.1). - return super().get(prop_name, default) + try: + return self.__getitem__(prop_name) + except KeyError: + return default get_property = get diff --git a/mmif/utils/video_document_helper.py b/mmif/utils/video_document_helper.py index 87c1823b..0acdad44 100644 --- a/mmif/utils/video_document_helper.py +++ b/mmif/utils/video_document_helper.py @@ -97,19 +97,28 @@ def open_container(video_document: Document): stream = container.streams.video[0] time_base = float(stream.time_base) fps = round(float(stream.average_rate), 2) + # Resolve duration (in seconds) from the most reliable source available. + # `stream.duration` is exact for CFR H.264/MP4, but Matroska/webm leave it + # unset and carry the length only at the container level + # (`container.duration`, in `av.time_base` microseconds). + if stream.duration is not None: + duration_s = float(stream.duration) * time_base + elif container.duration is not None: + duration_s = container.duration / av.time_base + else: + duration_s = None # `stream.frames` comes from the container header. Verified exact on # CFR H.264/MP4 inputs even with non-zero start offset; for VFR or # headerless streams it may be 0, in which case `duration * rate` is # the best available (approximate) estimate. if stream.frames > 0: frame_count = stream.frames - elif stream.duration is not None and stream.average_rate is not None: - frame_count = int(round(float(stream.duration) * time_base - * float(stream.average_rate))) + elif duration_s is not None and stream.average_rate is not None: + frame_count = int(round(duration_s * float(stream.average_rate))) else: frame_count = 0 - if stream.duration is not None: - duration_ms = int(round(float(stream.duration) * time_base * 1000)) + if duration_s is not None: + duration_ms = int(round(duration_s * 1000)) elif frame_count > 0 and fps > 0: duration_ms = int(round(frame_count / fps * 1000)) else: diff --git a/tests/black-2997fps.webm b/tests/black-2997fps.webm new file mode 100644 index 00000000..73e19029 Binary files /dev/null and b/tests/black-2997fps.webm differ diff --git a/tests/test_serialize.py b/tests/test_serialize.py index 78d4ed9a..f2d36e68 100644 --- a/tests/test_serialize.py +++ b/tests/test_serialize.py @@ -1304,7 +1304,36 @@ def test_document_added_properties(self): doc1_mmif_roundtrip = mmif_roundtrip['doc1'] self.assertEqual(0, len(doc1_mmif_roundtrip._props_pending)) self.assertEqual('me', doc1_mmif_roundtrip.get_property('author')) - + + def test_document_pending_property_membership(self): + # regression for issue #390: a property added via `add_property` + # lands in `_props_pending` and must be visible through `get_property`, + # subscripting, AND the `in` operator consistently -- without needing a + # serialize/deserialize round trip first. + doc = Document() + doc.at_type = DocumentTypes.VideoDocument + doc.id = 'doc1' + doc.add_property('fps', 29.97) + self.assertIn('fps', doc._props_pending) + # all three access paths must agree + self.assertEqual(29.97, doc.get_property('fps')) + self.assertEqual(29.97, doc['fps']) + self.assertIn('fps', doc) + # special fields and absent keys are unaffected + self.assertIn('id', doc) + self.assertEqual('doc1', doc['id']) + self.assertNotIn('nope', doc) + with self.assertRaises(KeyError): + _ = doc['nope'] + # an unset `location` must read as absent, not as always-present None + self.assertNotIn('location', doc) + with self.assertRaises(KeyError): + _ = doc['location'] + self.assertIsNone(doc.get('location')) + doc.location = 'file:///tmp/doc1.mp4' + self.assertIn('location', doc) + self.assertEqual('file:///tmp/doc1.mp4', doc['location']) + def test_document_adding_duplicate_properties(self): mmif = Mmif(self.data['everything']['string']) doc1 = Document() diff --git a/tests/test_utils.py b/tests/test_utils.py index 188a0774..2bae378f 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -183,6 +183,26 @@ def test_open_container(self): finally: c.close() + def test_open_container_matroska(self): + # regression for issue #390: Matroska/webm leave the per-stream + # `frames`/`duration` fields unset, so open_container must fall back to + # the container-level duration instead of caching frameCount/duration 0. + vd = Document({ + "@type": "http://mmif.clams.ai/vocabulary/VideoDocument/v1", + "properties": { + "mime": "video", + "id": "o1", + "location": f"file://{pathlib.Path(__file__).parent}/black-2997fps.webm", + } + }) + c = vdh.open_container(vd) + try: + self.assertAlmostEqual(29.97, vd.get_property('fps'), places=1) + self.assertGreater(vd.get_property('frameCount'), 0) + self.assertGreater(vd.get_property('duration'), 0) + finally: + c.close() + def test_sample_timepoints(self): # half-open interval; step in ms self.assertEqual([0, 100, 200, 300, 400],