Skip to content
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

Don't export EbmlMaster context, use the class static member #269

Merged
merged 16 commits into from
Mar 3, 2024

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Feb 24, 2024

The internal are reworked so only semantic context of EbmlMaster elements have a table of possible children.

"[Get]ElementSpec()" is generalized so there's stronger typing of the semantic context for all the types.

The callback to get the global elements now returns a EbmlSemanticContextMaster. Ultimately it only needs to return a container with a list of semantic context.

@robUx4 robUx4 added api-break breaks the API (e.g. programs using it will have to adjust their source code) abi-break breaks the ABI (e.g. programs linked against the library have to be recompiled) labels Feb 24, 2024
@robUx4 robUx4 force-pushed the hide_context branch 2 times, most recently from 7e03bb6 to 41895bd Compare February 25, 2024 16:00
@mbunkus
Copy link
Contributor

mbunkus commented Feb 25, 2024

Unfortunately this breaks this function & several other similar ones in MKVToolNix. The error message is:

src/common/ebml.cpp:88:15: fatal error: no matching function for call to 'tEBML_CTX_IDX'
    if (id == EBML_CTX_IDX_ID(context,i))
              ^~~~~~~~~~~~~~~~~~~~~~~~~~
/home/mosu/prog/video/mkvtoolnix/lib/libebml/ebml/EbmlElement.h:274:45: note: expanded from macro 'EBML_CTX_IDX_ID'
#define EBML_CTX_IDX_ID(c,i)   EBML_INFO_ID(EBML_CTX_IDX_INFO(c,i))
                                            ^~~~~~~~~~~~~~~~~~~~~~
/home/mosu/prog/video/mkvtoolnix/lib/libebml/ebml/EbmlElement.h:273:47: note: expanded from macro 'EBML_CTX_IDX_INFO'
#define EBML_CTX_IDX_INFO(c,i) EBML_SEM_SPECS(EBML_CTX_IDX(c,i))
                                              ^~~~~~~~~~~~~~~~~
/home/mosu/prog/video/mkvtoolnix/lib/libebml/ebml/EbmlElement.h:272:32: note: expanded from macro 'EBML_CTX_IDX'
#define EBML_CTX_IDX(c,i)      tEBML_CTX_IDX(c,i)
                               ^~~~~~~~~~~~~
/home/mosu/prog/video/mkvtoolnix/lib/libebml/ebml/EbmlElement.h:265:45: note: expanded from macro 'EBML_SEM_SPECS'
#define EBML_SEM_SPECS(s)   tEBML_SEM_SPECS(s)
                                            ^
/home/mosu/prog/video/mkvtoolnix/lib/libebml/ebml/EbmlElement.h:260:45: note: expanded from macro 'EBML_INFO_ID'
#define EBML_INFO_ID(cb)      tEBML_INFO_ID(cb)
                                            ^~
/home/mosu/prog/video/mkvtoolnix/lib/libebml/ebml/EbmlElement.h:531:36: note: candidate function not viable: no known conversion from 'const EbmlSemanticContext' to 'const EbmlSemanticContextMaster' for 1st argument
static inline const EbmlSemantic & tEBML_CTX_IDX(const EbmlSemanticContextMaster & c, std::size_t i)
                                   ^
1 error generated.

Any pointers how I would have to change my code? I need to iterate over the whole semantic tree & find elements, either by their ID, or by their debug name.

@robUx4
Copy link
Contributor Author

robUx4 commented Feb 26, 2024

I got the same problem in mkvalidator. I pushed a fix.

@mbunkus
Copy link
Contributor

mbunkus commented Mar 2, 2024

Compilation still fails after merging 9e6f5b5 (& 'hide_context' for libMatroska) with the same error message I've posted above. Meaning the same issue remains; I need to be able to iterate over the whole structure, and I don't know how after these changes.

@robUx4
Copy link
Contributor Author

robUx4 commented Mar 2, 2024

OK, I see the problem. Now that there's stronger typing you can only get a child EbmlSemantic from an EbmlSemanticContextMaster. You doesn't make much sense for an integer or a binary context. (I'd like to split this even more, but without relying on RTTI).

I'll make the patches for mkvtoolnix.

@mbunkus
Copy link
Contributor

mbunkus commented Mar 2, 2024

As written over on Gitlab, but to keep the conversation here: I needed a couple of fixes on top of your MKVToolNix PR to make MKVToolNix compile. Afterwards a handful of my test cases fail.

I've now found out why they fail: SkipData() works differently for elements with unknown sizes after merging this PR. Before the PR calling SkipData() on an element (a cluster in this case) with an unknown size caused the file pointer to be set to the end of the file, causing further reads to fail. That was fine for me.

After the PR the file pointer is now directly at the start of the cluster's data portion (directly after the cluster's head).

I consider the new behavior to be bad & wrong.

A typical algorithm used by MKVToolNix is to create a list of level 1 elements like this:

  1. read the EBML head
  2. find the segment
  3. while not at end of file:
    1. find next ID
    2. read element head
    3. make note of its position & size
    4. skip over it

This worked fine with the old behavior. With the new one the "skip over it" part will leave the file pointer after the level 1 element's head, meaning that the next "find next ID" call will find the ID of the first child element of that level 1 element.

That is a substantial change in semantics. Sure, I could work around this, but I shouldn't have to, in my opinion, as the new behavior is strictly worse to the old one.

Unfortunately I really don't understand why the changes in this PR affect skipping data that way & can therefore not offer a fix myself.

@mbunkus
Copy link
Contributor

mbunkus commented Mar 2, 2024

BTW, the file in question is this one.

@robUx4
Copy link
Contributor Author

robUx4 commented Mar 3, 2024

I removed the FindChildByEbmlId() commits for now. That's new code to simplify getting a child semantic. It's not needed in this PR and may be the source of the regression. I also updated Matroska-Org/libmatroska#183 to match this PR.

robUx4 added 10 commits March 3, 2024 09:08
It doesn't make sense for other types on element/context.
The size if kept in the EbmlSemanticContext to notify when it's a master element.
It can only be context with a list of elements.
EbmlHead is public/shared class. One can call EbmlHead::GetContextMaster();
Even though it's not very useful as a static member variable...
… any master class

This is the counterpart to EBML_CLASS_CONTEXT for master elements.
It's only used with master element classes in mkvtoolnix of VLC.

This will make iterating through the class context children possible without any code change.
Hopefully the compiler picks the most appropriate one ?
@robUx4
Copy link
Contributor Author

robUx4 commented Mar 3, 2024

I think I identified the source of the issue. The if (EltIndex >= EBML_CTX_SIZE(Context)) { par of the code was no longer executed in SkipData() when a child element was not found for infinite size elements.

@mbunkus
Copy link
Contributor

mbunkus commented Mar 3, 2024

Yep, that was it. Thanks!

@mbunkus mbunkus merged commit ceba125 into Matroska-Org:master Mar 3, 2024
17 checks passed
@robUx4 robUx4 deleted the hide_context branch March 4, 2024 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break breaks the ABI (e.g. programs linked against the library have to be recompiled) api-break breaks the API (e.g. programs using it will have to adjust their source code)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants