[FIX] Fix ownership of default Matroska track language#2193
[FIX] Fix ownership of default Matroska track language#2193Snorlax-011 wants to merge 3 commits intoCCExtractor:masterfrom
Conversation
|
Adding a bit of context for reviewers: this PR is intentionally kept minimal. The issue is that the default language value was a string literal ( I intentionally did not keep the earlier Validated with a successful WSL/Linux build. |
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
Thanks for the PR — the core fix (NULL check on strdup and removing the broken IETF switch-case code) is correct. That switch-case code was indeed a no-op: it freed lang, then immediately re-allocated "eng", never actually preferring lang_ietf.
However, the PR also removes the working IETF language preference from filename generation and language matching, which is a behavioral regression:
generate_filename_from_trackusedtrack->lang_ietf ? track->lang_ietf : track->lang— that was working correctlysave_vobsub_track— same pattern, also workingmatroska_save_all— IETF tag matching removed
After this PR, lang_ietf is still read, printed, stored in the track, and freed — but never used for anything. That's wasteful and loses functionality.
Could you either:
- Restore the
lang_ietf ? lang_ietf : langpreference in those three places (the filename generation and language matching code was fine, only the switch-case handling was broken), or - Explain why removing IETF preference is intentional — maybe there's a reason I'm not seeing?
The strdup NULL check and removal of the broken switch-case code are good to go. Once the IETF question is resolved, this is ready to merge.
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
Thanks for the PR — the core fix (NULL check on strdup and removing the broken IETF switch-case code) is correct. That switch-case code was indeed a no-op: it freed lang, then immediately re-allocated "eng", never actually preferring lang_ietf.
However, the PR also removes the working IETF language preference from filename generation and language matching, which is a behavioral regression:
generate_filename_from_trackusedtrack->lang_ietf ? track->lang_ietf : track->lang— that was working correctlysave_vobsub_track— same pattern, also workingmatroska_save_all— IETF tag matching removed
After this PR, lang_ietf is still read, printed, stored in the track, and freed — but never used for anything. That's wasteful and loses functionality.
Could you either:
- Restore the
lang_ietf ? lang_ietf : langpreference in those three places (the filename generation and language matching code was fine, only the switch-case handling was broken), or - Explain why removing IETF preference is intentional — maybe there's a reason I'm not seeing?
The strdup NULL check and removal of the broken switch-case code are good to go. Once the IETF question is resolved, this is ready to merge.
Restore the working IETF language preference that was accidentally removed in the previous commit: - generate_filename_from_track(): use lang_ietf ? lang_ietf : lang for buffer sizing and both snprintf calls - save_vobsub_track(): same lang_tag pattern for base filename - matroska_save_all(): check lang_ietf first (BCP-47), fall back to lang (ISO-639-2) when --mkvlang filter is active; remove now-unused char *match variable The strdup NULL check and broken switch-case removal from the prior commit are unchanged.
|
summary of what's in the final state:
Also fixed a clang-format issue (spaces -> tabs). Thanks for reviewing😊 |
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit ed7f544...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit ed7f544...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
3 participants
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
Summary
Fix ownership of the default Matroska track language string in
parse_segment_track_entry().What changed
strdup("eng")and add a NULL check after the allocationlang_ietf(BCP-47) overlang(ISO-639-2) ingenerate_filename_from_track(),save_vobsub_track(), andmatroska_save_all()language matchingWhy
langis treated as an owned heap string and freed during cleanup, so the default"eng"value must also be heap-allocated to keep ownership consistent and avoid invalid frees.
The
lang_ietfpreference in filename generation and--mkvlangmatching is also restored,as it was accidentally lost in an earlier iteration of this patch.
Validation
.clang-format)