diff --git a/musicalgestures/_flow.py b/musicalgestures/_flow.py index 4c2c26b5..6bf02adc 100644 --- a/musicalgestures/_flow.py +++ b/musicalgestures/_flow.py @@ -30,6 +30,26 @@ def __init__(self, parent: "musicalgestures.MgVideo", filename, color, has_audio self.filename = filename self.color = color self.has_audio = has_audio + + def _parent(self) -> "musicalgestures.MgVideo": + """The MgVideo this Flow belongs to. + + `self.parent` is a weak reference, so it resolves to None once the video + is garbage collected --- which happens with + `flow = mg.MgVideo(...).flow`, where nothing keeps the video alive past + the expression. Dereferencing it then produced an AttributeError naming + whichever attribute was reached for, which says nothing about the cause. + + Raises: + RuntimeError: If the parent video no longer exists. + """ + parent = self.parent() + if parent is None: + raise RuntimeError( + "the MgVideo this Flow belongs to no longer exists. Keep a reference to the " + "video rather than to its flow: `mv = mg.MgVideo(...); mv.flow.dense()`, not " + "`flow = mg.MgVideo(...).flow`.") + return parent def dense( self, @@ -88,13 +108,13 @@ def dense( # Convert to avi if the input is not avi - necesarry for cv2 compatibility on all platforms if convert and fex != '.avi': # first check if there already is a converted version, if not create one and register it to the parent self - if "as_avi" not in self.parent().__dict__.keys(): + if "as_avi" not in self._parent().__dict__.keys(): file_as_avi = convert_to_avi(of + fex, overwrite=overwrite) # register it as the avi version for the file - self.parent().as_avi = musicalgestures.MgVideo(file_as_avi) + self._parent().as_avi = musicalgestures.MgVideo(file_as_avi) # point of and fex to the avi version - of, fex = self.parent().as_avi.of, self.parent().as_avi.fex - filename = self.parent().as_avi.filename + of, fex = self._parent().as_avi.of, self._parent().as_avi.fex + filename = self._parent().as_avi.filename vidcap = cv2.VideoCapture(filename) @@ -274,10 +294,10 @@ def dense( os.remove(source_audio) # save result at flow_dense_video at parent MgVideo - self.parent().flow_dense_video = musicalgestures.MgVideo( + self._parent().flow_dense_video = musicalgestures.MgVideo( destination_video, color=self.color, returned_by_process=True) - return self.parent().flow_dense_video + return self._parent().flow_dense_video def get_acceleration(self, velocity: list, fps: int): @@ -347,13 +367,13 @@ def sparse( # Convert to avi if the input is not avi - necesarry for cv2 compatibility on all platforms if convert and fex != '.avi': # first check if there already is a converted version, if not create one and register it to the parent self - if "as_avi" not in self.parent().__dict__.keys(): + if "as_avi" not in self._parent().__dict__.keys(): file_as_avi = convert_to_avi(of + fex, overwrite=overwrite) # register it as the avi version for the file - self.parent().as_avi = musicalgestures.MgVideo(file_as_avi) + self._parent().as_avi = musicalgestures.MgVideo(file_as_avi) # point of and fex to the avi version - of, fex = self.parent().as_avi.of, self.parent().as_avi.fex - filename = self.parent().as_avi.filename + of, fex = self._parent().as_avi.of, self._parent().as_avi.fex + filename = self._parent().as_avi.filename vidcap = cv2.VideoCapture(filename) ret, frame = vidcap.read() @@ -487,7 +507,7 @@ def sparse( os.remove(source_audio) # save result at flow_sparse_video at parent MgVideo - self.parent().flow_sparse_video = musicalgestures.MgVideo( + self._parent().flow_sparse_video = musicalgestures.MgVideo( destination_video, color=self.color, returned_by_process=True) - return self.parent().flow_sparse_video \ No newline at end of file + return self._parent().flow_sparse_video \ No newline at end of file diff --git a/musicalgestures/_motionvideo.py b/musicalgestures/_motionvideo.py index bc501561..1dd422b7 100644 --- a/musicalgestures/_motionvideo.py +++ b/musicalgestures/_motionvideo.py @@ -474,7 +474,7 @@ def mg_motiondata( motion_analysis: str = 'all', data_format: str | list = "csv", target_name: str | None = None, - overwrite: bool = True) -> "list": + overwrite: bool = True) -> "str | list": """ Shortcut for `mg_motion` to only render motion data. @@ -494,7 +494,7 @@ def mg_motiondata( str/list: The path(s) to the rendered data file(s). """ - out = None + out: "str | list | None" = None if type(data_format) == str: if target_name is None: @@ -505,6 +505,7 @@ def mg_motiondata( if type(data_format) == list: out = [] + paths: list = out if target_name is None: # this csv is just a temporary placeholder, the correct extension is always enforced based on the data_format(s) target_name = self.of + '_motion.csv' @@ -514,7 +515,7 @@ def mg_motiondata( tmp_name = generate_outfilename(target_name_of + '.' + item) else: tmp_name = target_name_of + '.' + item - out.append(tmp_name) + paths.append(tmp_name) mg_motion( self, @@ -538,6 +539,7 @@ def mg_motiondata( # return outlist # else: # return self.of + '_motion.' + data_format + assert out is not None, f"unsupported data_format: {data_format!r}" return out diff --git a/musicalgestures/_utils.py b/musicalgestures/_utils.py index e79e50f7..0b6600a9 100644 --- a/musicalgestures/_utils.py +++ b/musicalgestures/_utils.py @@ -578,13 +578,13 @@ def get_frame_planecount(frame: np.ndarray) -> int: return 3 if len(np.array(frame).shape) == 3 else 1 -def frame2ms(frame: int, fps: int) -> int: +def frame2ms(frame: int, fps: float) -> int: """ Converts frames to milliseconds. Args: frame (int): The index of the frame to be converted to milliseconds. - fps (int): Frames per second. + fps (float): Frames per second. Not an integer: 29.97 and 23.976 are ordinary. Returns: int: The rounded millisecond value of the input frame index. diff --git a/musicalgestures/_video.py b/musicalgestures/_video.py index c159da28..abba5997 100644 --- a/musicalgestures/_video.py +++ b/musicalgestures/_video.py @@ -97,7 +97,13 @@ def __init__( self.filename = filename self.array = array - self.fps = fps + # The ARGUMENT is optional: it is the rate an array is encoded at, and a file + # carries its own. The ATTRIBUTE is not, because `get_video()` below reads the + # true rate from the file and overwrites this, so by the end of `__init__` it is + # always a number. 0.0 stands for "not given yet" over the few lines before that + # happens, and the from-array branch below tests it for truth rather than for + # None, which rejects an explicit `fps=0` --- not a frame rate anything can use. + self.fps: float = fps if fps is not None else 0.0 self.path = path # Name of file without extension (only-filename) self.of = os.path.splitext(self.filename)[0] @@ -126,7 +132,16 @@ def __init__( # Check input and if FFmpeg is properly installed self.test_input() - if self.array is not None and self.fps is not None: + if self.array is not None and not self.fps: + # Without a rate there is nothing to encode the array at, so the write below + # never happens and the read after it fails with "no such file" --- blaming + # the output for a missing argument. Both before and after this attribute + # stopped being Optional, so say what is actually wrong. + raise ValueError( + "fps= is required when MgVideo is given an array: an array carries no frame " + "rate of its own, so there is nothing to encode it at.") + + if self.array is not None and self.fps: self.from_numpy(self.array, self.fps) elif fps is not None: # `fps` is the rate an ARRAY is encoded at. For a file input `get_video()` below reads diff --git a/tests/test_flow_parent.py b/tests/test_flow_parent.py new file mode 100644 index 00000000..43aeceb3 --- /dev/null +++ b/tests/test_flow_parent.py @@ -0,0 +1,89 @@ +"""A Flow holds its video by weak reference, so it can outlive it. + +`MgVideo` keeps a strong reference to its `Flow` and the `Flow` keeps a weak one +back, which avoids a cycle. It also means `flow = mg.MgVideo(path).flow` leaves +nothing holding the video: it is collected at the end of the expression and the +flow's `self.parent()` resolves to None from then on. Dereferencing that raised +an AttributeError naming whichever attribute was reached for, which said nothing +about the cause. +""" +import gc + +import pytest + +import musicalgestures +from musicalgestures._flow import Flow + + +class _Detached: + """A Flow whose parent is already gone, without needing a real video.""" + + def __init__(self): + import weakref + + class _Doomed: + pass + + doomed = _Doomed() + self.flow = Flow.__new__(Flow) + self.flow.parent = weakref.ref(doomed) + del doomed + gc.collect() + + +class TestParentResolution: + def test_a_live_parent_is_returned(self): + import weakref + + class _Alive: + pass + + alive = _Alive() + flow = Flow.__new__(Flow) + flow.parent = weakref.ref(alive) + assert flow._parent() is alive + + def test_a_collected_parent_raises_something_explaining_itself(self): + flow = _Detached().flow + with pytest.raises(RuntimeError) as excinfo: + flow._parent() + message = str(excinfo.value) + assert "no longer exists" in message + assert "mv.flow.dense()" in message, "the message must show the working pattern" + + def test_the_failure_is_not_an_attributeerror(self): + """An AttributeError here names an attribute, not the reason.""" + flow = _Detached().flow + with pytest.raises(RuntimeError): + flow._parent() + + +class TestFlowIsStillWeak: + """The weak reference is deliberate. A well-meaning change to a strong one + would make every MgVideo that touched .flow immortal, since the video holds + the flow and the flow would hold the video.""" + + def test_the_stored_parent_is_a_weak_reference_not_the_video(self): + import weakref + + class _Video: + pass + + video = _Video() + flow = Flow.__new__(Flow) + Flow.__init__(flow, video, filename="f.avi", color=True, has_audio=False) + assert isinstance(flow.parent, weakref.ref) + assert flow.parent() is video + + def test_the_flow_does_not_keep_the_video_alive(self): + import weakref + + class _Video: + pass + + video = _Video() + flow = Flow.__new__(Flow) + Flow.__init__(flow, video, filename="f.avi", color=True, has_audio=False) + del video + gc.collect() + assert flow.parent() is None, "the flow kept its video alive" diff --git a/tests/test_result_attributes.py b/tests/test_result_attributes.py index fdff57dd..bef66bab 100644 --- a/tests/test_result_attributes.py +++ b/tests/test_result_attributes.py @@ -110,3 +110,32 @@ def test_x_maps_to_vertical_and_y_to_horizontal(self, kind): setattr(v, f"{kind}_y", "from-y") assert getattr(v, f"{kind}_vertical_image") == "from-x" assert getattr(v, f"{kind}_horizontal_image") == "from-y" + + +class TestFpsIsAlwaysANumber: + """`self.fps` is a float, not `float | None`. + + The ARGUMENT is optional, because a file carries its own rate and only an + array needs one supplied. The ATTRIBUTE is not: `get_video()` reads the true + rate from the file and overwrites whatever the constructor stored, so by the + end of `__init__` it is always a number. Saying so closed twelve mypy errors + across the modules that divide by it. + """ + + def test_a_constructed_video_has_a_real_rate(self, tmp_path): + import numpy as np + + arr = np.zeros((6, 16, 16, 3), dtype=np.uint8) + v = mg.MgVideo(filename=str(tmp_path / "rate.avi"), array=arr, fps=24) + assert isinstance(v.fps, float) + assert v.fps > 0 + + def test_an_array_without_a_rate_says_so(self): + """It used to fail with "no such file", blaming the output for a + missing argument, because the encode never ran.""" + import numpy as np + + arr = np.zeros((4, 8, 8, 3), dtype=np.uint8) + for kwargs in ({}, {"fps": 0}): + with pytest.raises(ValueError, match="fps= is required"): + mg.MgVideo(filename="unused.avi", array=arr, **kwargs)