-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consider removing outdated/deprecated Matroska Elements #117
Comments
if is not too terrible to implement we could have the two modes, |
I agree with @lu-zero about the two modes. Would it be an effort to define a file containing all deprecated elements and putting them behind a feature? In this way, to not break compatibility when old elements are required, one can only enable the |
It should be possible with a bit of work to get the two modes to work. So either A) fail immediately when encountering an unknown ID or B) log the unknown ID, and just move on. @Luni-4 Introducing a So the takeaway for now would be:
|
Yep, I agree with your analysis @FreezyLemon |
Some more info: That list of deprecated Elements has been in the draft since mid-2021: ietf-wg-cellar/matroska-specification#487. So not that long. There was some discussion surrounding DivX elements in the spec: ietf-wg-cellar/matroska-specification#305. Looks like nothing outside of DivX tools really used these elements. Also: The section about versioning says this about reading newer versions of the format:
Basically, if the document is a higher version than we support, ignore all unknown Elements as long as the Element Sizes are correct. |
Okay. The current behaviour is to fail when encountering an unknown Element. The error message is also not very good and does not indicate that we're failing because of an unknown Element ID. #123 changes this behaviour. It makes the library skip all unknown Elements with a I honestly don't see a use case for rejecting the entire Matroska file because of an unknown Element. It's always possible to have a file with some weird extension or just a newer version of the spec that is not supported yet (the spec also acknowledges this). Feel free to correct or override me, but after thinking about it, I don't really see the need for this "feature". |
I agree with your solution based on
Could it be a consistent solution in your opinion @FreezyLemon? |
Sure, that should work. It would make the API a bit more complicated, but should be possible. |
Instead of a boolean flag we can define an enumerator describing that behavior and respectively change the |
Since we have moved the main discussion in #126 we can close this issue. Reopen it if I'm wrong |
What do we do with Elements that have been removed from the Matroska spec? It's not clear to me yet, so I think the issue should stay open for now. |
Just a possible idea, sorry if I misunderstood the problem Can we create an hard-coded list in the library binary containing all Elements removed from the matroska specification over the years, perhaps subdivided for specification, and if we encounter an unknown element, as first operation we look for in that list if it's present? We can then notify the user with a warning that the specified element has been removed from the matroska specification. We can also add to this list additional info about the removal. |
I like that idea. There's a list of Elements at the bottom of the Matroska spec, I think we could start with that and check for "Reclaimed" Elements. |
Yep, let's start with them |
See the latest Matroska draft:
There are quite a few Elements listed there that were still in the spec when they were added to this code base (the comments refer to draft v03), but are now removed. Some examples are BlockVirtual, ReferenceVirtual, SilentTracks and all the DivX TrickTrack extension elements.
Maybe there should be a discussion on how to handle Matroska/EBML versioning in general. I can see some ways to handle Elements being removed from the spec.
One other thing I'm not 100% sure about is this: What happens if the parser encounters an unknown Element ID while parsing? If we go down route 2 or 3, and the parser does encounter an Element that it doesn't know, will it just completely fail parsing that Element? I'd argue that (at least for a list of known, deprecated IDs), the parser should just skip the deprecated Element instead of rejecting the entire file. I think that it currently errors out, but I am not sure. Something to test (and maybe introduce tests for).
(I'll soon add a list of Elements that are currently not in the spec, but still in the code base. IMO we should decide on these Elements too, to get a "clean slate". Discussion can start before that though)
The text was updated successfully, but these errors were encountered: