portugueslab / stytra

A modular package to control stimulation and track behaviour
http://www.portugueslab.com/stytra/
GNU General Public License v3.0
41 stars 26 forks source link

Newest Stytra version does not save video in some experiments -> found a potential fix #78

Closed birtezuidinga closed 2 years ago

birtezuidinga commented 2 years ago

Hi everyone,

I have found out that the error I reported in https://github.com/portugueslab/stytra/issues/76#issue-1187787368, was probably unrelated to the changes I made to the naming of the files. When returning to the most recent original Stytra version, without my changes except for the stimuli I created, the problem persisted (for certain stimuli, and not all the time). This was the error message:

Process :
Traceback (most recent call last):
  File "C:\Users\zuidinga\Miniconda3\envs\stytra_env_github\lib\multiprocessing\process.py", line 297, in _bootstrap
    self.run()
  File "C:\Users\zuidinga\PycharmProjects\stytra_birte\stytra\stytra\hardware\video\write.py", line 91, in run
    self._configure(current_frame.shape)
  File "C:\Users\zuidinga\PycharmProjects\stytra_birte\stytra\stytra\hardware\video\write.py", line 295, in _configure
    self._container = av.open(self.__container_filename, mode="w")
  File "av\container\core.pyx", line 364, in av.container.core.open
  File "av\container\core.pyx", line 146, in av.container.core.Container.__cinit__
ValueError: Could not determine output format

I found out that the error is caused by the self.__container_filename being empty at the time when the av container needs to be opened. As far as I understand, a different process is running in the background to create the filename, and somehow for certain (harder to compute?) stimuli that lasts too long. Since the new Stytra version includes the variable self.CONST_FALLBACK_FILENAME, to later be replaced by the correct filename, I added the following lines in the C:\Users\zuidinga\PycharmProjects\stytra_birte\stytra\stytra\hardware\video\write.py script, and as far as I can see, all my stimuli work now (no crashing and no unsaved videos anymore):

At line 283:

        else:
            self.__container_filename = self.__generate_filename(self.CONST_FALLBACK_FILENAME)

If this is how the fallback filename is supposed to be used, could you incorporate this change in the program?

nvbln commented 2 years ago

I think the main issue is actually on line 255. There the fallback is specified for self.__container_filename already. Rather, I think that the issue is that no output format is specified when assigning the fallback to the filename on line 255. Would it be possible to test changing self.__container_filename = self.CONST_FALLBACK_FILENAME to self.__container_filename = self.__generate_filename(self.CONST_FALLBACK_FILENAME)?

birtezuidinga commented 2 years ago

Thank you! I tested this with my stimuli and it appears to work well!

birtezuidinga commented 2 years ago

Sadly I have to retract the previous comment, after trying some more experiments today, I encountered the same error message again with the code you proposed. Adding back my line as well as your line of code appears to be working for now.

nvbln commented 2 years ago

Does this mean that both lines of code need to be added for it to work, or does your code work as a stand-alone as well?

EDIT: I think I found the issue, I forgot to encapsulate the filename in the _reset function with the filename generator call as well. Could you check whether the code in PR #79 works? Then I can mark it as non-draft and it will hopefully be merged soon. :)

birtezuidinga commented 2 years ago

Thank you! I'll check #79 later today and report back!

birtezuidinga commented 2 years ago

The code in #79 works for me without problems now!

nvbln commented 2 years ago

Great to hear! Thanks for all the help getting this resolved!