Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Don’t truncate parsed times >=10h #104

Closed
wants to merge 1 commit into from

Conversation

TheOneric
Copy link

@TheOneric TheOneric commented Jan 6, 2024

Such times are accepted by all modern renderers and with ffmpeg and mkvtoolnix it appears muxers support it as well.

Testing the new code, everything still seems to be fine:

    //  11:42:42.421 =  42162421 ms
    agi::Time time = agi::Time(42162421);
    std::cout << " 11:42:42.421 ASS = " << time.GetAssFormatted() << std::endl;
    std::cout << " 11:42:42.421 ASS = " << time.GetAssFormatted(true) << std::endl;
    std::cout << " 11:42:42.421 SRT = " << time.GetSrtFormatted() << std::endl;
    std::cout << "Parsed from string: " << agi::Time("11:42:42.421").GetSrtFormatted() << std::endl;
    // 111:00:00.001 = 399600001 ms
    time = agi::Time(399600001);
    std::cout << "111:00:00.001 ASS = " << time.GetAssFormatted() << std::endl;
    std::cout << "111:00:00.001 SRT = " << time.GetSrtFormatted() << std::endl;
    std::cout << "Parsed from string: " << agi::Time("111:00:00.001").GetSrtFormatted() << std::endl;
    //   1:42:42:120 =   6162120 ms
    time = agi::Time(6162120);
    std::cout << "  1:42:42.120 ASS = " << time.GetAssFormatted() << std::endl;
    std::cout << "  1:42:42.120 SRT = " << time.GetSrtFormatted() << std::endl;
    std::cout << "Parsed from string: " << agi::Time("01:42:42.120").GetSrtFormatted() << std::endl;
 11:42:42.421 ASS = 11:42:42.42
 11:42:42.421 ASS = 11:42:42.421
 11:42:42.421 SRT = 11:42:42,421
Parsed from string: 11:42:42,421
111:00:00.001 ASS = 111:00:00.00
111:00:00.001 SRT = 111:00:00,001
Parsed from string: 111:00:00,001
  1:42:42.120 ASS = 1:42:42.12
  1:42:42.120 SRT = 01:42:42,120
Parsed from string: 01:42:42,120

time_test

I tried to also deal with the input boxes, by allowing prepending a digit if the cursor is at the start and a modifier is pressed, but it appears those keyboard events don’t even reach TimeEdit::OnChar and I’m not sure how to resolve this (even with Shift as a modifier).
If there already are multi-digit hours, all digit can be edited as usual (unless the leading digit is replaced by a zero in which case it disappears).

As a hacky workaround until a proper fix: I also noticed if dead keys or an IME is involved OnChar also appears to be skipped. This can be abused to insert any character at the start and then subsequently replace it as usual with the desired digit.

(Side note: when this limit started to be enforced in 1d4c0c0, there already was wxString::format available and used for SMPTE, so this was no reason to avoid multi-digits (anymore already))

All current renderers and muxers have no issues with longer timestamps.
However, classic VSFilter keeping track of time with a 32-bit int,
leads to a more meaningful limit at 596h, so enforce that instead.

Since the stringification code for SRT and ASS (but not SMPTE) relied on
hours being single digit, replace them with a simple format string.

GUI inputs are still limited to <10h, but at least
pre-existing timestamps no longer get mangled.
@TheOneric
Copy link
Author

TheOneric commented Jan 7, 2024

Updated patch to not just lift the one-digit limitation, but to also instead enforce the < 596h limit stemming from 32-bit timestamps.

The new CI failure for MacOS and Windows seems unrelated:

Run-time dependency Boost found: NO (tried system)
Downloading boost source from https://boostorg.jfrog.io/artifactory/main/release/1.74.0/source/boost_1_74_0.tar.gz
A fallback URL could be specified using source_fallback_url key in the wrap file

meson.build:114:12: ERROR: Incorrect hash for source:
 afff36d392885120bcac079148c177d1f6f7730ec3d47233aa51b0afa4db94a5 expected
 5e89103d9b70bba5c91a794126b169cb67654be2051f90cf7c22ba6893ede0ff actual.

@arch1t3cht
Copy link
Owner

I could finally sit down to test this now - merged in 8fcb7c6, thanks a lot!

And, yeah, personally I'm not too worried about it not being possible to enter times >10h in the input boxes (certainly not enough to spend time on trying to make it work). In the very unlikely case that users do need it, they can enter frame numbers instead or use an automation script.

@arch1t3cht arch1t3cht closed this Feb 8, 2024
@arch1t3cht
Copy link
Owner

... aaand just as I merged this, I see a small issue: The widths of the grid columns for the start and end times don't account for times > 10h, so the times don't fit in completely.
But I can just fix that myself.

@TheOneric
Copy link
Author

Sorry, I missed that

Btw, if you’re worried about recalculating the column width each update, we could just always reserve enough space for MAX_TIME from libaegisub/ass/time.cpp. This will admittedly waste 4 characters for most (sub-10h) files, but relative to the total available space this doesn’t seem like a lot to me

@arch1t3cht
Copy link
Owner

Yeah, that was my backup solution if this turns out to cause bigger problems. But my thinking was that it shouldn't contribute too much, since there are already other O(#events) functions run per commit, like uploading the new file to the subtitle renderer.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants