sub/sd_ass: subtitle timing rounding error - #18039
Conversation
|
@arch1t3cht: Could you do the same in aegi? |
|
Please squash the commits. I also think that a test for single math operation is a bit overkill. |
|
Of course. I'm removing the test file before squashing then |
ea85a60 to
05a9810
Compare
Yeah, I will. A hardcoded |
|
Why not just use |
Having bigger epsilon is safer. If the |
Sure. Are you also referring to |
Yes, and 2 more. Search for |
Use floor(pts * 1000 + 1e-6) instead of llrint() for all timestamp conversions to avoid floating-point precision errors causing early subtitle presentation on boundry frames. Fixes: mpv-player#17453 Signed-off-by: Ricardo Fonseca <ricardofonseca94321@tecnico.ulisboa.pt>
|
Done. Let me know if anything else needs changing. |
Coordinated with mpv-player/mpv#18039 The "integer millisecond count -> double -> / 1000 -> * 1000 -> floor -> int" round trip is not always lossless, causing subtitle lines to not appear when they should in rare edge cases. For example, floor(518810 / 1000 * 1000) is 518809, causing a subtitle line with start time 0:17:07.36 to not show up on its first frame when that frame's timestamp is also exactly 0:17:07.36 (which matches frame 24632 (0-indexed) of a 24000/1001 video). The proper solution here might be to get rid of the floating point format for timestamps altogether and use integer millisecond values or fractions, but that is a larger refactor. For now, adding a small epsilon works as a workaround and matches mpv. Fixes arch1t3cht#205.
Coordinated with mpv-player/mpv#18039 The "integer millisecond count -> double -> / 1000 -> * 1000 -> floor -> int" round trip is not always lossless, causing subtitle lines to not appear when they should in rare edge cases. For example, floor(518810 / 1000 * 1000) is 518809, causing a subtitle line with start time 0:17:07.36 to not show up on its first frame when that frame's timestamp is also exactly 0:17:07.36 (which matches frame 24632 (0-indexed) of a 24000/1001 video). The proper solution here might be to get rid of the floating point format for timestamps altogether and use integer millisecond values or fractions, but that is a larger refactor. For now, adding a small epsilon works as a workaround and matches mpv. Fixes arch1t3cht#205.
Coordinated with mpv-player/mpv#18039 The "integer millisecond count -> double -> / 1000 -> * 1000 -> floor -> int" round trip is not always lossless, causing subtitle lines to not appear when they should in rare edge cases. For example, floor(518810 / 1000 * 1000) is 518809, causing a subtitle line with start time 0:17:07.36 to not show up on its first frame when that frame's timestamp is also exactly 0:17:07.36 (which matches frame 24632 (0-indexed) of a 24000/1001 video). The proper solution here might be to get rid of the floating point format for timestamps altogether and use integer millisecond values or fractions, but that is a larger refactor. For now, adding a small epsilon works as a workaround and matches mpv. Fixes arch1t3cht#205.
5 participants
Use nearbyint() when the value is extremely close to an integer and floor() otherwise to prevent forward rounding caused by floating-point precision errors. This avoids early subtitle presentation on boundary frames.
Add test for ASS subtitle timing to ensure correct rendering around boundary timestamps.
Fixes: #17453
Read this before you submit this pull request:
https://github.com/mpv-player/mpv/blob/master/DOCS/contribute.md
Reading this link and following the rules will get your pull request reviewed
and merged faster. Nobody wants lazy pull requests.