fix VDH's (un-)handling of incomplete video container metadata

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
 

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:

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()

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],