[rjksmith]

Refined regex for VMT timestamp

• Exactly 3 digits for ms
• At least 2 digits for hours

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-13T17:42:41.476596Z	533747f	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 533747f1c2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Return BAD for newly rejected fractional timestamps

When a cue uses a fractional field of any other length (for example 00:01.12), pattern_cue still accepts it, but this stricter regex no longer matches it. parse_vmt_timestamp therefore returns 0 from its no-match path instead of reaching the existing length check and returning BAD_VMT_TIMESTAMP as before; the encoder can then merge the malformed cue into the time-zero packet rather than skipping it with the bad-timestamp diagnostic. Preserve the invalid-timestamp sentinel for these newly rejected forms.

Useful? React with 👍 / 👎.

[farindk]

ChatGPT clearly didn't read my comment before providing its feedback ;-)

I think its point was different. I asked it for a more detailed review.

---

The tightened pattern matches the WebVMT timestamp syntax (hours two or more digits, minutes and seconds exactly two, thousandths exactly three).

One follow-up is needed in parse_vmt_timestamp(), though. Timestamps that the new pattern rejects no longer reach the BAD_VMT_TIMESTAMP path. They now take the no-match branch, which returns 0:

input	before	after
00:01.12	BAD_VMT_TIMESTAMP, reported and skipped	0
00:01.1234	BAD_VMT_TIMESTAMP, reported and skipped	0
1:02:03.000	3723000	0
:02:03.000	123000	0

A return value of 0 is not harmless in the cue loop. If no positive-time cue has been seen yet, the malformed cue's data is silently merged into the time-zero packet. Otherwise it fails the ordering check and is reported as "Bad WebVMT timestamp order", which points at the wrong defect. Before this change, 00:01.12 was caught by the fs.length() == 3 check and reported as a bad timestamp. With the new pattern that check can never fire.

Suggested fix: return the sentinel on no-match and drop the now-dead length check.

   if (!std::regex_match(vmt_time, match, pattern)) {
-    return 0; // no match
+    return BAD_VMT_TIMESTAMP;
   }
@@
-  if (fs != "") {
-    if (fs.length() == 3) {
-      ms = std::stoi(fs);
-    }
-    else {
-      return BAD_VMT_TIMESTAMP; // invalid
-    }
-  }
+  if (fs != "") {
+    ms = std::stoi(fs); // the pattern guarantees exactly three digits
+  }

The only caller is the cue loop, and its cue regex only passes strings of digits, colons, dots and a leading minus, so nothing depends on 0 for unparseable input. An empty start time (a line like --> 00:01.000) would then also be reported instead of landing in packet zero.

Two minor notes on the pattern itself, no change required:

• The fraction is still optional. The spec's "collect a WebVMT timestamp" algorithm returns an error when the full stop is missing, so 01:02 is not a valid WebVMT timestamp. Keeping it optional is fine as tolerance, but the PR description's "exactly 3 digits for ms" overstates the strictness a bit.
• Single-digit hours such as 1:02:03.000 are accepted by the spec's parsing algorithm (any first field that is not exactly two digits is treated as hours), but the authoring syntax requires two or more digits. Rejecting them is defensible, as long as it is reported, which the fix above provides.

[rjksmith]

Suggested fix: return the sentinel on no-match and drop the now-dead length check.

Yes. I concur with both changes. The former change addresses an existing problem that was masked by the relaxed syntax check.

The only caller is the cue loop, and its cue regex only passes strings of digits, colons, dots and a leading minus, so nothing depends on 0 for unparseable input. An empty start time (a line like --> 00:01.000) would then also be reported instead of landing in packet zero.

Omitting cue start time is an error, so that new behaviour is correct. Omitting cue end time is permitted for unbounded cues, but is already handled correctly elsewhere.

Two minor notes on the pattern itself, no change required:

* The fraction is still optional. The spec's "collect a WebVMT timestamp" algorithm returns an error when the full stop is missing, so `01:02` is not a valid WebVMT timestamp. Keeping it optional is fine as tolerance, but the PR description's "exactly 3 digits for ms" overstates the strictness a bit.

* Single-digit hours such as `1:02:03.000` are accepted by the spec's parsing algorithm (any first field that is not exactly two digits is treated as hours), but the authoring syntax requires two or more digits. Rejecting them is defensible, as long as it is reported, which the fix above provides.

I agree. I'd spotted the former, though not the latter.

• Milliseconds are mandatory which is important for accurate video frame synchronisation.
• Hours can be a single digit which is compliant with WebVMT and WebVTT syntax.

I've updated the regex to reflect this and trialled that revised syntax in the vmttool tests which continue to pass. I think this is an improvement that reflects the specification more accurately. Many thanks for your help.

[farindk]

Thanks Rob, merged. I squashed your two commits into 4d2daff (author preserved) and added a few commits on top, so GitHub shows this PR as closed rather than merged.

Follow-ups on examples/vmt.cc:

• 7a35ca8 – the fix to the issue reported above: timestamps rejected by the pattern return the sentinel and are reported instead of silently becoming 0.
• 7d81c9e – hour values that overflow the 32-bit millisecond counter are rejected (hours above 596 overflowed, very long hour fields aborted via std::stoi).
• 81365ac – BAD_VMT_TIMESTAMP is now UINT32_MAX.
• c307a77 – minutes and seconds above 59 are rejected, as in the spec.
• 906e59b – an almost complete rewrite of the parser, see below.

About 906e59b: while reviewing the rest of the file I found that std::regex was the root of several problems, so I replaced it with hand-written parsing.

• The libstdc++ regex matcher recurses once per input character for patterns like [\s\S]*? or (\d|:|\.)*. One sync data value, one line of digits, or one line of whitespace of roughly 30,000 characters overflowed the default 8 MB stack and crashed heif-enc with SIGSEGV. A 20 KB base64 blob in a sync command, which the wiki documents, was enough to trigger it.
• . does not match a carriage return, so in CRLF files every bounded cue failed the cue pattern and was silently dropped, while unbounded cues happened to survive.
• JSON escapes in sync data were not handled: "say \"hi\"" was stored as say \.
• A missing file, or a file without the WEBVMT signature, produced an empty metadata track with exit code 0.

The new code reads the file whole and accepts CR, LF and CRLF plus a BOM, treats any line containing --> as the cue timing line, parses the timestamp by hand with the rules from this PR, and reads the payload with a minimal JSON reader (string escapes including \uXXXX surrogates, nesting depth capped). Hex and base64 decoding now report bad characters, odd digit counts, misplaced padding and partial groups. Messages carry the line number of the cue.

Behaviour changes you may notice in your vmttool tests:

• A payload that is not a list of JSON objects, or a sync command without a type or a string data, is reported and the cue is skipped. The regex version silently wrote an empty packet. A cue with JSON but no sync command still writes an empty packet, now with a warning.
• A missing file or missing signature makes heif-enc exit with code 5. Per-cue problems remain warnings with exit code 0, as before.
• NOTE must be followed by whitespace or the end of the line.

The documented rules are unchanged: end times ignored, same start time merged, negative timestamps are zero. It would be great if you could run your vmttool tests against master and tell me if anything regressed.

[rjksmith]

That's a great improvement. Many thanks for your support and feedback.