fix(text): drop seconds past one hour and stop leaking mkv AC-3 extras - #36
Conversation
Two default-text gaps found by testing 14 new mkv samples that the existing parity set cannot reach, because every old sample is shorter than one hour and none is FLAC, DD+ 7.1, or 2160p Dolby Vision. formatDuration printed three units once a duration reached an hour. Official prints only the two most significant units, truncated. This is confirmed against 190 paired raw-to-text values from official v23.04. Matroska E-AC-3 tracks also copied raw mixing and dialnorm metadata into the friendly text output. Official keeps those under JSON extra only. Commit fba9661 removed the ltrt and loro pair; this removes the rest. Verified against official v23.04 on 32 real files: 14 better, 18 same, 0 worse. The default-text diff over the mkv, mp4, and TS set fell from 611 lines to 193, and three samples reached 0. JSON and --language=raw output stay byte-identical to the previous commit on every file, including the BDAV and DVD controls.
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change updates hour-based duration formatting and removes legacy friendly-text projection for Matroska E-AC-3 metadata. Raw JSON retains dialog normalization and mixing metadata. Tests cover duration boundaries and E-AC-3 output separation. ChangesMedia output updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/mediainfo/matroska_eac3_probe_test.go (2)
259-260: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete raw
extrapayload.This test checks only a short substring, so it can pass if those fields appear in the wrong order or among extra keys. Replace the
strings.Contains(extra,"dialnorm":"-31","compr":"-0.28","acmod":"7","lfeon":"1","dmixmod":"Lo/Ro")assertion with an exact match or unmarshal-and-assert check. Then assertJSONRaw["extra"]separately for the JOC fields before or after the existingComplexityIndexcheck.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mediainfo/matroska_eac3_probe_test.go` around lines 259 - 260, Update the test assertion around JSONRaw["extra"] to validate the complete raw extra payload rather than checking a partial substring; use an exact match or unmarshal-and-assert approach that rejects reordered or unexpected keys. Separately assert the JOC fields in JSONRaw["extra"] before or after the existing ComplexityIndex check, preserving the expected E-AC-3 mixing metadata.Source: Coding guidelines
237-246: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the E-AC-3 default text output.
The current assertion only checks canonical
stream.Fields.projectRawTextReportthen projects raw text throughrawTextLabel("dialnorm")when extra entries are visible, so this test can pass whileDialog Normalizationstill appears in raw text. Add a minimal raw-text assertion for the EAC3JOC text metadata test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mediainfo/matroska_eac3_probe_test.go` around lines 237 - 246, The E-AC-3 metadata test only validates canonical stream fields, not projected raw text. In the EAC3JOC text metadata test, add a minimal assertion against the output of projectRawTextReport to confirm Dialog Normalization and related dialnorm metadata are omitted from the default text projection while retaining the existing stream.Fields checks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/mediainfo/matroska_eac3_probe_test.go`:
- Around line 259-260: Update the test assertion around JSONRaw["extra"] to
validate the complete raw extra payload rather than checking a partial
substring; use an exact match or unmarshal-and-assert approach that rejects
reordered or unexpected keys. Separately assert the JOC fields in
JSONRaw["extra"] before or after the existing ComplexityIndex check, preserving
the expected E-AC-3 mixing metadata.
- Around line 237-246: The E-AC-3 metadata test only validates canonical stream
fields, not projected raw text. In the EAC3JOC text metadata test, add a minimal
assertion against the output of projectRawTextReport to confirm Dialog
Normalization and related dialnorm metadata are omitted from the default text
projection while retaining the existing stream.Fields checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7bb12b0-0c2d-4a2b-a0bf-a871de8c626c
📒 Files selected for processing (5)
AGENTS.mdinternal/mediainfo/format_duration.gointernal/mediainfo/format_duration_test.gointernal/mediainfo/matroska_ac3_canonical.gointernal/mediainfo/matroska_eac3_probe_test.go
💤 Files with no reviewable changes (1)
- internal/mediainfo/matroska_ac3_canonical.go
The text assertions cover the default output, because stream.Fields is already the projectTextStore result that RenderText uses. Nothing covered the other half of the split, so add one check that raw text still carries dialnorm.
1 participant
This continues the Matroska default-text work in #32, #33, and #35. The text output still printed three duration units past one hour. Matroska E-AC-3 tracks also copied raw mixing and dialnorm metadata into the same output. Official MediaInfo prints two duration units and keeps that metadata under JSON
extra. #33 removed theltrtandloropair, and this removes the rest.Those PRs measured against samples
mkv-01tomkv-10. That set cannot show either fault, because every sample is shorter than one hour and none is FLAC or DD+ 7.1. Fourteen new mkv samples found both. Against official v23.04 on 32 real files the result is 14 better, 18 same, and 0 worse. JSON and--language=rawoutput stay byte-identical, including on the BDAV and DVD controls.Summary by CodeRabbit