skiptosilence: disable video/subtitle while skipping #12
Conversation
Disabling the video and subtitle while skipping instead of applying a blackout filter, fixes detuur#3. When there are too many packets in the demuxer packet, caused by exceeding the expected inside a packet then skipping will not stop and result in the action to never end. This is resulted from subtitles and video output from my testing. Also fixed the script-opts template in the same PR.
|
Wonderful. That bug has been a thorn in my sides since I never actually managed to replicate it in the first place. I'll test the changes later today so expect a merge by tomorrow. I never even knew the opts template was wrong... |
|
There are also additional possible enhancements. I'll probably make enhancements to this PR or add additional one. In case a file that doesn't have any moment of silence, it is better to add an It is also possible to include Also, a nice feature could be is to set a configurable maximum silence length, so it breaks the functions (or doesn't even initiate "dont know the possibility for now", if the next silence point exceeds the configured time) e.g.: 120 seconds. |
Multiple enhancements and additions including: **Enhancements:** * Handle pause and file ending while skipping is triggered. * Pausing will revert to the initial trigger point and pause (cancels on-going skipping). * Changing / ending file will stop skipping towards the new loaded file. **Fixes:** * Fixed spamming `doSkip` (f3), causing crash. * Fixed script getting stuck when initiating `doSkip` (f3) at the end of file. **Miscs** * Removed mute option since mpv by default mutes speed 4x and above. * Renamed `duration` to `silence_duration` since other duration options are planned. * Removed obsolete filter comments and moved dev note up. * Changed indentation to tab.
|
I have made drastic changes to be able to include additional options. When I am not changing the script too much, I could be submitting a PR, however maintaining two separate projects of similar functionality will be an overhead for me. |
Fix for ignore anything less than a second ahead caused, it was caused due to disabling video while seeking.
Fixed crashes related to changes in previous PR. Renamed user config to make it easier for users. Added multiple new options: Reverted removal of force mute: (force_mute_on_skip) Added the following new options: ignore_silence_duration=10 min_skip_duration=0 max_skip_duration=120 force_mute_on_skip=no osd_msg=yes
Slight update for config template
|
The above commits adds many new features and enhancements, which are not related to the bug. I think now I poured most of the ideas I have for skiptosilence, thank you for the awesome script =) |
Fixed wrong height and width as osd should be used. Added force-window so that mpv doesn't minimize when launched via terminal.
removed dev comments to make script smaller.
autoload script caused script to get stuck due to loading of sid. removed changing sid state and replaced with sub-visibility.
There was a problem hiding this comment.
I've done a bit of a formal code review, since the changes have gotten pretty big. Your code looks good! I have mostly stylistic remarks.
Just a general warning: I write my review comments in a pretty brusque style. This means that they read a lot "ruder" than they really are, but writing them in a polite way would be a very exhausting exercise.
| quietness = -30, | ||
| duration = 0.1, | ||
| mutewhileskipping = false | ||
| local o = { |
There was a problem hiding this comment.
Don't use single-character variable names. This variable is impossible to search for in the code.
This is also an instance of "don't change variable names".
| # -10 is more tolerant to noise. | ||
| quietness = -30 | ||
| #--(#number). Maximum amount of noise to trigger, in terms of dB. Lower is more sensitive. | ||
| silence_audio_level=-40 |
There was a problem hiding this comment.
Avoid changing variable names unless they're outright wrong or conflict with new variables. Changing them results in needless noise in the git diff/history.
| -- If we don't wait at least 50ms before messaging the user, we | ||
| -- end up displaying an old value for time-pos. | ||
| mp.add_timeout(0.05, skippedMessage) | ||
| if value == "{}" or value == nil then |
There was a problem hiding this comment.
Don't change the code style. By changing spaces to tabs for the entire file, this line shows up in the diff while nothing actually changed. In general, code style changes are pretty disruptive to git history and should be avoided unless technically required.
| -- end up displaying an old value for time-pos. | ||
| mp.add_timeout(0.05, skippedMessage) | ||
| if value == "{}" or value == nil then | ||
| return |
There was a problem hiding this comment.
In a similar vein to the previous note, avoid removing (or even spell-checking!) comments unless something about them is wrong. This is another source of noise in git diff/history and reviews.
| * script-opts/skiptosilence.conf in mpv's user folder. The | ||
| * parameters will be automatically loaded on start. | ||
| * | ||
| * Dev note about the used filters: |
There was a problem hiding this comment.
Minor nitpick: the dev note should stay in the code. For userscripts, the header is aimed at users, not devs.
| force_mute_on_skip=no | ||
| #--(yes/no). Display osd messages when actions occur. | ||
| osd_msg=yes |
There was a problem hiding this comment.
I'm on the fence about this option. I don't like it from the perspective of hiding/showing debug messages (like "silence was less than minimum" on line 102). These messages don't belong in the OSD; users can find them in the terminal.
On the other hand, this option is valuable for people who want to hide "skipped to silence at XXX" (line 116).
Additionally, on line 108 I've found another use for this option. Weigh in with your opinion.
| end | ||
| if o.max_skip_duration > 0 and skip_duration >= o.max_skip_duration then | ||
| restoreProp(initial_skip_time) | ||
| if o.osd_msg then mp.osd_message('Skipping Cancelled\nSilence is more than configured maximum') end |
There was a problem hiding this comment.
Could be changed to:
No silence found in the next XXX seconds.
See max_skip_duration in skiptosilence.lua
This message in particular will be seen by many users who won't have bothered to read the header. In this case we could even change it to something like this?
No silence found in the next XXX seconds.
See README section in skiptosilence.lua
What do you think? I want to move away from osd_msg but it could be repurposed as a yes_I_have_read_the_readme sort of variable. When set to yes it would only show the first line.
|
Totally agree with all your comments, things got mixed with the branch in where I was planning to make drastic changes, and the one I am doing enhancements / fix for PR. |
|
Let's get rid of |
|
Apologies, been very busy lately. |
|
Take your time. There's no urgency here. |
skiptosilence.lua 脚本合并来自 detuur/mpv-scripts#12 的更改
skiptosilence.lua 脚本合并来自 detuur/mpv-scripts#12 的部分更改并优化
skiptosilence.lua 脚本合并来自 detuur/mpv-scripts#12 的部分更改并优化
skiptosilence.lua 脚本合并来自 detuur/mpv-scripts#12 的部分更改并优化
skiptosilence.lua 脚本合并来自 detuur/mpv-scripts#12 的部分更改并优化
skiptosilence.lua 脚本合并来自 detuur/mpv-scripts#12 的部分更改并优化
skiptosilence.lua 脚本合并来自 detuur/mpv-scripts#12 的部分更改并优化
skiptosilence.lua 脚本合并来自 detuur/mpv-scripts#12 的部分更改并优化
skiptosilence.lua 脚本合并来自 detuur/mpv-scripts#12 的部分更改并优化
skiptosilence.lua 脚本合并来自 detuur/mpv-scripts#12 的部分更改并优化
2 participants
Disabling the video and subtitle while skipping instead of applying a blackout filter, fixes #3.
When there are too many packets in the demuxer packet, caused by exceeding the expected inside a packet then skipping will not stop and result in the action to never end. This is resulted from subtitles and video output from my testing.
Also fixed the script-opts template in the same PR.