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 caching subtitle streams issue #109

Closed
dekaidekai opened this issue Dec 23, 2024 · 5 comments
Closed

PAL caching subtitle streams issue #109

dekaidekai opened this issue Dec 23, 2024 · 5 comments

Comments

@dekaidekai
Copy link

dekaidekai commented Dec 23, 2024

I might be wrong on how PAL is supposed to behave, but I believe how PAL caches streams to ensure that it does not need to update future episodes again when users play the episodes again might be causing some issues when switching back and forth between languages.

Example flow of reproducing the issue

(you can pick any two different languages to test this out)

  1. Start PAL container fresh with no cache and empty plex.cache.default_streams.
  2. Open show's first episode, setting subtitles language to Arabic
    a. This will cache this episode's stream to dict key and value /library/metadata/25911: (155623, 155627).
    b. All future episodes are set to be Arabic. Thanks PAL!
  3. Actually...nvm I want the show subtitles to be in Japanese.
  4. Open show's first episode again, setting subtitles language to Japanese
    a. PAL will check here, is the episode's stream the same as the cached one? No it's not since it's now Japanese and a different value; /library/metadata/25911: (155623, 155641), so PAL continues to change the show's subtitles!
    b. ❗ PAL will NOT cache this new set Japanese stream for this episode here. Because we use the method setdefault, which if a key is already set in a dict, will not override it's value. Since we already set this episode key in step 2a., then the cache will not be overridden, and the cached value for this episode stream is still Arabic; /library/metadata/25911: (155623, 155627).
    c. Even though it didn't cache to Japanese, PAL code continues and all future episodes subtitles are set to be Japanese. Thanks PAL!
  5. Actually...nvm again! I want the show subtitles to be in Arabic again.
  6. Open show's first episode, setting subtitles language back to Arabic
    a. ❗ Again, PAL will check here, is the episode's stream the same as the cached one? Well, since we never updated the cache to be Japanese, the cached value is still Arabic; /library/metadata/25911: (155623, 155627), and it thinks the stream is unchanged, and returns early!
    b. ❗PAL will NOT set any of the future episodes subtitles language to Arabic.

This essentially means since the cache has the langauge Arabic stream value stuck in it for this episode and it cannot be updated, we can't switch the show's subtitles back to Arabic, unless we recreate the container and wipe the cache.

Proposed fix:

I think the issue is when we cache the episode's stream value here. We use setdefault instead of a method that should always override the cached value like:

replace this line with:

plex.cache.default_streams[item.key] = pair_id
@dekaidekai
Copy link
Author

dekaidekai commented Dec 23, 2024

cc @JourneyOver 🙏

@JourneyOver
Copy link

Hmm this bug seems to only be an issue when the update_strategy is set specifically to next instead of all as I did a test running through your example flow above with my config file which had it set to all and I wasn't experiencing this issue, so decided to do the test again with it set to next and ran straight into the issue of it not switching the subtitle language back.

I'll do a quick test with the proposed fix to see if it affects the update_strategy any when switching between the two different settings and if it doesn't then I'll push up the changes so you can test and see if everything is working correctly on your end and then we can call this fixed or not depending on things.

JourneyOver added a commit to JourneyDocker/Plex-Auto-Languages that referenced this issue Dec 23, 2024
This issue seemed to only happen specifically when the `update_strategy` was set to `next`, setting it to `all` never seemed to run into this issue.

Fixes RemiRigal#109
@JourneyOver
Copy link

JourneyOver commented Dec 23, 2024

Have pushed up a new build with the proposed change, I did a small test beforehand and it didn't seem to cause any adverse affects, but feel free to test and confirm/deny if everything is working okay on your end.

I've also gone ahead and allowed issues over on my repository now as well.

@dekaidekai
Copy link
Author

dekaidekai commented Dec 23, 2024

@JourneyOver Good point sorry I forgot to mention I was in next strategy! I'll test both modes as well.

Thanks as always for these quick fixes 💯

@JourneyOver
Copy link

It's all good :P thankfully I remembered that some of my settings are different than the default settings, so I decided to check it out with default settings which for sure ran into the issue.

And it's not a problem, gotta thank you for the proposed code changes as well as it makes getting these fixes out quickly :)

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

2 participants