smacke / ffsubsync

Automagically synchronize subtitles with video.
MIT License
6.73k stars 276 forks source link

new subtitle file does not have attachments #126

Closed ghost closed 3 years ago

ghost commented 3 years ago

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

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

Philippe-Cholet commented 3 years ago

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 commented 3 years ago

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

Philippe-Cholet commented 3 years ago
  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 commented 3 years ago

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!