Add a callback to select which element are written #173

robUx4 · 2023-12-18T15:07:48Z

And remove the ForceNoDefault() which is not needed anymore.

There's a EBML_WRITE_FILTER define to tell between libmatroska versions with this feature or not.

mbunkus

Interesting, thanks. I haven't tried converted MKVToolNix yet & cannot say how much work supporting both types of interfaces would be & whether I like it or not. Fact is that this type of interface change does require a substantial amount of changes, whereas the simple flag/toggle I suggested to add in the elements wouldn't. On the other hand callback-type systems are more flexible. But do we really need that flexibility?

Have to think about it some more.

ebml/EbmlElement.h

mbunkus · 2023-12-18T16:19:55Z

src/EbmlElement.cpp

@@ -601,21 +601,21 @@ filepos_t EbmlElement::OverwriteData(IOCallback & output, bool bKeepPosition)

  auto CurrentPosition = output.getFilePointer();
  output.setFilePointer(GetElementPosition() + HeaderSize);
-  auto Result = RenderData(output, true, bKeepPosition);
+  auto Result = RenderData(output, true, bKeepPosition ? WriteAll : WriteSkipDefault);


I don't understand the intent behind this change. What does keeping the position have to do with the question whether or not to write the element?

This is just the translation of the previous call into the new system. bKeepPosition would be used as the bWithDefault value. So if it's true it writes elements even if they are the default value (WriteAll), if it's false it only writes the elements if they are not the default value (SkipDefault).

The original logic may be flawed and it should be fixed separately. In fact when overwriting you'd probably want to tell which elements you want to write (the least possible).

This whole API is a recipe for broken files (which originally was a welcome possibility in libebml to be able to generate broken files to test against). It seems mkvtoolnix doesn't use it, there's only a test calling OverwriteHead(). So I'd rather remove the API altogether.

Ah, I see. Thanks for the explanation. Yeah, MKVToolNix (or rather: mkvpropedit) only uses OverwriteHead for cases where it has to cover a one-byte space that's suddenly left by other operations; in such a case it forces the element size field to be one byte longer than necessary.

It never uses OverwriteData, though. If it has to overwrite a whole element, I think it simply uses Render & sets the file position manually.

mbunkus · 2023-12-18T16:23:38Z

ebml/EbmlElement.h

@@ -413,4 +427,6 @@ class EBML_DLL_API EbmlElement {

 } // namespace libebml

+#define EBML_WRITE_FILTER 1


I… don't like this #define. Consumers can already use the version number #defines in order to differentiate between v1 & v2 of the library.

Not sure how I'll handle it in MKVToolNix. Will have to look into how many places will have to be changed as I'm generally not a fan of a lot of conditional compilation anyway. Maybe I'll just throw the switch & require v2 as soon as I add support for it.

The define is there to tell v2 users which API they can expect. Once v2 is stable we can remove it and everyone should assume it's there for v2 code. In other words it's to make your life easier if you want to support the old and new API in your codebase for a while. I don't mind removing it and you handle it on your side.

Makes sense, but isn't needed, at least not for me. As there's no official release of v2 yet, I won't support different snapshots of the upcoming v2 API in MKVToolNix. I'll try my best to support v1 & the latest v2 simultaneously, but even that is in question with this RFC.

robUx4 · 2023-12-19T06:57:14Z

whereas the simple flag/toggle I suggested to add in the elements wouldn't. On the other hand callback-type systems are more flexible. But do we really need that flexibility?

There's either the ForceNoDefault() we currently have which tells a specific instance it's not it's default value (even when it does, completely changing the meaning of the IsDefault test). Even if you change the value after. Or we keep the IsDefault meaning intact and change the logic to tell if an element has to be written. Maybe we can still add a flag in each instance that would replace the ForceNoDefault(). But IMO it's intrusive and wastes space in each and every instance. While passing the "algorithm" to decide how to store it when we do seems less intrusive.

Being less intrusive you don't have to have a kax_block_add_id_c class in mkvtoolnix anymore.

robUx4 · 2023-12-19T07:11:43Z

Also this kind of "agnostic" filter means we can also select which element to write based on the Matroska version targeted. When writing a v2 file, you don't want to end up writing v4 or v5 elements. Once we have the supported version inside each element class, we can easily do that.

mbunkus · 2023-12-19T08:15:07Z

There's either the ForceNoDefault() we currently have which tells a specific instance it's not it's default value (even when it does, completely changing the meaning of the IsDefault test). Even if you change the value after.

Hmmm I somehow wasn't aware of ForceNoDefault(). I'll try to look into it some more before Christmas, including how much of a change MKVToolNix would need to support this type of callback API.

mbunkus · 2023-12-19T08:18:56Z

Also this kind of "agnostic" filter means we can also select which element to write based on the Matroska version targeted. When writing a v2 file, you don't want to end up writing v4 or v5 elements. Once we have the supported version inside each element class, we can easily do that.

Not sure if that would actually help me. MKVToolNix does it the other way around: it first writes v2 as the DocTypeVersion & updates that field depending on which features are activated & used further down the line (e.g. after writing the track headers it potentially updates the EBMLHead with the actual profile values used if they differ from what's been written initially).

If the library routines need to know what to use before the whole file is written, a lot would have to changed in MKVToolNix. Or I'd simply have to use a filter callback that ignores the profiles altogether so that the existing code continues to work. For me this type of feature be an advantage.

robUx4 · 2023-12-19T08:26:09Z

Not sure if that would actually help me. MKVToolNix does it the other way around: it first writes v2 as the DocTypeVersion & updates that field depending on which features are activated & used further down the line (e.g. after writing the track headers it potentially updates the EBMLHead with the actual profile values used if they differ from what's been written initially).

If the library routines need to know what to use before the whole file is written, a lot would have to changed in MKVToolNix. Or I'd simply have to use a filter callback that ignores the profiles altogether so that the existing code continues to work. For me this type of feature be an advantage.

I wasn't sure if I should add an opaque pointer (or if there's a cleaner way in C++ for such callbacks) but it might be useful in your case. During the write phase each element written goes through your filter which could collect the highest profile used.

Not that you'd have to implement it this way, but it might be useful as a generic solution.

ebml/EbmlElement.h

mbunkus · 2023-12-27T13:51:20Z

I've finished the required modifications in MKVToolNix. All in all I'm fine with this change if the callback type is changed to the aforementioned std::function<void(const EbmlElement &)> instead. That allows me to use std::bind() in order to have a member function be a callback, simplifying what I have to do. It's just way more flexible than a plain old C-style function pointer.

On top of that the only remaining change I'd like to ask for is the other aforementioned one, getting rid of that one unused variable warning.

Thanks.

mbunkus · 2023-12-27T21:20:50Z

I've pushed branch callback_select_write_enhanced to this repo: it's your PR rebased onto current master with the two additional changes I asked for. This is what I've tested the support in MKVToolNix against.

This is more flexible than a flag that applies to all the elements inside a Master. It can now be filtered by element, based on their ID, position, profile, etc. A EBML_WRITE_FILTER define is added to be able to check libmatroska 2.0 with this feature or not.

This is a hack to pretend an element doesn't have its default value so it ends up being written.

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 Dec 18, 2023

robUx4 changed the title ~~Add a callback to select which element are written~~ [RFC] Add a callback to select which element are written Dec 18, 2023

robUx4 force-pushed the callback_select_write branch from 51eb97d to 844f7ac Compare December 18, 2023 15:12

mbunkus requested changes Dec 18, 2023

View reviewed changes

robUx4 force-pushed the callback_select_write branch from 844f7ac to 40d64d9 Compare December 19, 2023 07:00

robUx4 mentioned this pull request Dec 19, 2023

Switch to the ShouldWrite API to select which elements to write Matroska-Org/libmatroska#122

Merged

robUx4 force-pushed the callback_select_write branch from 40d64d9 to f735b15 Compare December 23, 2023 06:53

robUx4 mentioned this pull request Dec 26, 2023

Use templates for the base types to factorize code #186

Merged

mbunkus reviewed Dec 26, 2023

View reviewed changes

ebml/EbmlElement.h Outdated Show resolved Hide resolved

mbunkus requested changes Dec 27, 2023

View reviewed changes

ebml/EbmlElement.h Outdated Show resolved Hide resolved

robUx4 and others added 4 commits December 28, 2023 14:29

remove ForceNoDefault()

213380d

This is a hack to pretend an element doesn't have its default value so it ends up being written.

use std::function as callback for ShouldWrite

0391375

avoid "unused variable" warnings

8b6c24e

robUx4 force-pushed the callback_select_write branch from 16d5d6a to 8b6c24e Compare December 28, 2023 13:29

robUx4 changed the title ~~[RFC] Add a callback to select which element are written~~ Add a callback to select which element are written Dec 28, 2023

robUx4 merged commit e7359b5 into Matroska-Org:master Dec 28, 2023
15 checks passed

robUx4 deleted the callback_select_write branch December 28, 2023 13:34

neheb mentioned this pull request Jan 28, 2024

pass std::function by const ref #231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a callback to select which element are written #173

Add a callback to select which element are written #173

robUx4 commented Dec 18, 2023

mbunkus left a comment

mbunkus Dec 18, 2023

robUx4 Dec 19, 2023

mbunkus Dec 19, 2023

mbunkus Dec 18, 2023

robUx4 Dec 19, 2023

mbunkus Dec 19, 2023

robUx4 commented Dec 19, 2023 •

edited

Loading

robUx4 commented Dec 19, 2023

mbunkus commented Dec 19, 2023

mbunkus commented Dec 19, 2023

robUx4 commented Dec 19, 2023

mbunkus commented Dec 27, 2023

mbunkus commented Dec 27, 2023

		@@ -413,4 +427,6 @@ class EBML_DLL_API EbmlElement {

		} // namespace libebml

		#define EBML_WRITE_FILTER 1

Add a callback to select which element are written #173

Add a callback to select which element are written #173

Conversation

robUx4 commented Dec 18, 2023

mbunkus left a comment

Choose a reason for hiding this comment

mbunkus Dec 18, 2023

Choose a reason for hiding this comment

robUx4 Dec 19, 2023

Choose a reason for hiding this comment

mbunkus Dec 19, 2023

Choose a reason for hiding this comment

mbunkus Dec 18, 2023

Choose a reason for hiding this comment

robUx4 Dec 19, 2023

Choose a reason for hiding this comment

mbunkus Dec 19, 2023

Choose a reason for hiding this comment

robUx4 commented Dec 19, 2023 • edited Loading

robUx4 commented Dec 19, 2023

mbunkus commented Dec 19, 2023

mbunkus commented Dec 19, 2023

robUx4 commented Dec 19, 2023

mbunkus commented Dec 27, 2023

mbunkus commented Dec 27, 2023

robUx4 commented Dec 19, 2023 •

edited

Loading