use standard types instead of custom ones

[mbunkus]

Let's have an issue focussed on the upcoming change to using standard types (e.g. std::uint32_t) instead of our custom ones (e.g. uint32).

There's an older PR, #86, which does a lot of what I'd like to see, but it's somewhat controversial. The points I've seen there & in other places are:

• switch all (u)int(8|16|32|64) to std::(u)int(8|16|32|64)_t (probably uncontroversial)
• switch size_t to std::size_t
• keep the old (u)int(8|16|32|64) types around for backwards compatibility but derive them from the official types instead of doing our own compiler- & OS-based #ifdef thing
• the header libebml_t.h is suposed to be usable from C applications as well, leading to a couple of follow-up questions:
  • are there any known C applications that actually use it?
  • do we want to keep it around? If so, we must derive our backwards-compatible types from the C types, not the C++ ones (meaning… stdint.h)
  • do we want to remove it? → somewhat serious API break if there are actual users outside of our libraries.

Anything else I've missed?

[mbunkus]

@robUx4 Pinging for your input, mostly wrt. to the C-style include file.

[robUx4]

keep the old (u)int(8|16|32|64) types around for backwards compatibility but derive them from the official types instead of doing our own compiler- & OS-based #ifdef thing

If we're OK with API breakage, we can remove the old types altogether. With #107 there's already some code that may have to be conditionally compiled between libebml 1.x and 2.x. So we can also remove them IMO.

are there any known C applications that actually use [libebml_t]?

Not AFAIK. The VLC module using libebml/libmatroska is in C++.

do we want to keep it around?

No

If so, we must derive our backwards-compatible types from the C types, not the C++ ones (meaning… stdint.h)

We don't, but they are supposed to be compatible anyway.

[mbunkus]

According to this comment there are only three programs packaged in Debian that use the libraries: VLC, MKVToolNix and gerbera. MKVToolNix had been a heavy user of the custom types but has since been converted to using all standard types.

gerbera has exactly two occurrences (1, 2) of the custom types that I can see. Both are trivial to fix. The first occurrence has to be fixed due to the breaking change of the return type of IOCallback::read() anyway. I suggest we provide PRs for the changes.

VLC uses the types extensively in all of their Matroska code, but those look easy enough to convert.

All in all we should probably keep them around for the time being but deprecate them. Then work on replacing them in gerbera & VLC first.

[robUx4]

VLC uses the types extensively in all of their Matroska code, but those look easy enough to convert.

I'll take care of that when we switch to 2.x libebml/libmatroska. It shouldn't be a problem.

[neheb]

since gerbera was mentioned, its matroska handling leaks memory. I had a PR that fixed it: gerbera/gerbera#2280 but upstream is convinced the actual bug is in libebml.

Since libebml is C++14 now, would it make sense to make use of std::shared_ptr for 2.0? gerbera and mkvtoolnix are C++17 and VLC is c++14 (I think?).

edit: libebml_t.h should go away IMO. There are no C users. AFAIK they use whatever libavX provides them.

edit2: speaking of libavX, reminds me of this little difference: gerbera/gerbera#2155

[robUx4]

Fixed by #121 or is there anything left ?

[mbunkus]

There are still a couple of places left where the standard types are used without the std:: namespace:

• src/IOCallback.cpp 1, 2
• src/EbmlCrc32.cpp lines 43–47

For consistency's sake those should be changed, too.

Other than these places I haven't found anything else, including in libMatroska.

[mbunkus]

Well, that and we could discuss removing filepos_t or at least making it namespace-scoped as we're breaking API anyway. It's such a general name, and having it in the main namespace kind of sucks.

Similar argument for removing the binary type.

Or at least changing all of our sources to the underlying types & only keeping the type aliases around as a backwards-compatibility thing.

[robUx4]

We should try to move to standard usage of the vector as well. I think it should be possible to have a default clone constructor for all classes. The tricky one is EbmlMaster which currently stores pointer of elements it holds. As said above, using std::shared_ptr should help with this.

[mbunkus]

I'd love for the library to completely remove the manual memory handling & using std::shared_ptr (or std::unique_ptr if more appropriate), not just for EbmlMaster but for the others, too. A quick rg '\bnew\b' finds occurrences in EbmlElement, EbmlString, EbmlUnicode, EbmlCrc32, SafeReadIOCallback, KaxCues & KaxBlock.

[neheb]

Changes API, no?

[mbunkus]

It sure does, but we're already changing the API quite a bit with all the other changes. I just think that this would be a really big step in the right direction of using proper modern memory management that it's worth it.

[robUx4]

For the record I found some more (more or less) active projects using libebml/libmatroska:

• lumahdrv: https://github.com/gabrieleilertsen/lumahdrv/blob/4cf5949f496341a0d6dc22862de4634c546fc5c6/src/mkv_interface.cpp#L42
• librgbd: https://github.com/telegie/librgbd/blob/f2287b015bcb7cefff2486bf866fd4a62b35a649/src/record_writer.cpp#L28
• MkvImport: https://github.com/MaddTheSane/MKVImport/blob/master/MKVImporter/GetMetadataForFile.mm