mkvalidator: Fix parsing of Video element and checking of cues #39
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The earlier code checked the PixelCrop* elements as if they applied after the scaling to DisplayWidth/Height has been done. Furthermore some checks have been improved (the earlier code didn't check whether PixelWidth/Height is zero). Moreover a heuristic check has been added to test whether it has been forgotten to update DisplayWidth/Height in light of the PixelCrop* elements. Signed-off-by: Andreas Rheinhardt <[email protected]>
Signed-off-by: Andreas Rheinhardt <[email protected]>
The current level basically ensures that files produced by mkvmerge produce a warning, although they don't use an excessive amount of void data. So raise the arbitrary threshold a bit to make this warning meaningful. Signed-off-by: Andreas Rheinhardt <[email protected]>
This function seems to have been copied and pasted from MATROSKA_CueTimeCode, with only minimal modifications performed to make it do what its name suggest. This means that a variable that never stores a time is called TimeCode and that the function returns INVALID_TIMECODE_T instead of INVALID_FILEPOS_T. These things have been changed. Signed-off-by: Andreas Rheinhardt <[email protected]>
The differences of MATROSKA_GetNextBlockForTimecode and the classical MATROSKA_GetBlockForTimecode are threefold: 1. The new function can return both the inner Block as well as the outer BlockGroup based upon a parameter (that is irrelevant for SimpleBlocks). 2. Consequently the return value is a pointer to an ebml_element and no longer a matroska_block*. 3. It also allows to get a matching block that is not the first matching block in a cluster. This commit is in preparation of a patch for mkvalidator. Signed-off-by: Andreas Rheinhardt <[email protected]>
mkver
changed the title
The earlier code had several flaws: 1. It was very slow, because finding a matching block always started with the very first block in the first cluster and checked every block until an acceptable block (one with the right timestamp from the right track) had been found. 2. Only one CueTrackPositions (namely the first one) has been checked per CuePoint, although there can be multiple CueTrackPositions. 3. If e.g. no CueTrack element was present, one got a generic error (error code 0x200), because a mandatory element was missing; but MATROSKA_CueTrackNum returned -1 (for invalid TrackNum) and so one also received a nonsense error 0x312: "CueEntry Track #-1 and timecode xxx ms not found". 4. The CueClusterPosition and CueRelativePosition elements have not been checked at all. All four points have been corrected. (mkvmerge versions 5.9 to 6.3 (inclusive) wrote CueRelativePosition elements that (in case of BlockGroups) pointed to the inner blocks and not the outer BlockGroups. mkvalidator now detects such files as invalid.) Signed-off-by: Andreas Rheinhardt <[email protected]>
|
This looks useful. Please review and merge. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The earlier code checked the PixelCrop* elements as if they applied after the scaling to DisplayWidth/Height has been done.
Furthermore some checks have been improved (the earlier code didn't check whether PixelWidth/Height is zero). Moreover a heuristic check has been added to test whether it has been forgotten to update DisplayWidth/Height in light of the PixelCrop* elements.
I tried to preserve the error codes as much as possible; after all, they may be used in scripts.
Also added is a quick fix for a bug in calculating the amount of void data.
[Edit]: I have now also fixed #28. The speed gain is truly tremendous: The stuff that is being done after the file has been completely read is now instantaneous, whereas before it could take some time to check all the cues in case that the GOPs are short (think of remuxes of DVDs, Blurays or TV) or that there are multiple subtitle tracks with their own cues.
I needed a modified version of MATROSKA_GetBlockForTimecode to achieve my goals; that's the reason why I included this commit in this PR. And the commit relating to MATROSKA_CuePosInSegment is not really necessary, but I stumbled upon this during the course of the cues-patch for mkvalidator, so I include it here, too. (I retained the Timecode in the name of the new function for consistency with the old one; I don't know if this is desired or if you want to use Timestamp instead.)
And I have have increased the amount of void data accepted, because there is really no reason to warn for files created by mkvmerge. They are (usually) fine.