improve subtitle filename rename #23
Conversation
|
From issue:
Another note: Kodi also removes common prefix with video name. |
|
@joelpurra What do you think of this? |
|
@berkanteber: have some thoughts. Won't have time to neither test this PR nor review closer until earliest next week; I'll reply further then. Brain dump:
At this point I am actually unsure if manipulating subtitle filenames helps, or disrupts, Kodi's subtitle handling. Perhaps it's easiest to just leave all subtitle filename (assuming that the filename is not shortened) parsing to Kodi? Would have to investigate several test cases first. |
👍
It does, but it does poorly (requires a certain way). This all started by me seeing Hindi for Hearing Impaired the other day and thinking we could do better. Can Kodi do better than us? For folder subtitles, maybe. For file subtitles and OpenSubtitles subtitles, probably no.
I don't know about Kodi but we also sometimes shorten them. We try to do it nice (preserve extension & added language in name).
I don't know about this. I assume external always comes first, so duplicates wluld be at the bottom. But if Kodi can handle it, we could skip
We don't parse or keep these properties. If we decide to let Kodi handle file subtitles (point above), maybe these can be deduced. Parsing these ourselves seem to complicated.
I think saying unknown is better than sometimes identifying in wrong. I could trust Kodi extract from file, but parsing filename seems error prone. |
There was a problem hiding this comment.
Sorry for the delay. Several notes in the code review overlap the below comments. (Did not yet run the code, only read it.)
I assume that Kodi already can/will parse the subtitle filename to guess the language.
- putio-kodi adding the language code to the filename when it is parsed from the filename is unnecessary. Kodi probably does general parsing better.
- Externally acquired information, perhaps tagging from opensubtitles, might help though. (I already spotted opensubtitle language tagging errors though.)
It does, but it does poorly (requires a certain way). This all started by me seeing Hindi for Hearing Impaired the other day and thinking we could do better. Can Kodi do better than us? For folder subtitles, maybe. For file subtitles and OpenSubtitles subtitles, probably no.
Kodi does parse the subtitle filenames provided by putio-kodi, as made evident by the parsed English - Video (External) format in kodi's subtitle picker (for parseable filename formats). (This should have been obvious to me earlier, but didn't think of the full language name prefix as having been parsed already.)
Parsing seemingly works the same way it would if .srt files were found on the local disk (where putio-kodi does place them) next to the video file. While I can agree that kodi is not the best at parsing out languages, the format is at the very least established both by kodi and (often) matches filenames provided opensubtitles (where uploaders choose the filename).
The language extraction logic in putio-kodi for folder subtitles seems to mirror that of kodi. The putio-kodi folder subtitle filename parsing merely duplicates the last part of the filename as the last-last part:
- Video.eng.srt
+ Video.eng.eng.srtThat does not add new information nor help kodi; I would remove this logic.
- https://kodi.wiki/view/Subtitles
- https://github.com/putdotio/putio-kodi/pull/23/files#diff-a6b15b003d6e376a2b0b6757c2c12a4b3b160bc424a981d638475a176c430b36R215
The subtitle filenames sometimes seem cut/shortened/ellipsised (
...).
- This could be due to the full file path being too long.
- Shortened names could lose information, in particular at the end.
- I forget if it is actively shortened by putio-kodi, or somewhere else?
I don't know about Kodi but we also sometimes shorten them. We try to do it nice (preserve extension & added language in name).
There is display name shortening logic in putio-kodi, but don't think it applies to subtitle filenames.
While there may be api filename length limitations, I would attempt to keep the full subtitle for kodi to parse. Tried to look at the special:// protocol path handling to see if it explicitly limits path length, but didn't see anything obvious -- which doesn't mean that there aren't (platform-specific) checks and limitations.
- https://github.com/putdotio/putio-kodi/blob/v3.1.0/resources/lib/putio.py#L156-L157
- https://github.com/xbmc/xbmc/blob/20.5-Nexus/xbmc/filesystem/SpecialProtocol.cpp#L119
- https://kodi.wiki/view/Special_protocol
Kodi can parse and extract subtitles directly from video files.
- Unsure if this requires a setting.
- Is duplicating file-internal subtitles confusing Kodi?
I don't know about this. I assume external always comes first, so duplicates wluld be at the bottom. But if Kodi can handle it, we could skip
mkvsource from returned list. It would make it strange though if file subtitles would be at the bottom. Haven't tested, so all are pure hypotheticals.
Kodi (seemingly) correctly parses/extracts from video files (mkv, and presumably mp4 and other container formats). These "internal" (as opposed to .srt files which are labeled "external") subtitles indeed end up at the bottom of the list of subtitles. The language code seems to be properly extracted, as are flags such as "default" and "forced" (unsure about CC/HI/SDH). They are displayed as English [default,forced] (N/M) where N and M is the subtitle index/count.
- There are useful settings to auto-enable CC/HI/SDH, but unsure exactly how they are applied for putio-kodi.
...- If "manipulating" the filename/language, does putio also need to perform such steps to not lose valuable variant information?
We don't parse or keep these properties. If we decide to let Kodi handle file subtitles (point above), maybe these can be deduced. Parsing these ourselves seem to complicated.
Kodi will parse the provided subtitle filename no matter what, so perhaps it doesn't matter whether putio/putio-kodi handles CC/HI/SDH separately.
For undefined/unknown language, do not add anything and leave filename parsing to Kodi.
...I think saying unknown is better than sometimes identifying in wrong. I could trust Kodi extract from file, but parsing filename seems error prone.
Explicitly adding unknown/undefined/undetermined short-circuits kodi's language parsing. While kodi may also fail to determine the language by looking at the subtitle filename, adding und tells it not to try (because und becomes the "language"). Adding und means that the parsed language will always -- not sometimes -- be identified wrongly. (You can argue whether or not "Hindi" is "more wrong" than "Undetermined", but since kodi does not incorrectly guess "Hindi" for each and every subtitle it will be "more wrong" to add und.)
| subtitle_language = subtitle['language_code'] or '' | ||
| if len(subtitle_language) == 3: # sometimes this returns wrong | ||
| subtitle_name = subtitle_name + ' - ' + subtitle_language | ||
| if subtitle_name.endswith('.srt'): |
There was a problem hiding this comment.
Hardcoded .srt suffix ignores other possible subtitle file formats and extensions. Filtering for .srt extensions may be performed upstream (putio, opensubtitles), meaning that there will only "ever" be .srt files, but this way is not particularly future-proof.
An improvement would be to match against a list of known subtitle file extension, and saving the match to be able to add the file extension back later.
| subtitle_name = subtitle_name[:-4] | ||
|
|
||
| subtitle_lang = subtitle['language_code'] or '' | ||
| if len(subtitle_lang) not in (2, 3): # sometimes this returns wrong |
There was a problem hiding this comment.
This sanity check is performed before the last place subtitle_lang is assigned a value. While further subtitle_lang assignments may have other checks, this sanity check would make more sense placed last.
| if len(subtitle_lang) not in (2, 3): # sometimes this returns wrong | ||
| subtitle_lang = 'und' | ||
|
|
||
| # language is unknown for folder subtitles, use last part if name ends with `.` + 2-3 chars |
There was a problem hiding this comment.
The filename format is not improved by this parsing:
- Video.eng.srt
+ Video.eng.eng.srtNo new information is added, in particular as it already mirrors kodi's own subtitle parsing (assuming that the language code is well-known).
|
|
||
| subtitle_lang = subtitle['language_code'] or '' | ||
| if len(subtitle_lang) not in (2, 3): # sometimes this returns wrong | ||
| subtitle_lang = 'und' |
There was a problem hiding this comment.
Having putio-kodi explicitly inserting the und (undetermined) language code when putio doesn't know the language code (in particular for folder subtitles) means that kodi's own subtitle parsing is "disabled" because und becomes the "language". Since the subtitle always has a language (even if it's not in the filename, metadata, etcetera) this means that und is always the wrong language code.
| subtitle_lang = parts[-1] | ||
|
|
||
| # add language even if it's also in name, this way shown name will always be the original name | ||
| subtitle_fullname = subtitle_name + '.' + subtitle_lang + '.srt' |
There was a problem hiding this comment.
The .srt file extension should mirror the file extension which was removed in the first step. Since both checks have .srt hardcoded, the .srt file extension is assumed to be the only possible case. Do notice that the first .srt check removing .srt only if it matches the file extension, but this line always adds the .srt file extension. This means that another file extension (such as .sub, .mks) will be incorrectly "extended" with .srt and have a file contents versus extension mismatch.
(There is also the possible case of having an invalid subtitle type file extension, such as a mistaken language code .eng suffix, but that would be covered by having a list of known subtitle file extensions in the first step and adding it back here (or not, for mismatched file extension).)
If removing the und fallback value above, make sure to not extend the filename with an empty subtitle_lang string (in addition to the . separator character); instead leave the filename as-is.
| # language is unknown for folder subtitles, use last part if name ends with `.` + 2-3 chars | ||
| if subtitle['source'] == 'folder': | ||
| parts = subtitle_name.split('.') | ||
| if len(parts) > 1 and len(parts[-1]) in (2, 3): |
There was a problem hiding this comment.
This is not a check for a valid language code; any random 2-3 characters will become the "language code". Pretty sure this will yield false matches, since it comes down to the somewhat random (video and) subtitle filenaming format(s) people use.
| if subtitle_name.endswith('.srt'): | ||
| subtitle_name = subtitle_name[:-4] | ||
|
|
||
| subtitle_lang = subtitle['language_code'] or '' |
There was a problem hiding this comment.
Looking at the putio api response for subtitles, it seems language_code will be provided as und for folder subtitles already on the api level. As commented elsewhere, I find that und reduces usability. I would add a check to also exclude und from "valid"/"useful" subtitles, such that language_code cannot be neither empty nor und.
| subtitle_lang = 'und' | ||
|
|
||
| # language is unknown for folder subtitles, use last part if name ends with `.` + 2-3 chars | ||
| if subtitle['source'] == 'folder': |
There was a problem hiding this comment.
Note that even if a subtitle with source folder has a valid language_code which is not und, it will be (possibly) overwritten with a locally parsed "language code" here. While the putio api may only return und for folder-sourced subtitles today, this is not a future-proof assumption.
An improvement would be to only perform filename parsing if the language_code is empty or und.
|
So, just writing here instead of replying to code comments one by one.
So, I say:
|
|
Note: There is a new format for settings which allows help text. We could explain the reason for file subtitles there. |
|
@berkanteber: quick notes; I'll come back to this when I can.
Since I had a bunch of code comments, I'm considering creating a separate draft pull request (based on your code) to show rather than tell. Again, I didn't have time to run the code yet -- sorry about that. |
It didn't sound like that. But thinking about the fact that Kodi already recognizes file subtitles, I thought we could use that. But there are some negatives as well (placement & all vs user languages), so I thought options would be better. If not options, it would be better to keep file subtitles IMO. But it wouldn't complicate too much (just filter by source), and if we make the option only for advanced/expert, it wouldn't confuse people. (I have no idea if people use advanced settings or not, I just assume not.)
This is the correct order generally. In all places I can think of, external subtitles comes first. External = folder generally. The issue is we use external for all subtitles, so order becomes: (1) folder, (2) file (user language), (3) OpenSubtitles, (4) file (by Kodi, all language). If we decide to keep all sources, this is OK, But just hiding 2 and using 4 makes the order bad.
No problem. |
Closes #22.