lagmoellertim / unsilence

Console Interface and Library to remove silent parts of a media file πŸ”ˆ
https://unsilence.readthedocs.io/
MIT License
550 stars 44 forks source link

Added option to check for bad intervals #140

Closed Garbaz closed 1 year ago

Garbaz commented 1 year ago

This addresses the isssue #108 (See the investigation comment there for more details).

Unfortunately, there doesn't seem to be a way to tell the concat script of ffmpeg directly to ignore files it can't handle. And the ffmpeg command that produces the bad interval files in the first place doesn't seem to be aware either that it is doing so.

So I added an additional command line option (--check-intervals) that, if provided, causes the RenderIntervalThread to run ffprobe on the rendered file for each interval to ensure that it will be recognized by ffmpeg later in the concatenation step.

I made this optional, since running ffprobe for every interval's file adds quite a bit to the total runtime of the program (For my test file, it went from 0:02:06 to 0:02:36). Though if it is considered acceptable in face of the fact that it prevents the output file from silently being truncated, it could also just be made the default behavior. I would argue for that, but to be conservative, I have opted to implement it as optional for now.

lagmoellertim commented 1 year ago

Hey, thanks for your PR! I also think we should make this check the default behavior, since new users might be thrown off by the corrupted output file and searching for a flag to fix it seems like a bad workaround. Maybe we could name the flag --skip-interval-check or --skip-validation instead (or something different if you can think of anything else)?

Garbaz commented 1 year ago

Okay, made it the default behavior. I named the option --skip-check-intervals now, does that sound okay?

lagmoellertim commented 1 year ago

Should we merge this in or should I wait for the PR you announced that would check for too small intervals after speed up? Because with that fix, running ffprobe on every file would be overkill, so it should be an optional flag.

Garbaz commented 1 year ago

You can merge this PR for now. I will try to do the better fix this weekend, but it depends on how much time I have. I'll just make another PR for that.

And good idea, I will keep this functionality in and make it optional, once the better fix is complete.

Garbaz commented 1 year ago

Alright, I implemented a minimum interval length now. I set it to .25s instead of .1s, since with .1s for some reason I got one interval (though a completely different one than before), which didn't concat at all. 0.2s worked as well, but to be safe, I put it at 0.25s.

And the checking with ffprobe is off by default again.

lagmoellertim commented 1 year ago

Hey, amazing work πŸ”₯ could we make the minimum interval size configurable using flags? In case some user has an ffmpeg version that only works for intervals >= 0.5s, that could be adjusted on the fly.

Garbaz commented 1 year ago

Sure, more customizability won't hurt :)

Btw, I also tried out different versions of ffmpeg (my distro is still on 4.2.7), but even with the newest binary, I didn't see any difference.

lagmoellertim commented 1 year ago

Perfect πŸ‘Œ I currently don't have a PC at hand, but next week I think I am able to test your changes and merge it in πŸ‘

Garbaz commented 1 year ago

Cool πŸ‘