Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

options: add --sub-newline-to-space #13139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

karelrooted
Copy link
Contributor

Enables replace newline with space in subtitles when they are available, if the subtitles are in a plain text format (or ASS if --sub-ass-override, --secondary-sub-ass-override is set to strip). Useful when secondary sub postion is adjacent to main subtitle (Default: no).

replace newline with space when display two subtitle at the bottom will have a better user experience, is adding a new option --sub-newline-to-space acceptable?

Enables replace newline with space in subtitles when they are available,
if the subtitles are in a plain text format (or ASS if
``--sub-ass-override``, ``--secondary-sub-ass-override`` is set to strip).
Useful when secondary sub postion is adjacent to main subtitle.

    Default: no.
Copy link

Download the artifacts for this pull request:

Windows

@llyyr
Copy link
Contributor

llyyr commented Dec 20, 2023

Do you have an example where this is useful? How frequently does this option help? This seems too niche to be a mpv option

@paulguy
Copy link

paulguy commented Dec 20, 2023

Could a script process subtitles?

@karelrooted
Copy link
Contributor Author

karelrooted commented Dec 20, 2023

basically when enable secondary subtitle at the bottom with main subtitle, you will want to enable this option , otherwise the experience is poor

without sub-newline-to-space ( without overlap minimum secondary-sub-pos=84)

Screenshot 2023-12-20 at 11 50 25 PM-min

with sub-newline-to-space ( without overlap minimum secondary-sub-pos=92)

Screenshot 2023-12-20 at 11 55 49 PM-min

extra backgroud info:
I have a local bilingual player build ontop of libmpv (with on the fly translated secondary subtitle), if combine the two subtitle into one and use sub-reload to reload the subtitle on the fly, sometimes when sub-reload is triggered, the video will pause for a little while and affect the user experience , so for better user experience, the solution is change to use mpv's native sid, secondary-sid option, this way , when the video has built in sid={source_language} , secondary-sid={target_language}, there is no sub-reload needed, otherwise because mpv only has the subtitle info currently loaded but not the entire subtitle, sub-reload is required to refresh the newly combined bilingual subtitle

@dyphire
Copy link
Contributor

dyphire commented Dec 21, 2023

As mentioned above, this usage scenario is too niche and has significant limitations.
There are three common uses of line breaks in subtitles.

  • Used to split long sentences and avoid overflowing the screen
  • Used for segmenting different language texts in bilingual subtitles
  • Splitting dialogue content between different characters at the same time

The new option may only be beneficial for the first uses and may disrupt the other two uses.
I think you should try adding a new sub-filter-regex-* option to achieve a more universal subtitle content replacement function.

@karelrooted
Copy link
Contributor Author

karelrooted commented Dec 21, 2023

As mentioned above, this usage scenario is too niche and has significant limitations.

There are three common uses of line breaks in subtitles.

  • Used to split long sentences and avoid overflowing the screen

  • Used for segmenting different language texts in bilingual subtitles

  • Splitting dialogue content between different characters at the same time

The new option may only be beneficial for the first uses and may disrupt the other two uses.

I think you should try adding a new sub-filter-regex-* option to achieve a more universal subtitle content replacement function.

Libass has built in overflow detect and soft wrap accordingly

Basic on my local test, most movie and tvshow will be fine(mostly with built in English subtitle, translated secondely sub is added on the fly), since is a option, we can programly switch it on and off anyway

sub-filter-regex-*, as the name suggests complete discard the match subtitle event (I didn't tested or check it thoroughly though, only briftly read the doc)

have consider add a more universal sub-replace-regrex option, but it seems overkill for this replace newline to space purpose

@dyphire
Copy link
Contributor

dyphire commented Dec 21, 2023

Basic on my local test, most movie and tvshow will be fine(mostly with built in English subtitle, translated secondely sub is added on the fly), since is a option, we can programly switch it on and off anyway

As mentioned above, the usage scenario you mentioned is too niche, and I personally believe that adding a dedicated option for this is not necessary enough.

sub-filter-regex-*, as the name suggests complete discard the match subtitle event (I didn't tested or check it thoroughly though, only briftly read the doc)

have consider add a more universal sub-replace-regrex option, but it seems overkill for this replace newline to space purpose

The benefit of adding a universal sub-replace-regrex option is that it can address more potential usage needs, rather than adding separate options for each usage scenario.

@Dudemanguy
Copy link
Member

I agree that a general find/replace would be a better solution. The only problem is that I don't think the regex filter actually works on windows? Not sure.

@karelrooted
Copy link
Contributor Author

karelrooted commented Dec 21, 2023

As mentioned above, the usage scenario you mentioned is too niche, and I personally believe that adding a dedicated option for this is not necessary enough.

The benefit of adding a universal sub-replace-regrex option is that it can address more potential usage needs, rather than adding separate options for each usage scenario.

as more and more videos include built in multi language subtitle, the usage scenario will increase(assuming more people will want to display two subtitle at the bottom as the issue #3022 suggest)

a universal sub-replace-regrex options does seems more appropriate though, is being so many years,but there is no sub-replace option,so i assume is not added by design(only filter to discard the entire event)

@karelrooted
Copy link
Contributor Author

I agree that a general find/replace would be a better solution. The only problem is that I don't think the regex filter actually works on windows? Not sure.

does --sub-replace-string more suitable for this option than --sub-replace-regex,it cover more usage case while maintaining simplicity, and has the benefit of supporting no posix system(like windows)

@Dudemanguy
Copy link
Member

Sorry for the really late reply here. The thing is that if we add a really simple --sub-replace-string or something like that, someone is going to eventually want a more powerful way to find/replace stuff which means people are going to want regex or something close to it.

We do actually have --sub-filter-jsre which exists essentially as a way to do --sub-filter-regex on non-posix. I know it's more work but probably this feature should be implemented like that. You could just do whichever implementation suits your usecase (the regex or javascript one).

@kasper93 kasper93 added priority:stalled priority:needs-author-feedback The original author of the issue/PR needs to come back and respond to something labels Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:needs-author-feedback The original author of the issue/PR needs to come back and respond to something priority:stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants