From 391a165004e88adb6cd92b9234e2746aac36194b Mon Sep 17 00:00:00 2001 From: Keigh Rim Date: Thu, 18 Jun 2026 10:45:06 -0400 Subject: [PATCH 1/2] unified behaviors of `Document.get` and `Document.__getitem__` --- mmif/serialize/annotation.py | 78 +++++++++++++++++++++++------------- tests/test_serialize.py | 31 +++++++++++++- 2 files changed, 80 insertions(+), 29 deletions(-) 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/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() From 3428ebfb6e8fe2c439c1f5b5f3080c032f8186c0 Mon Sep 17 00:00:00 2001 From: Keigh Rim Date: Wed, 17 Jun 2026 20:43:03 -0400 Subject: [PATCH 2/2] vdh falls back to container metadata when ... stream metadata is incomplete to get basic technical information. This happened with matroska container file. --- mmif/utils/video_document_helper.py | 19 ++++++++++++++----- tests/black-2997fps.webm | Bin 0 -> 67670 bytes tests/test_utils.py | 20 ++++++++++++++++++++ 3 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 tests/black-2997fps.webm 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 0000000000000000000000000000000000000000..73e19029a900cb3f537321a4d608023405364076 GIT binary patch literal 67670 zcmeI533L=y*2k+m`wAk4Wl$1EL>R#UMi!Z|v!N_W*g;2xu!UU~gWw0)oe)4kWEor# zG!P6+P!M!L0@%TYD10mfC^Lfsq9Ta%p(vt&e(%1j3UsF5|Mof4)2B%~2RNbXU-!Lw zuWonWU;pb>{q*BSucXX$SW~CY3;C{>m4if|HbuseU$j$(Tvi0VqdLZ@8j$b>eZ%ZpS+P@BgK|sf2M2pq+$2AYSB{sNo?6S+4cWv zPtUe}+a;d5LEYc~y3x6gpExWN-=96_^K~PdA8-82GKbdat!4SSV~5Nv=rf>Cw|4ms z%kNL+&#+u=wEo1>jd@o4wmi#;r4DQFjq`fH?XdLTBK~f5SZNS!S0+xpJlJW^DPNLa zG&du;b5@T5DLs-i)6!rfAzY1c-u~m=0>h9FX?;v(>0S1*ZO)(dV0OJ&Fbg9{N$hh{M=iO zPe1caa*I8G8TQc9MJv`cJzezW+oO8)8}xtwb3pTQxT2K~EkP^RY>OIP8E{gw+Q+Sr zr*3N@{pufzyCnZ9Ds*V6hNgd+X0eZRrx;>Jv8WUUhT_i&ZWYB2ZM9T<|3?2ATBzPg z)QBaCxgR&{vpMZVzdmhx_3s^Dw9%oJOWm>=y_2;dZQPW!C+yUtnCVAqJrdh4Ui)TL z`@P!qBeApNrbkacsudMEw6ppVkN3*1Xgh3r+C3Yax74N{sd?!O5?P{j(deElu8F3; z-Ms6z_w0sfwZn3kE*jXQtU?quZPR$W(_dKV{Q0Yv16$dK*h_BvOTjB6FRZ?D@XnlDuI@Rrd+&mG0yLDwqYUtq*5o1lr*=++3J-aM- z%AwNuI`WtOAK13#{If9ap?UYeHRZ54`rF4I*w=phSoeKc3w(YFho!G`uv43RzF(C%_)OW~=f`%W$!RMgv{ZAp1+ zzzFyd$^t zyfVbLMvT;o{E^oEG!8!|$f>ayz9jpx_;2!v zUq(MNn?~uthM`ZMxO;WOwr9UN*!L&-%hY{!1 zA93N8&a3W@y>rQr508C*_P^{MZ$4i3P?IZK(OHM(F>RG*@09s+lWumaecJD96kctR zxW`6UOK7hK+VNZMy>5Rqv0aDHnvR{A_2t}@TkdN(<%@#GL(b0Wm-I#7q04(mZPPEy ze%)mfe-uxLY9To-*C*SD2dxu7_iJUz31xJdFVLmkx?h)vF2bTxhh>v4da&EIgwdnr zTxoH6%(|w#hemwUD0|kluNUP1A+79{smDrAhOdg;&~gE_a7aHmE$dn$xpqaoq*#8` zMYb$UMIsq3P^X2$8mB8dWLEB|>@6wU=!&)(|JF-0#zVT|p01Ukcv1 zwt~z>zwc(ZILD2+BHkN3uX~T;Gi|aq z*t%V5a#QZfgX8l*yBu?|@Oa0Pgk_fcSLPyX-eHzS`P)iLzq z(AxD^4Sr|qEBj*_U5wU(8~))RZO$!kG$*Ei>E)+S)%s@nl`sBS*z1kA$Bs`*9MNat z38z*zG`p;5ox}Q>7(b&a@%HhdliJW?pSpPJe62@+YjUXM`zZ&soCzBWi`F=-zg^Rs zZzJxNLG-3%L`BM%9YHDot_-I3WSe3?y)jja*dy7t;Q@%T7(br^t5N5ge(%P|Qy_VAq`bdt~s# zNzI0bHG6vUl8X)gwPwQByCYki`y{!^!`uHMUOn2B#UJ_V^S(zXZd>uuiS2imZtfqv zT(mgp-cf~%Uwd?H<7u<*Ti4+As9keQlkA$-H?;c`!?s1Vzy1)hIkok%It6tmU4O`H zCp7Ku!d;DeHfCi9T`2tJ9p^iYY03 zzkXTK%Z^~O`BTHPtN7cyW_{qeW2<#}muV*|{a2iPNIwx=UXu>DgBLP| z*)Lx4;PKm;{S~hXiuNmB2i)%ujmPmrD{@|O{9xwz8O;8QoL9u(%^d$V-X9i;_S=p* z!u}v;Urb=#lw{Wltkt2x&Ia$kFflfJZ@XEEUzxv(50y)H6^y=qlfHL?xMEHkL^5;yAZ9<4o)NDy z$1~{}!K7zo1dbmmvJ=gJWLIYY7-m0neMfF&jz7rkXRhz4+IW7WnCm-=xxS+&MeLd>3XvbA89m zXO3sC?-=I#j`<$Pj}_N9%HP-|W`7@MKXZM@zQ`O;#XNX_iDjN}o>5F6e-#_E}z8)P^ z)DPnGMa^)te-^jL*KEq{H?D7G{L#$u#`W#?ctXfMTAa?SxuM8*$%P?1X98KFn!@M*Zot6NjFCC+c#Snrrh`J~?~a zZ4K9MFRk0U%yFt9ZKsg`l;@w=bA3OZ~Uk0ek*Fax1@ECUP&PxewezY z`N%H&e^)=R|MPe5-JH}owOh~q5A2!n@v80Ly|Je8-zPkl(Yc%Lzu!X92ZWs^2brI6 zo;b)ycOQJ0PEUYm+m#d3&clU;sd| zTAy;DFgO5E5QK(3l>{0qs4T{cOjhw2R_xIcAu)>%GhB$5_3)chWT^x!DGI=3TR8-Td7$z)K$1q_bP#CZfC`?$WJQsq6I+sxPbO)=T z@>#MCWeh5yp+W*Qte_$qE2xad3Q8R3B9}VVHTi<5l*TbAaj=3A8dL}@#wjXzl4?w^ z5hbO9>QdZ~3aX@xQNfjziVCu%RIu4_jV`HT5MRRQA|VnyTZxgJX!b+cYzU!R6}Ljw zDps)BAb}E$A(0YRu-Onwbt{g+W`+JixpJEVg+S`6U}}Y)v-7R zn+;*aYH$pyW}!mz8myp-7AvTx#R{S}SV46yRK(0+5=-q-h&mC4Ol@nF;)=u!3s7TqNy^5F^K&z3Zg$a z2Gz$-H2X1THgru;eCC%7h+0uAHW>X-E6i+AD~JTaRwN6;3Nss&t;7-!!ZC;lVFfWE ztRO0c6=pW5dn(9~K69$ig*9%|Tj`LQt(ZAUheSVYS*p2}d`Kv+=R2-dK@B4tfPxqi zjv*)!oy+x<#m$&3;n4KO3JNzF^J zshN=&Mq)5-t&G<*BSQz65mr#?ixtF-u)-*P0W&g6U$w$?Edqs6`T_+pBiK`6P=gAc z8A-!}*O-yOTxt>{nGse{l?)Y<8DRxg$ynjWYT^1CVgX@?Ev)Le-5o|>=BdlPvftV4VeaVb)D^w+81u-M6V6(xU3CVR#L*4is zDk<1(kjx17G?dK-$&7Fes*<6?oh8ZjT*#Be#Gon}w<59u&%R_vI0l;yk{RI`ZX6J< zXY0=6Q%T{*1yM=i#s^VJLCgsD)ScI-QVch4h)N1J8^WndhW(Jt2rJlZh!9mWo_)!T za4S?LV+B>oSixq4I|q>Km`1wsNK{g=*&vw_>}e!1Bdl=am8jGYs*-UGHX9@}f-xjB z!U~ZMc=p}-f?UtVoinJCg3X2~s*-U}*=%s<5OVE@JC9H$1u-Mo4|hJHN-?NP#xdAz zkjx0ikjw}x*ldUqRWhD^$&9EKCB3Nd4XG8xjBpGy8`LpO_mEN%do$Ncg@Z^P!}JiT z6{d?wtsrIu&o-8r5muPlpl(GlBYoynpZO&-60I=INGKR)Bou%M5i`OHBO5>r!Hno! z7-l4Usw6_;dOrYFYRW2z8R1q&vx;Y+!emCRVgk&F?zftkpsAUWm8qwWno;_y6<+Zf8Kp01g_sdIm+I>?qAFS7HZ|Rn%m^!3XN9=YkZXmQ z5pIQbR@^{7qut!u%3Jl|cvLB$5$mjw%m|K$2RgU zXGB#p9Mfv%GomUPj)zx#Mh&@E78L`d^dFL1$kATm}HTOO68JW%s zrDwaE`HT!_h5FN{yuS=yg&z2fs7i)0~j5n32(zf;-(f zJ|kjA@LU2uBZUvq$~r5&;xl5M71x$~Qhgh6ZILIH;s){=5i=49Q~g;$Y#b-o!zx0;aa+=f>r zpAj)5I3Dg+7jhksfX~QvJUs9j&F4nl-I_y{S}~m!O3o5_H(jT>v+g})9Ce7&7Gbwu0plK%m%fhD*23v8Ho{N zm=RX+KBKSnRum*gtefI)6(iTVb+?XDC57*NMm}{VB-G3k;mik@FZ`L#Njn9af5gZT6jIe_D8L?|t$a~XZD?zNYB8U>l ziN0}?_om@ieB(1BW(51;H|rTyC7;n!Zq)r|JtN=vjEEV*t57l{toWLX5f;HJ*lY-+ zyK!JY2_;Gh$b8k<195tz<@6F@+m3d2bqyQI&i~bpHW7TfbS)$TvPCVn%R0 zyt1AV-HiiV@tgIGeB(1BW(3E>Z`L!aNlsxgpAoxeMKGHU!NiQ<%z0%!Bj5Oph#A3ik<17yK4oLDYgYKp zdPc;I;Msb`XGF{hw&FMI8C4~pk-27t(l_BZ>lyjRXJo8dp?=?Y_bfBl?{Fj76>cMl<0zRYb>}G`y4}3;5#Hc%w*PxpDjEEV*t57l{tRQBD6?FdrRQSz$ zM!xYG5i^40;lXp$0zM;`-1op|G)9cN6Xkw2^BEB{f>)ted`7g+DQ<<$h6vxRXXG27 z5iuh;9v(b5E#Nb9$$bx=n?|et!rb?p^^APuGa_aLuR_U;u!8p)z0bD7W66D*zEPU3W^F6GP01da70sGr|gDMo=M{5mtC(J)^4RGrC_NQ)UfpF{a?ov#R7X zB4#AcR>6Dd4mcwlfP$EjI)=%N3ML1bk?u371d ziFsj|kx($q2rG1Fwae+d(&i3Es7Ovy|SJWF(cfHkqrzpQpYfv(Q|iHb7s^? zTc!JqY-(mChLL4Psrrp}J4xoK=RgtH#ha5fvl*=z`>?-Ss>gtOTYPTwcs z7<8v5R}lk) ztb)yk$iJ~MSPyCxn+;L)eF8j}sJ=o0XFrO~hA7s9>cMl<*ldVmU3bxJHbk=?)M)xX z0ghO-_;!s^yviy*U=?gO#L)K%u%{mQjJk>#aQ0)w_X)BUH4!qiL9L+g6R4+Z#X(kK zW`i|(9)r0zO|$}- zk&z96_vGuX00l84(F#ydHi&a+ka|*4_}3tUDtoGAMn*OOn$wJp!LMgzWCLhrK5WHg zM#V=0%t-ecC90W`m~fAnk)d-(KnfimJU4BoK4pMA8Ko~!5HrI4AZ7#=9;|0X%m~LY zN?&zPJ$P=KU`7^gkr$bfKve4LLoy?*AZ7#=k{MwIF(a%XW`q?~C1V9KBdnk*87rtt zh6>4yu!5>&tRQBD6~v6NLR884`$ox(a15%Fv4W~(s0b!zgcVdJV+B>oSYg0c^_3h< zRWgo2%m^!p8DRxg$xtDg5mr!@j1^QRV}-~DJo}Ov;TUW-NM?j%P?Zc7k{MwIF(a&? zDj6$?8DRxg$yh;EGFGtJAej-2A(;_Yu-QOsVB^`B%m}waRWepkm5dc^Hb`a!V|eh~ zG&UO~Gs3M9Gr|gDMp!{rGFGtJAej-2;epRcWCNak$&7F-Y&J+{gkw;Z3>A_YVFfWG zte`3xD~K6k1y#vdK~*wVu-V|jbJK_!!B!+Q!U|C(2RGAZCOWA{*#j7-l3CfYpTStpEivBOHU65mpd0!U|$WYK6&+w#*1HBi&~Nn2{Jp zkC>661I!33X6jR><}4a9BOHU65mpd0f{N|iqD9uWH{>8%x6T*2#$wWd`46y!&a)9 z&xmza1bjxWxgS9U)rnsDJ@6S(l?<=KYUVRCofXRa%Dmz;GMyF5R;roL$Z%GuKYa## zMy|OZ;|-q?Rmqh5N&*sNW`kN$m3&6TjNo|0dc$XQ)qK^4%t-WuVMap1Fe9O0m=RVG zGr|fYsLub;XGF|Mw8AhWtRQBD6~v6N!pH_iKO{5ibvm!Sb#-P&=NuZ{n`ZBnIVV2e z#b;DECU^PT_lDeGTJNr7>juiZpt?Bk8+X5^+AgOa>#$vyh>VAUgUxRF@@g|&LQky!tQyn{`kQ1xf7AVHS!_fi2jlIy23 z51y`8z1&Ew|H3P7Bz6rjc?TQ3)eB(x9{TGrf!E*_?kz6y|12+=u zzmRvZ!BG!q{TJakjvI-{5wD6)Lz9^~1;pg~B9DM?VjcCEbnmACo2Vvf&X~GIZ|2g}l!VD~K#%1@BoR zvV>!J!?Sc3*NP>RRT#JHs5qMM*GBSBK3W^eH=ZTBvj>icSJpXe7 z6nBZZ5s@Wv6)L!qNtV!rFl|YA)Y#JORbxx57-LI^zZ+Xpwi{cr zVvH?4M;TkPlZ`Dy>Kj`o<{Dcb|IpY{u-4d8FxlAh{HMm2=k74Jth>qBQhdbNvOV3{ svSo&`W#1HIOL=Q!%Zb;FEk{2$wp<9KEekh`t98p