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

new subtitle file does not have attachments #126

Closed
ghost opened this issue Apr 18, 2021 · 4 comments
Closed

new subtitle file does not have attachments #126

ghost opened this issue Apr 18, 2021 · 4 comments
Labels
bug Something isn't working

Comments

@ghost
Copy link

ghost commented Apr 18, 2021

Some subtitle files have attachments (embedded fonts) but after using script attachments dissappear.

before: https://del.dog/ogepedingi
after: https://del.dog/inyrerygnu.txt

@ghost ghost added the bug Something isn't working label Apr 18, 2021
@Philippe-Cholet
Copy link

ffsubsync uses pysubs2 to handle various subtitle types, but from what I saw it seems pysubs2 does not consider embedded fonts. Maybe you could ask them to consider it.

@ghost ghost closed this as completed May 8, 2021
@ghost ghost reopened this May 9, 2021
@ghost
Copy link
Author

ghost commented May 9, 2021

pysubs2 now supports fonts but ffsubsync still doesn't add fonts to new subtitle file

@Philippe-Cholet
Copy link

Philippe-Cholet commented May 10, 2021

  1. Consider embedded fonts when parsing ASS/SSA files in GenericSubtitleParser.fit with a new argument fonts_opaque.
  2. Then the handler GenericSubtitlesFile must consider fonts_opaque argument at instance creations and when writing the output ASS/SSA file.

For the first point, in GenericSubtitleParser.fit method, I would add this argument to GenericSubtitlesFile.

https://github.com/smacke/ffsubsync/blob/master/ffsubsync/subtitle_parser.py#L125

fonts_opaque=(
    parsed_subs.fonts_opaque
    if isinstance(parsed_subs, pysubs2.SSAFile)
    else None
)

I note there is self._skip_ssa_info which is unclear to me. Can do the same for fonts_opaque if needed.

And after looking at appearances of GenericSubtitlesFile, add a line there:
https://github.com/smacke/ffsubsync/blob/master/ffsubsync/subtitle_transformers.py#L47

fonts_opaque=subs.fonts_opaque,

For the second point, I handle the new fonts_opaque argument like it handled info, since both seem optional.

https://github.com/smacke/ffsubsync/blob/master/ffsubsync/generic_subtitles.py#L90

class GenericSubtitlesFile:
    def __init__(self, ...):
        ...
        self._fonts_opaque = kwargs.pop('fonts_opaque', None)  # new line

    # New property
    @property
    def fonts_opaque(self):
        return self._fonts_opaque

    def offset(self, ...):
        ...
        return GenericSubtitlesFile(
            ...,
            fonts_opaque=self.fonts_opaque,  # new line
        )

    def write_file(self, ...):
        ...
        if self._sub_format in ('ssa', 'ass'):
            ...
            # 2 new lines
            if self.fonts_opaque is not None:
                ssaf.fonts_opaque = self.fonts_opaque
            ...
        ...

I think it should be enough to handle embedded fonts.

@smacke
Copy link
Owner

smacke commented May 10, 2021

Hi @Philippe-Cholet, thanks for the detailed investigation, and thanks @sethazazel for opening the original issue. The "skip_ssa_info" is only used for integration tests I think to keep the result the same as without the ssa info (I was bad and wrote a few brittle tests).

The other thing we need to do is bump the min required version of pysubs2 in requirements.txt.

I just pushed a new release of ffsubsync with these changes; they should be picked up after running

pip install --upgrade ffsubsync

Note that I didn't actually do any end-to-end test involving embedded fonts, so please let me know if you encounter any issues after the upgrade!

@smacke smacke closed this as completed May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants