Fix: Previous Subtitle Navigation with Multiple Tracks #578

[JSchoreels]

Fix: Previous Subtitle Navigation with Multiple Tracks

When two subtitle tracks are enabled, pressing "Seek to previous subtitle" would stay on the current subtitle instead of jumping to the actual previous one.

The bug was in the adjacentSubtitle function at line 23 of common/key-binder/key-binder.ts. The original code was:

adjacentSubtitleIndex = now < s.end ? Math.max(0, i - 1) : i;

This assumed that i - 1 would give the previous subtitle. But when multiple tracks are enabled, the subtitles array contains all tracks interleaved and sorted by time. So i - 1 might point to a different track's subtitle.

For example, at timestamp 631787ms with these subtitles:

[323] Track 1: 629295-632129ms  "Of course, it is."
[324] Track 0: 629296-632130ms  "もちろんそうです"
[325] Track 1: 631130-635343ms  "Monsters inside the dungeon" (current)
[326] Track 0: 631131-635344ms  "ダンジョンの中のモンスター"

The algorithm would loop through all subtitles looking for the minimum time difference. It would correctly find subtitle 323 as the previous one for Track 1. But then it would continue looping and check subtitle 326 (Track 0), which has an even smaller diff. From there, searching backward for Track 1's previous subtitle would incorrectly find 325 - the current subtitle.

The fix identifies which track is currently showing, then when seeking backward, explicitly searches for the previous subtitle of that same track and breaks out of the loop to avoid it being overwritten.

[JSchoreels]

I changed a bit the implementation to search the current subtitles by binary search since the subs list is a sorted list

[killergerbah]

Thanks.. this has been broken or a while

[JSchoreels]

FYI I changed a bit the logic, I realized that sometimes not all subtitles are "paired" so going for the previous one for the same track might jump over other subtitles.

For examples, the japanese subs might have some entries to describe what is happening while the english only appears when there's something said, or if something japanese is written in the current scene.

So I changed the implementation to just look backward from the current index, but consider to jump only for subtitle with a bigger difference than a certain delta. I set the delta to 100ms and it seems to work fine in the examples I tried.

I also created another util function findAllCurrentSubtitles that allow to find all sub ID overlapping a certain timestamp. It's not used in the code itself because the main algo doesn't really need it (and I personally thought it would complexify the main logic).

There's a lot of edge cases that could happen especially with overlapping subtitles (if for example a sub is starting from the start of the video and ends at the end), but I guess there's no perfect implementation for it (and please keep in mind right now the current one is just not working at all for users with multiples tracks)

[killergerbah]

Hey sorry for the late response. I've been really busy and all my asbplayer time was spent reading the big in-progress coloring PR. I also appreciate you writing a lot of tests.

After reading the diff more closely I'm not completely convinced that it's a significant improvement over the previous logic. Let me know if I'm off about anything.

[killergerbah]

Now that I'm reading closely I'm a little worried about this new algorithm. Binary search here doesn't consider the diff with the current timestamp at all, it just returns for the first intersecting subtitle it finds, even if there's a "closer" overlapping subtitle.

[JSchoreels]

Question is how to chose the "closer", how to define it.

For example let's say you have 3 entries : A, B, C. Time timeline goes :

AAAAAAAAA------
--------B------
--------CCCCCCC

You ask to find the subtitles at B time. You get A, B, C.

How would you define the algo to find the best match ? Start time ? End time ? Duration ?

In any case, please remember that this issue right now means that for people not running my fork, they can't even seek backward caption if they have more than 1 sub track, so I don't really know if those edge case justify the bug to continue to live on.

[killergerbah]

Right, I agree we should define what "best match" means.

In any case, please remember that this issue right now means that for people not running my fork, they can't even seek backward caption if they have more than 1 sub track, so I don't really know if those edge case justify the bug to continue to live on.

I think you might have made your changes based on an incorrect interpretation of the existing code. The existing code allows you to seek through subtitles in-order, regardless of track.

When I first saw this PR, I thought that existing behavior might actually be undesirable, since it could be confusing for users using 2nd and 3rd tracks that are mis-aligned with the 1st for whatever reason. However, what you're describing looks to be a completely different issue.

[JSchoreels]

Hmmm maybe you misunderstood the issue reported initially but seeking back the previous subtile when two subs are enabled means it just keep on repeating the same section. Typically if the two tracks are two different languages with similar timestamp.

So basically except if you double press seek back fast enough it won't go to the previous ones, it will keep on repeating the same section

By previous implementation you mean the existing one in master or the previous one of this PR ?

[killergerbah]

Hmmm maybe you misunderstood the issue reported initially but seeking back the previous subtile when two subs are enabled means it just keep on repeating the same section. Typically if the two tracks are two different languages with similar timestamp.

I did investigate this issue a little bit and the subtitles submitted don't have this problem. I suspect that the issue has more to do with seeking precision in some videos due to only being able to seek to certain frames.

By previous implementation you mean the existing one in master or the previous one of this PR ?

The one in master. It doesn't constrain the track, hence why I was observing OK behavior above.

[JSchoreels]

If I take for example this test :

        const subtitles = [
            subtitle('Track 1 - First', 1000, 2000, 0),
            subtitle('Track 2 - First', 1001, 2001, 1),
            subtitle('Track 1 - Second', 3000, 4000, 0),
            subtitle('Track 2 - Second', 3001, 4001, 1),
        ];

With master branch logic, pressing "Seek Previous Subtitles" at "Track 2 - Second" will set the playback at "Track 1 - Second", and not at one of the 2 "First".

Since the difference might be a millisecond, you'd have to press twice the button under 1ms to jump from "Track 1 - Second" to "Track 2 - First". Most people will have the impression of being stucked at "Second" subtitles.

That's why restricting helped initially. But then I realized not all subs are duplicated across all languages, thus why I just took one outside a minimum distance threshold

[killergerbah]

I'd be more comfortable with the algorithm if you used this function instead. At least it will consider all overlapping subtitles instead of picking the first one it finds.

[killergerbah]

Assuming that currentIndex is the correct one, is the only difference with the previous algorithm that we are now considering subtitles that are more than threshold away?

[JSchoreels]

The main difference is that for the previous algorith, you could jump across other subtitles if they were from other tracks. For example let's say you are in a section of the anime where there's only a English (Track 2) subs about some kind of sign being shown. If you pressed previous, it would jump to the previous Track 2 subs which might be at the start of the anime which means you skip all the japanese (Track 1) ones.

With this one, won't happen

[killergerbah]

For example let's say you are in a section of the anime where there's only a English (Track 2) subs about some kind of sign being shown. If you pressed previous, it would jump to the previous Track 2 subs which might be at the start of the anime which means you skip all the japanese (Track 1) ones.

Are you sure? The previous code just checks for positive min distance to subtitle in a certain direction, regardless of track. I don't see any logic that would constrain the chosen adjacent subtitle chosen to the same track.

[JSchoreels]

Here in the first commit it was how I initially did it but then I realized in some case it was jumping to far back because the matched sub had less timeslot entries than the others (for examples signs etc)

b4717d8