Implement subtitle 24/25fps frame rate correction

[jmbreuer]

Description

Add an option to the subtitle dialog in video player to correct subtitle files that are mastered to a different frame rate than the media

Motivation and context

It's a common nuisance in Europe that 24fps original material is ... "remastered" to our 25fps TV standards - just by speeding it up with or without pitch correction, in essence.

Sometimes, one might only/easily get hold of media that is mastered in a certain way, but all easily accessible subtitle files are timecoded for the "other" mastering. I.e. you might have 25fps media and a subtitle file that assumes 24fps playback or vice versa. This causes subtitles to rapidly drift out of alignment (by one second every 25 seconds, roughly).

With Kodi already providing subtitle offsets, audio offsets etc to provide a better playback experience of non-ideal media, adding a way to correct for this seems sensible.

How has this been tested?

I've watched a season of a TV series that I only have in 25fps media using 24fps subtitle files. With the correction, those come out to be spot on.

What is the effect on users?

Pro: Users get easy access to correct subtitle files that assume a different frame rate from the media being played (currently, limited to both directions of the common 24/25fps case).

Con: There's one more option in the subtitle dialog 😉

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• None of the above (please explain below)

Checklist:

• My code follows the Code Guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have updated the documentation accordingly
• I have read the Contributing document
• I have added tests to cover my change
• All new and existing tests passed

(how do I run the existing tests? ... locally? - or do I just wait for Jenkins to pick this up...?)

Personal Note

It's been a while since I contributed to Kodi; please let me know how I can improve this / make things go smoother for everyone.

The change of std::vector<int> m_subtitleCapabilities to std::vector<IPlayerSubtitleCapabilities> m_subtitleCapabilities (in xbmc/video/dialogs/GUIDialogSubtitleSettings.h) is in there because my compiler was complaining about it, and I don't see why it shouldn't be type safe.

[jmbreuer]

OK, how do I make that "Mergeable" check happy? Did I overlook something in how to properly set up the PR?

[fuzzard]

Mergable checks will be handled by team members, don't worry about them

[CastagnaIT]

This feature seems particularly specific and limited for your purposes than for the community, should not allow a wider fps selection?

I have no precise judgement to make on this implementation but atm to me sound strange

It's a common nuisance in Europe that 24fps original material is ... "remastered" to our 25fps TV standards

"to our 25fps TV standards" what means to "our"? a country? please specify the country
it would be useful to understand which country is doing this, a video title example would be appreciated

original material is ... "remastered"

remastered is usually done to improve audio/video quality of original source and sell it with increased price for collectors, but different fps? and from different country? this is odd

the way you tell it sounds like you have "re-encoded" videos with different fps which is not so common
however subtitles editor software already have the feature to adapt subtitle timeline for re-encoded videos

maybe worth understanding how much user demand there is for this feature

[wsnipex]

25p /50Hz interlaced is used by PAL and SECAM standards:

PAL is an abbreviation for Phase Alternate Line. This is the video format standard used in many European countries. A PAL picture is made up of 625 interlaced lines and is displayed at a rate of 25 frames per second.

SECAM is an abbreviation for Sequential Color and Memory. This video format is used in many Eastern countries such as the USSR, China, Pakistan, France, and a few others. Like PAL, a SECAM picture is also made up of 625 interlaced lines and is displayed at a rate of 25 frames per second. However, the way SECAM processes the color information, it is not compatible with the PAL video format standard.

[jmbreuer]

This feature seems particularly specific and limited for your purposes than for the community, should not allow a wider fps selection?

I'm happy to extend it for other frame rate conversions, as it might apply. The 24/25 fps thing is the only common case I'm aware of, and the only one I've ever needed. That one fairly regularly, if not quite daily, though. For my usage pattern, it's something I encounter somewhere between once per week to once per month. "Once" gets old fast when it affects a whole TV series / season.

I have no precise judgement to make on this implementation but atm to me sound strange

It's a common nuisance in Europe that 24fps original material is ... "remastered" to our 25fps TV standards

"to our 25fps TV standards" what means to "our"? a country? please specify the country it would be useful to understand which country is doing this, a video title example would be appreciated

I meant "our" to refer to "Europe". Considering I grew up with / was regularly confronted by that particular nuisance, this information was actually surprisingly hard to find online:

https://www.lifewire.com/why-ntsc-and-pal-still-matter-1847856#toc-ntsc-and-pal

"Countries doing this" is blue and orange on this map, or put the other way around, everything that's not green.

This in particular includes every single "PAL" DVD pressing of originally 24fps movie material; as well as any TV broadcast of such.

Due to regional licensing I don't know how to point you to easily accessible examples (assuming you live in a 'green' country). Maybe order a "PAL" / "Region 2" DVD pressing of any 1990s blockbuster (very new transfers might use other technologies that don't exhibit this particular issue) from a UK online store that delivers internationally? And find a subtitle file on OpenSubtitles.org/addic7ed.com that would correspond to a "NTSC" / "Region 1" / 24fps / eg also BluRay/HD release of that movie.

In my latest specific case, it was a TV recording / PVR capture of "The Listener" Season 2 encoded at 25/50fps; while (all) the (non-HI, didn't look at the others) subtitle files available on addic7ed.com assume 24fps timecodes. It was just the straw that broke the camel's back for me to have a stab at implementing that feature in Kodi (which is basically the sole source device for my TV), rather than fudging around with the subtitle files on a PC. As it turns out, wasn't all that hard.

original material is ... "remastered"

remastered is usually done to improve audio/video quality of original source and sell it with increased price for collectors, but different fps? and from different country? this is odd

Yeah, it's "odd" alright, but it's what broadcasters did forever (since TVs for the longest time couldn't cope with changing frame rates). I'm not sure which term - "remastering", "reencoding", ... would most correctly apply here.

This talks about the PAL / 25/50 fps standard variant:
https://en.wikipedia.org/wiki/Telecine#2:2_pulldown

And this is what often happens/used to happen in NTSC / 30/60 fps regions:
https://en.wikipedia.org/wiki/Telecine#2:3_pulldown

Kodi explicitly implements appropriate pulldown correction on playback, in the same vein as de-interlacing. I feel my "feature" only extends this to what is, unfortunately, a very "normal" corner case of having a subtitle file that is timecoded for the "other" telecine variant, so to speak.

Modern TVs now can, but broadcast systems and institutions - here: at least Germany and UK from first-hand experience - are stuck in "we broadcast at one fixed frame rate forever" paradigm, which again here means 25p/50i. So this applies to all PVR recordings of originally 24fps material in affected countries as well.

the way you tell it sounds like you have "re-encoded" videos with different fps which is not so common however subtitles editor software already have the feature to adapt subtitle timeline for re-encoded videos

Yeah, it's just a nuisance to have a whole TV recorded show (with OVA even, yay!) and have to fudge around with all the subtitle files on a PC or such instead of just watching it on my Kodified TV and choose that correction at the click of the remote's button.

maybe worth understanding how much user demand there is for this feature

I guess there's "some" demand in all the traditionally 50fps countries. I do agree that it might be a somewhat niche feature, in the sense that a lot of people wouldn't know what the problem is even when they encounter it (subtitles drifting away over time). Is there a way to put a descriptive text with that option? I've seen some settings dialogs that show context help near the bottom; I don't know whether or how the subtitles dialog might support this, however.

On the other hand, there's all these video playback options concerning de-interlacing, video scaling, post-processing, tone-mapping... hah, I see it 😉 to me, this feature is on a similar level to the "Invert stereoscopic 3D mode (flip eyes)" one - it deals with an issue that stems in how video is encoded/mastered, and that can easily be corrected for in the player.

[mkreisl]

@jmbreuer , my first thought was, what a useless function, sorry.
24fps material you can search here with the lupe, especially when it comes to videos. There 23.976fps is actually represented exclusively. I encode a lot of videos and know what I'm talking about.
I would either do it like in the MKVToolnix GUI, that you can just enter a factor or implement all possible fps conversions as a dropdown menu like SubTitleEdit offers.

[jjd-uk]

I think the complete opposite.

In the former PAL regions we are still stuck with 25p or 50i for broadcast tv. So when a movie or many of the US tv shows is shown, the broadcaster has a master at 24fps, but will play it back at 25fps. So having movies at 25fps is common if you record with pvr.

For more background https://medium.com/@Jamiesgt/how-us-tv-is-broadcast-in-the-uk-75849920806a

[mkreisl]

@jjd-uk, for tv series this may be true, but this is supposed to be a universal function and not only for series that are watched via pvr. and for movies the situation is quite different. but i don't care either way, as far as i'm concerned i'll never need this anyway, i just wanted to share my opinion here

[scott967]

Can't comment on implementation, but from a requirements view, agree that the historical play 24 fps video back at 25 fps resulting in 4% speed-up is common and timing in time-code based sub formats (text) need to reflect that. Not an issue when 3/2 pulldown is used for 24/30 as the extra generated frames keep time consistent.

[CrystalP]

I could have used that feature a few times for some obscure movies / TV series when the only available subs have a different fps.

I think a list of presets is the right approach, and additional fps values/combinations would make this change applicable to more situations. I'd say that the most common fps are 23.976, 25 and 29.97, but 24 and 30 could be considered.

Since Kodi is playing the file, the destination fps should be known and the user could enter just the sub's original fps, no?

The number of messages should be minimized as they have to be translated to all languages so I think a single generic fps message with placeholders would be preferable, like "{0} to {1} fps" and calls to StringUtils::Format() to expand for each combination.
Not sure if fmt::formatter is better or not, there seems to be std::format also. The more standard the better.

[CastagnaIT]

@jmbreuer thanks for background info its more clear now

Since Kodi is playing the file, the destination fps should be known and the user could enter just the sub's original fps, no?

I thought the same VP provides the fps information so it would also simplify the new setting
for what concern fmt::formatter template to me its not clear for what uses its needed

[CrystalP]

Since Kodi is playing the file, the destination fps should be known and the user could enter just the sub's original fps, no?

I thought the same VP provides the fps information so it would also simplify the new setting

bonus points for retrieving the sub fps from the sub provider to make the feature automatic :-)

On second thought, variable refresh rate would be a problem, but maybe it's not relevant for the materials the feature is intended for. I remember a recent TV series with intro/outro in 30fps and main content in 24fps. No idea if it's widespread.

[jmbreuer]

Thank you for all the valuable feedback!

The main points I'm taking from this are:

• UI can be simplified to choose only the (assumed) frame rate of the subtitle file
  • getting that automatically from the subtitle provider would be great, however I'm almost certain that this kind of metadata is typically not available, much less in a standardized way. (This means: If anyone knows where/how to somewhat reliably get that metadata, please let me know!)
    I'd still like to keep the feature in a way that allows the user to override wrong metadata in that case, i.e. only initialize from there.
• rework logic to use actual media frame rate as the target for conversion
  • not for now: how to deal sensibly with non-constant-fps media then?
    I'd wager that (necessary) frame rate correction of subtitles for non-constant-fps material is not something that occurs "in the wild".
• making and keeping translated strings minimal

I'll rework the code towards these criteria "soon".

[notspiff]

If media_length > threshold guess_based_on_last_frameno() where you'd guess based on which is closest to media length would probably work well as a heuristic for the default value in that list but maybe a bit too involved.

[jmbreuer]

I've updated the code to allow selection of subtitle fps, and determine the "stretch factor" taking video fps into consideration from that. Feedback/comments welcome.

@notspiff I don't (yet) see how guessing subtitle fps could realistically work. Subtitle files (as I've seen them) do not specify the total (assumed) length of the media file - in that case, it would be easy. The only information available is the timestamp of the last subtitled line of dialog, which in the case of a 2h movie would typically occur before end credits, which might run 3-8 minutes (or not). Those 5 minutes of uncertainty are on the same order as the 4% 24/25fps difference caused by PAL telecine.

[notspiff]

You pretty much summarized it. Hence my suggestion was to only use it as a heuristic for selecting the default option in the most definitely needed list, as it cannot be entirely reliable as an automation. But it will at least get it right often-ish.

[CrystalP]

Thanks for your contribution @jmbreuer !

The following prevents Jenkins from building successfully on a few platforms:
/Users/Shared/jenkins/workspace/OSX-64/xbmc/cores/VideoPlayer/VideoPlayer.cpp:3365:19: error: implicit conversion increases floating-point precision: 'float' to 'double' [-Werror,-Wdouble-promotion]
15:38:02 double dValue = 0.0f;

no f needed for double type.

[jmbreuer]

The following prevents Jenkins from building successfully on a few platforms: /Users/Shared/jenkins/workspace/OSX-64/xbmc/cores/VideoPlayer/VideoPlayer.cpp:3365:19: error: implicit conversion increases floating-point precision: 'float' to 'double' [-Werror,-Wdouble-promotion] 15:38:02 double dValue = 0.0f;
no f needed for double type.

Ah, thanks! Refactoring/cherry-pick/merge f-up. Pun half-intended.

You pretty much summarized it. Hence my suggestion was to only use it as a heuristic for selecting the default option in the most definitely needed list, as it cannot be entirely reliable as an automation. But it will at least get it right often-ish.

I'll have a look through stuff I happen to be watching for the timestamps of subtitles, media length (and whether they need fps correction or not). Maybe there is a useful pattern in there. With my current lack of knowledge, implementing any kind of heuristic would, I feel, mostly have potential to make things worse - requiring users to know what to do to re-fix their subtitles which got erroneously un-fixed.

I do feel such heuristic auto-detection might/could/should be a separate PR; not least for clarity's sake.

[jmbreuer]

... sorry for the noise, now the patch finally looks like I'd want it to look.

I've not picked the remaining Jenkins code formatting suggestion, since it would break with the formatting convention of the block of code this one line is added to. If that is what's rather preferred, though, just let me know.

I'm confused by the somewhat spurious Jenkins build errors for OSX-ARM64:

14:38:25 [100%] Built target dmg
14:38:25 ++ cd /Users/Shared/jenkins/workspace/OSX-ARM64/build/tools/darwin/packaging/osx/
14:38:25 +++ getBuildRevDateStr
14:38:25 +++ echo 20230530-8f8ee295-PR23206-516
14:38:25 ++ UPLOAD_FILENAME=kodi-20230530-8f8ee295-PR23206-516-arm64.dmg
14:38:25 ++ mv org.xbmc.kodi-osx_21.0-0~alpha1_macosx-arm64.dmg /Users/Shared/jenkins/workspace/OSX-ARM64/tools/darwin/packaging/osx/kodi-20230530-8f8ee295-PR23206-516-arm64.dmg
14:38:26 [OSX-ARM64] $ /bin/sh -xe /var/folders/jm/3ycqgkps58gc4sbwpmfcpd_h0000gp/T/jenkins2157953217262311695.sh
14:38:26 + '[' true == true ']'
14:38:26 + . /Users/Shared/jenkins/workspace/OSX-ARM64/tools/buildsteps/osx-arm64/run-tests
14:38:26 /var/folders/jm/3ycqgkps58gc4sbwpmfcpd_h0000gp/T/jenkins2157953217262311695.sh: line 4: /Users/Shared/jenkins/workspace/OSX-ARM64/tools/buildsteps/osx-arm64/run-tests: No such file or directory
14:38:27 Build step 'Execute shell' marked build as failure

To me, this looks like there's something wrong with the way the build is set up. If this is breaking due to something in this PR, please point me in the right direction - I couldn't find it in the build console log.

[jmbreuer]

Looking through various build logs, it seems that running tests on OSX-ARM64 was enabled / started to be a new thing about 24 hours ago, and was worked on - causing intermittent failures - (at least) until about 8 hours ago, when those builds started to succeed again, now including tests.

Can I ask someone with Jenkins access to rebuild this, please?

[jmbreuer]

Again, the only test failure for WIN-64 marking this as failing looks completely unrelated to this PR to me...

[CrystalP]

You could rebase on current master. The commits can be squashed unless they help understand your changes.

[CrystalP]

@CastagnaIT, as Mr. Subtitles, do you have thoughts about the feature and what's been said so far, before going into the details of the implementation?

[CastagnaIT]

currently i my little time is on ISAdaptive so im not so active on xbmc repository
i have not tested it, however from what i see there are some my considerations:

• The new setting, imo, should have "Disabled" default value set where the code do no-operations
• Improving ESUBTITLEFPS enum, it must be enum class without name prefixes, e.g. SUBTITLE_FPS and values as SAME, 24 etc...
• From a novice user eye, imo the setting could be not so clear, so the setting title is Subtitle frame rate correction, ok, but could be not so clear that the setting refers to the frame rate at which the subtitle was created, but since the current GUI window seem not allow add a setting description, maybe we can improve the setting title in some way, e.g. Subtitle frame rate correction, source FPS ?
• The struct fmt::formatter<ESUBTITLEFPS> code can be full deleted, it makes no sense to complicate things for simple strings
• The ESUBTITLEFPS enum should have an appropriate translation string in .po file for each value, and not hardcoded as done now

for variable framerate cases: we can ignore it has no sense with this setting, maybe in an ideal world in case of variable frame rate, this GUI setting should be disabled/greyed out, but from what i remember we have no easy way to know if we have a variable framerate content

Edit forgot it:
ST_FPS_24 = 23976
or you are talking about 24fps or 23~, this is confusing

[jmbreuer]

I've been using this for a while now, and out of that experience I decided to simplify the feature, especially with regards to UX / necessary string translations etc.

The only relevant cases that I ever encountered are: Video at "24 fps" (which in practice means 23.976, I've never encountered true 24.0 in that time) and subtitles mastered for that evil sped up 25 fps, OR A/V at 25 fps and subtitle files for the original 23.976 material.

Which means that, from a user's perspective, there can be a simple on/off switch: "Compensate for wrong subtitle 'speed'/fps".

This is what I've done with my current commit on top (which I'm happy to squash into the original one, if desired).

This should also obviate most of the points by @CastagnaIT above:

• The new setting, imo, should have "Disabled" default value set where the code do no-operations

Yes, that's exactly how it behaves.

• From a novice user eye, imo the setting could be not so clear, so the setting title is Subtitle frame rate correction, ok, but could be not so clear that the setting refers to the frame rate at which the subtitle was created, but since the current GUI window seem not allow add a setting description, maybe we can improve the setting title in some way, e.g. Subtitle frame rate correction, source FPS ?

Edit forgot it: ST_FPS_24 = 23976 or you are talking about 24fps or 23~, this is confusing

I've chosen to represent 23.976 to the user as 24fps for simplicity. The only place where that still matters is the option label string in strings.po and easily changed to whatever wording is deemed most appropriate. Currently, it reads "Fix 24/25 fps mismatch" (within the subtitles dialog), is a simple on/off option, defaults to off.

Everything else, i.e. concerning the SUBTITLEFPS enum, is no longer necessary and gone now.

I hope this brings us a bit closer to something that would be generally useful.

As an additional point of data, since the start of this year I've provided two friends with a local build of this feature when they told me how they were also bitten by the issue of mismatching subtitles.

[jenkins4kodi]

I've made some formatting changes to meet the current code style. The diffs are available in the following links:

• PR23206-0001-9b05b380cd-Implement-subtitle-frame-rate-corr.diff
• PR23206-0002-929a307eb7-Make-SubtitleFPS-an-enum-class-min.diff
• PR23206-0003-f4f049e9e7-Simplify-esp-usage-for-the-only-pr.diff
• PR23206.diff

For more information please see our current code style guidelines.