Skip to content

Get correct frame amount for video files without nb_frames property & allow codec selection#193

Closed
ximion wants to merge 3 commits intodenisecailab:masterfrom
ximion:master
Closed

Get correct frame amount for video files without nb_frames property & allow codec selection#193
ximion wants to merge 3 commits intodenisecailab:masterfrom
ximion:master

Conversation

@ximion
Copy link
Copy Markdown
Contributor

@ximion ximion commented Mar 15, 2022

Hi!
First of all, I'm really impressed by this project! Two years ago, it was a bit mediocre compared to MIN1PIPE, but now based on very early testing, Minian seems to outperform CaImAn and MIN1PIPE for my datasets! (Still need a lot more testing for that though, I wonder whether switching out Minian's median filter with an anisotropic filter for denoising would make things even better...)
The best thing is that Minian's documentation is excellent, which makes it very easy to get started, and all public API seems to be well documented as well. Really awesome work!

The data I generate is recorded with PoMiDAQ or Syntalos, which prefer the MKV container for video, as it's a bit nicer to store FFV1 video in, but more importantly I use MKV's ability to save arbitrary metadata with the video file quite excessively.
When loading these files into Minian, it currently crashes as the nb_frames property apparently doesn't exist for MKV containers. Fortunately, the amount of frames is very easy to calculate, so this patch adds support for just that!

Thanks for considering the change! :-)
Cheers,
Matthias

@ximion ximion force-pushed the master branch 2 times, most recently from 45024c3 to 900f432 Compare March 16, 2022 21:56
@ximion ximion changed the title Get correct frame amount for video files without nb_frames property Get correct frame amount for video files without nb_frames property & allow codec selection Mar 16, 2022
@ximion
Copy link
Copy Markdown
Contributor Author

ximion commented Mar 22, 2022

This PR also includes a change to allow setting the video codec. When depending on ffmpeg~=5.0 (and using Mamba to resolve the environment in seconds, as conda takes almost a day and doesn't find a good solution) I can do this now using AV1:

generate_videos(varr.sel(subset),
                Y_fm_chk,
                A=A, C=C_chk,
                vpath=sys_tmpdir,
                vname='overview.mkv',
                vcodec='libsvtav1',
                options={'preset': '4', 'g': '240'},
                verbose=True)

Using AV1, the 1.4GB overview video is now just 2.2MB in size! This makes it possible to just always generate this video as part of a batch processing pipeline and just keep it around, while with the larger video handling the data would be more of a pain (and the backup costs would explode in the long run). The AV1 quality with these settings is a bit worse than H.246, but at least for me that's definitely acceptable here :-)

@ximion
Copy link
Copy Markdown
Contributor Author

ximion commented Mar 22, 2022

Test failures look unrelated to this PR.

@phildong
Copy link
Copy Markdown
Member

Hey @ximion Thank you so much for the PR! I skimmed through the changes and everything looks great!
I'm wondering whether you can provide a small piece of mkv data to be stored on the minian github so that we may be able to incorporate this in part of the test?
At the same time I'll try to resolve the testing failure for now

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 18, 2022

Codecov Report

Merging #193 (7a8f579) into master (f2da3f9) will decrease coverage by 0.10%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
- Coverage   75.43%   75.32%   -0.11%     
==========================================
  Files           9        9              
  Lines        2548     2557       +9     
==========================================
+ Hits         1922     1926       +4     
- Misses        626      631       +5     
Impacted Files Coverage Δ
minian/utilities.py 69.06% <28.57%> (-0.75%) ⬇️
minian/visualization.py 79.10% <100.00%> (+0.07%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@ximion ximion force-pushed the master branch 2 times, most recently from 56f3d0f to 7a8f579 Compare June 19, 2022 18:35
@ximion
Copy link
Copy Markdown
Contributor Author

ximion commented Jun 19, 2022

Hi @phildong and sorry for the delay! Congrats on publishing the Minian paper! :-)

I'm wondering whether you can provide a small piece of mkv data to be stored on the minian github so that we may be able to incorporate this in part of the test?

Sure, I added a small 30sec piece (which is still 148MB in size) to this PR, let me know if you need a longer slice.
I did enable Git LFS for this file, to store the large binary file in this Git repository efficiently.
It may make a lot of sense to enable this for the AVI files as well, but to do that you would need to rewrite the Git history (so only makes sense after merging this PR, and only if you want to).

The commands for a LFS conversion of this repository (after making sure the LFS module is installed as described at https://git-lfs.github.com/) would be:

# track .avi files in LFS
git lfs track "demo_movies/*.avi"

# test what a migration run would do to the repo
git lfs migrate info --everything --include="demo_movies/*.avi"

# actually migrate everything
git lfs migrate import --everything --include="demo_movies/*.avi" --verbose

# force-push, possibly rewriting the history
git push -f

# ensure working copies of the large files are up to date
git lfs checkout

Sidenote: I don't think LFS is an amazing solution in every case, as it stores the large objects on a central server, breaking Git's decentralized approach, it adds an extra step to using a repo and it is annoying to switch away from it on a repo that is using it. However, for large test data as is the case with this, I think its benefits definitely outweigh the cost at the moment :-)

@ximion
Copy link
Copy Markdown
Contributor Author

ximion commented Oct 7, 2022

Any news on this? We have been using this code for pretty much a year now without any issues :-)

@ximion
Copy link
Copy Markdown
Contributor Author

ximion commented May 17, 2023

Closing this in favour of #246, since I need the master branch of the original fork for, well, our Minian fork ^^
(I didn't expect that this would take so long to get merged)

@ximion ximion closed this May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants