Skip to content

[IMPROVEMENT] Add missing braces in processmp4 frame-type check - #2362

Merged
cfsmp3 merged 1 commit into
masterfrom
fix/mp4-subtitle-iframe-braces
Oct 2, 2026
Merged

cfsmp3 merged 1 commit into
masterfrom
fix/mp4-subtitle-iframe-braces

Conversation

@cfsmp3

@cfsmp3 cfsmp3 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

In raising this pull request, I confirm the following (please check boxes):

Reason for this PR:

  • This PR adds new functionality.
  • This PR fixes a bug that I have personally experienced or that a real user has reported and for which a sample exists.
  • This PR is porting code from C to Rust.

Sanity check:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • If the PR adds new functionality, I've added it to the changelog. If it's just a bug fix, I have NOT added it to the changelog.
  • I am NOT adding new C code unless it's to fix an existing, reproducible bug.

Repro instructions:

None — style only, no behaviour change.


Follow-up to #2321. The if it extended in processmp4() still has a braceless body; project style requires braces on every if/else/for/while body.

Builds clean; output on a tx3g subtitle-only MP4 is byte-identical to #2321.

The if body extended in #2321 still has no braces, which the project
style requires on every if/else/for/while body. No behaviour change.
@cfsmp3

cfsmp3 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

@canihavesomecoffee — request for the regression suite, related to #2321 (merged as e313f70).

Sample 162 (Yomeddine_1080p_HD, m4v with a tx3g subtitle track) is on the platform but has no regression test, which is how the bug #2321 fixed went unnoticed: before it, every one of the track's 797 cues came out as 00:00:00,000 --> 4222190269:4294967266:4294967249,4294967295.

Could you add a test for it — plain --out=srt is enough? The tx3g track is written to the second output file (<name>_2.srt). The expected output should come from master at or after e313f70; I checked that output against ffmpeg's own extraction of the track and all 797 cues agree within 0–2 ms.

@ccextractor-bot

Copy link
Copy Markdown
Collaborator
CCExtractor CI platform finished running the test files on linux. 171/237 tests matched the approved output:
Report Name Tests Passed
Broken 9/13
CEA-708 2/14
DVB 0/7
DVD 3/3
DVR-MS 2/2
General 25/27
Hardsubx 1/1
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 69/86
Teletext 0/21
WTV 12/13
XDS 32/34

66 tests do not match the approved output. That is the pass/fail verdict. Whether this branch caused it is a separate question, answered below.


Compared with the tip of master — test 9596, commit e313f70:

  • 0 pass there and fail here
  • 76 fail there and pass here
  • 29 fail on both, with different output
  • 37 fail on both, byte for byte the same

Fail on both but produce different output — behaviour moved even though the verdict did not:

Fail there, pass here:


Compared with the commit this branch was cut from: the same run as the tip of master (test 9596), so the comparison above already covers it.


This branch changes the behaviour of 29 test(s) relative to the tip of master. Those are the ones worth looking at; anything else in the list fails the same way on both sides.

@ccextractor-bot

Copy link
Copy Markdown
Collaborator
CCExtractor CI platform finished running the test files on windows. 154/237 tests matched the approved output:
Report Name Tests Passed
Broken 9/13
CEA-708 2/14
DVB 0/7
DVD 3/3
DVR-MS 2/2
General 24/27
Hardsubx 0/1
Hauppage 3/3
MP4 2/3
NoCC 10/10
Options 69/86
Teletext 0/21
WTV 3/13
XDS 27/34

83 tests do not match the approved output. That is the pass/fail verdict. Whether this branch caused it is a separate question, answered below.


Compared with the tip of master — test 9589, commit 670a5ab:

  • 0 pass there and fail here
  • 0 fail there and pass here
  • 0 fail on both, with different output
  • 83 fail on both, byte for byte the same

Compared with the commit this branch was cut from: the same run as the tip of master (test 9589), so the comparison above already covers it.


No test changes behaviour relative to the tip of master: every failure above fails there too, byte for byte. The approved output for those tests is out of date, which is a baseline to review rather than a regression in this branch.

@cfsmp3
cfsmp3 merged commit 6e9c8cb into master Oct 2, 2026
46 of 48 checks passed
@cfsmp3
cfsmp3 deleted the fix/mp4-subtitle-iframe-braces branch October 2, 2026 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants