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

PAL can't differentiate SDH and regular subtitle #106

Open
gotson opened this issue Nov 19, 2024 · 23 comments
Open

PAL can't differentiate SDH and regular subtitle #106

gotson opened this issue Nov 19, 2024 · 23 comments

Comments

@gotson
Copy link

gotson commented Nov 19, 2024

i have a show where there are 2 english subs: English and English SDH. When i select English, PAL sets English SDH for the whole show.

Here is what the XML contains:

<Stream id="119071" streamType="3" canAutoSync="0" codec="srt" index="11" bitrate="0" language="English" languageTag="en" languageCode="eng" hearingImpaired="1" title="SDH" displayTitle="English SDH" extendedDisplayTitle="SDH (English SRT)"></Stream>
<Stream id="119072" streamType="3" canAutoSync="0" selected="1" codec="srt" index="12" bitrate="0" language="English" languageTag="en" languageCode="eng" displayTitle="English" extendedDisplayTitle="English (SRT)"></Stream>

It seems one has hearingImpaired="1", which should be sufficient for PAL to use the other one, and not only match on languageTag.

@JourneyOver
Copy link

JourneyOver commented Nov 22, 2024

Sadly it's not as simple as just looking at that attribute, as I was looking in the xml for some of my videos that have both English and English SDH and almost none of my videos had the hearingImpaired attribute except for a couple of them.

@gotson
Copy link
Author

gotson commented Nov 23, 2024

Sadly it's not as simple as just looking at that attribute, as I was looking in the xml for some of my videos that have both English and English SDH and almost none of my videos had the hearingImpaired attribute except for a couple of them.

the bad quality of subs out there should not prevent the good subs to be chosen.

@dekaidekai
Copy link

dekaidekai commented Dec 22, 2024

I think I agree with @gotson, looking at some of my shows XML data in Plex, Plex seems to include the SDH in the title of a subtitle stream (ex. displayTitle="Japanese SDH") when it contains the value hearingImpaired=1. So it seems like Plex knows how to differentiate SDH vs non SDH to some degree.

I understand that maybe just looking for hearingImpaired=1 will not catch all the cases of subtitles that may be SDH or not, but won't adding some rule to search for the hearingImpaired=1 at least catch the obvious ones that are SDH? Similar to how Plex seems to be doing it with naming subtitles on their UI?

@dekaidekai
Copy link

dekaidekai commented Dec 22, 2024

Did a little digging and I think the model plexapi.media.SubtitleStream we use in the code already offers the boolean member hearingImpaired, which indicates True if this is a hearing impaired (SDH) subtitle..

Similar how the code already uses the .forced member with SubtitleStream, with .hearingImpaired I think we can do something like the code below in the function _match_subtitle_stream to handle differentiation of SDH subtitles.

    def _match_subtitle_stream(self, subtitle_streams: List[SubtitleStream]):
        # If no subtitle is selected, the reference stream can be 'None'
        if self._subtitle_stream is None:
            if self._audio_stream is None:
                return None
            match_forced_only = True
            language_code = self._audio_stream.languageCode
        else:
            match_forced_only = self._subtitle_stream.forced
            match_hearing_impaired_only = self._subtitle_stream.hearingImpaired   # check if subtitles is SDH
            language_code = self._subtitle_stream.languageCode
        # We only want stream with the same language code
        streams = [s for s in subtitle_streams if s.languageCode == language_code]
        if match_forced_only:
            streams = [s for s in streams if s.forced]
        if match_hearing_impaired_only:                                         # filter out to SDH if required
            streams = [s for s in streams if s.hearingImpaired]
        if len(streams) == 0:
            return None
        if len(streams) == 1 or match_forced_only:
            return streams[0]
        # If multiple streams match, order them based on a score
        scores = [0] * len(streams)
        for index, stream in enumerate(streams):
            if self._subtitle_stream.forced == stream.forced:
                scores[index] += 3
            if self._subtitle_stream.hearingImpaired == stream.hearingImpaired:      # add score for SDH subtitles
                scores[index] += 3
            if self._subtitle_stream.codec is not None and stream.codec is not None and \
                    self._subtitle_stream.codec == stream.codec:
                scores[index] += 1
            if self._subtitle_stream.title is not None and stream.title is not None and \
                    self._subtitle_stream.title == stream.title:
                scores[index] += 5
        return streams[scores.index(max(scores))]

JourneyOver added a commit to JourneyDocker/Plex-Auto-Languages that referenced this issue Dec 22, 2024
@JourneyOver
Copy link

JourneyOver commented Dec 22, 2024

I just pushed up the change noted by @dekaidekai, so hopefully it'll work. can test by either pulling ghcr.io/journeydocker/plex-auto-languages:main or journeyover/plex-auto-languages:main if everything works then I may make a versioned release since it's been a tiny bit.

@dekaidekai
Copy link

dekaidekai commented Dec 22, 2024

@JourneyOver Thanks for the quick patch 🙇

Is there any setup steps different required for your image as opposed to remirigal/plex-auto-languages:latest?

The image remirigal/plex-auto-languages:latest works for me, but tried running your image and got this error on container start up:

2024-12-22 18:10:21,482 [INFO] Setting value of parameter PLEX_URL from environment variable
2024-12-22 18:10:21,482 [INFO] Setting value of parameter PLEX_TOKEN from environment variable
Traceback (most recent call last):
  File "/app/main.py", line 132, in <module>
    plex_auto_languages = PlexAutoLanguages(args.config_file)
  File "/app/main.py", line 22, in __init__
    self.config = Configuration(user_config_path)
                  ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
  File "/app/plex_auto_languages/utils/configuration.py", line 59, in __init__
    self._validate_config()
    ~~~~~~~~~~~~~~~~~~~~~^^
  File "/app/plex_auto_languages/utils/configuration.py", line 116, in _validate_config
    if self.get("data_path") != "" and not os.path.exists(self.get("data_path")):
       ~~~~~~~~^^^^^^^^^^^^^
  File "/app/plex_auto_languages/utils/configuration.py", line 66, in get
    return self._get(self._config, parameter_path)
           ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/plex_auto_languages/utils/configuration.py", line 73, in _get
    return config[parameter_path]
           ~~~~~~^^^^^^^^^^^^^^^^
KeyError: 'data_path'

Here's my setup:

plexautolanguages:
    image: journeyover/plex-auto-languages:main
    container_name: plexautolanguages
    restart: unless-stopped
    networks:
      - local
    environment:
      - PLEX_URL=http://plex:32400
      - PLEX_TOKEN=$PLEX_TOKEN
      - TZ=$TZ
    volumes:
      - $DOCKERDIR/plexautolanguages:/config

I don't mount any config.yaml file for the settings. I'm guessing with this image I'm required to have a config.yaml and set data_path with an empty string ""?

@JourneyOver
Copy link

JourneyOver commented Dec 23, 2024

@dekaidekai You don't need to exactly mount the config.yaml file, just the folder like you are already.. The whole data_path thing comes from this PR #89 (tbh I might revert this pr on my side though xD)

It should be setting a data directory by default if one is not provided, so I'm not sure why it didn't for you but you should be able to pass the values regardless either way through the env variables but I might need to recheck all that again to make sure it's all actually working as expected.

JourneyOver added a commit to JourneyDocker/Plex-Auto-Languages that referenced this issue Dec 23, 2024
@JourneyOver
Copy link

JourneyOver commented Dec 23, 2024

So I'm a moron @dekaidekai and accidentally left the data_path bit commented out when I was messing with the default.yaml file in a recent commit. That was my mistake and it should hopefully now be rectified and fixed. so feel free to test again!

@dekaidekai
Copy link

@JourneyOver You are not a moron at all 😎 thanks again for the quick replies and fixes!
I'll try to test out tonight or tomorrow!

@JourneyOver
Copy link

hehe it's not a problem :P Sounds good, lemme know how testing goes when you get to it so I can know if the actual main issue is fixed or not @dekaidekai :) (even if technically I won't be able to close this issue)

also wish I could find fixes for some of the other issues that have been cropping up in this repo, but I sadly haven't experienced/ran into any of the ones that some people seem to be running into >.<

@dekaidekai
Copy link

dekaidekai commented Dec 23, 2024

@JourneyOver I have tested the journeyover/plex-auto-languages:main image and it seems to successfully be setting and differentiating between SDH and non SDH subtitles! 🎉

Thank you so much for the patch and your hard work 🙏

@JourneyOver
Copy link

JourneyOver commented Dec 23, 2024

@dekaidekai Glad to hear! and it's not a problem, Always down to try to fix something if I can, though I do have to thank you for the code as well as you are the one who posted mostly about it above :P

Guess we can officially call this issue closed.

@gotson Feel free to close this issue if it ends up working for you as well (if you ever read this and check it out), otherwise I guess this will remain open even though a fix is now out unless @RemiRigal decides to randomly show up and close this xD


Edit: I have now officially made a new release as well (starting with version 1.3.0 since the last technical release from at least Remi was 1.2.3 and I never changed any versioning between then and now until this release).

So now we have three tags for pulling from right now 1.3.0, latest and main for at least my image.

@dekaidekai
Copy link

dekaidekai commented Dec 23, 2024

@JourneyOver Awesome thanks again!

Actually doing a little more testing and I think I see some other issues, but these might be unrelated due to other things like caching (specifically the Skip if selected streams are unchanged when it shouldn't skip) and working with Infuse playback as well.

I'll be fooling around trying to make fixes for these and I might post my findings later in another issue!

@JourneyOver
Copy link

@dekaidekai sounds good, if you do just go ahead and ping me as your help in getting fixes out (especially for things I don't run into myself) is greatly appreciated.

@dekaidekai
Copy link

dekaidekai commented Dec 23, 2024

@JourneyOver You are a saint thank you 🙇

The new issue I found I posted here! #109

@JourneyOver
Copy link

@dekaidekai Hmm so we need to come up with a different method for this I think, as currently what seems to be happening is that if anything is added that actually had SDH subtitles, it automatically gets forced to use those subtitles when the content gets added in and scanned just like "forced" has always done. idk if it's due to scoring or something else but I'm trying to figure it out fully and get a solution out.

@dekaidekai
Copy link

dekaidekai commented Dec 25, 2024

@JourneyOver Thanks for testing this out! I think then it would maybe be due to the scoring, do we know what we want for the intended behaviour? To make it pick forced subtitles automatically when it's added in?

I can take a look at the code and investigate!

@dekaidekai
Copy link

dekaidekai commented Dec 25, 2024

This is another issue I might have found unrelated, but if you set the subtitles to off, I think it crashes 🤔

new issue tracked here!
#110
I can try to look around for something on this too sometime outside of this current issue!

@JourneyOver
Copy link

@JourneyOver Thanks for testing this out! I think then it would maybe be due to the scoring, do we know what we want for the intended behaviour? To make it pick forced automatically when it's added in?

I can take a look at the code and investigate!

@dekaidekai

I think the way things should be was how it was before in a sense:

If there is a Forced Subtitle, PAL can automatically select and set it for that episode and any other episodes that have Forced Subtitles, otherwise leave it at off or the user selected subtitles.

If user has an SDH subtitle do not automatically select it even on initial scans and such unless the user has specified gone in and set it to be selected even if it is the only subtitle. (e.g: if user has two subs for a video one SDH and one non-SDH and user selects the non-SDH then it should go through only set the videos that has the non-SDH while leaving the rest off, if user only has one sub and it's SDH do not automatically set it and just leave it off unless the user has gone in and selected it himself)

Currently from my testing where I have a video that only has one subtitle and it's SDH, it's automatically setting said subtitle to SDH even if I go in and reset the subtitle back to off, next scan it'll just go and reset it back to the SDH subtitle.

@JourneyOver
Copy link

I've gone ahead and moved this issue over to my own repo as well.

@gotson
Copy link
Author

gotson commented Jan 2, 2025

Hi !

i tried with journeyover/plex-auto-languages:latest and it seems to work.

What's the relationship between that image and remirigal/plex-auto-languages:latest though ?

@JourneyOver
Copy link

JourneyOver commented Jan 2, 2025

Hi !

i tried with journeyover/plex-auto-languages:latest and it seems to work.

What's the relationship between that image and remirigal/plex-auto-languages:latest though ?

The main difference is that Remi is no longer maintaining this repository, so there haven’t been any updates to his image. In contrast, my fork, is actively maintained, includes fixes (like one that corresponds to this issue), and is kept up to date.

Essentially, I’ve taken over the project in a fork and am working on addressing issues and trying to implement fixes as they arise.

If you’re okay using an outdated image without recent fixes or updates (and it’s unlikely that Remi will return to maintain the project), you can stick with remirigal/plex-auto-languages:latest. However, if you prefer an actively updated image with ongoing fixes and improvements, I recommend switching to my fork.

@gotson
Copy link
Author

gotson commented Jan 2, 2025

thanks for the details. I don't know if you managed to get in touch with Remi, but maybe he could add you as a maintainer of the project ? Or somehow update the readme to say this is not maintained.

I switched to your fork :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants