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

make EbmlElement::HeadSize private #258

Merged
merged 14 commits into from
Feb 24, 2024
Merged

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Feb 18, 2024

The Head size is rarely needed and may be 0 depending on writing filter.

Draft on top of #255

It won't change after the initial computation.
Either it's 0 when we the replacement element fully replaced the EbmlVoid. Or we have a valid size.
It may be filtered out which will result in a size of 0.
Return an error since the replacement didn't happen.
The result is used that way in mkvtoolnix.
Even if it would be filtered out, we want to know its actual size.
And expand/shrink the values that can be safely changed based on the context.
It's not specific to EbmlMaster.
The Head size is rarely needed and may be 0 depending on writing filter.
@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 18, 2024
@mbunkus
Copy link
Contributor

mbunkus commented Feb 18, 2024

Hard no from me. mkvpropedit & the header editor both need to know the head size in order to determine where to place things. Sometimes they even have to expand the head size (by making the size field one byte larger the the minimum size) if they have to replace an existing element with an element exactly one byte smaller (meaning they cannot use an EbmlVoid there).

@robUx4
Copy link
Contributor Author

robUx4 commented Feb 18, 2024

to determine where to place things.

There's GetDataStart() for that. Now it's available for all elements.

To expand the size you can use the Get/SetSizeLength() without having to know the exact size.

@mbunkus
Copy link
Contributor

mbunkus commented Feb 18, 2024

So what's the use of removing such a useful function? Apart from creating more work for me. Yes, I'm a bit salty at the moment due to the amount of churn I have to invest in order to keep MKVToolNix compile with the ever changing libs 2.0 & still keep it compatible with 1.x.

So — honest question. What's gained by removing this function?

@robUx4
Copy link
Contributor Author

robUx4 commented Feb 19, 2024

So — honest question. What's gained by removing this function?

Reducing the amount of API we have to support. We should have the bare minimum so that mkvtoolnix and VLC (and mkvalidator/mkclean) work. In this case the start of the data seems more interesting in general than the size of the head. In the end you can deduce one from the other.

@mbunkus mbunkus merged commit e415b3c into Matroska-Org:master Feb 24, 2024
15 checks passed
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