search

ffmpeginteropx / FFmpegInteropX

FFmpeg decoding library for Windows 10 UWP and WinUI 3 Apps
Apache License 2.0
210 stars 53 forks source link

Inline classes #2

Closed brabebhin closed 6 years ago

brabebhin commented 6 years ago

One of the things that really bothered me with ffmpeg interop from microsoft was the use of split cpp and .h files.

Should we continue on with this or move to inline classes? With modern compilers, there is really no advantage to having them split anymore, and inline classes are easier to read. There are successful header-only libraries for C++ out there, I don't see the point in sticking to this old approach.

lukasf commented 6 years ago

I also do not like the split implementation. It is pretty annoying having to write the same method declaration in two different places, and having to update two places when it changes.

For bigger projects, there are compile time benefits when using split file. If you only use headers and change a method in one header that is referenced by a lot of other classes, then all classes will need to be rebuilt. But if you have split header and cpp, and change the implementation in cpp file, then only that cpp needs to be rebuilt. But for a small project such as this, complie time is generally not a big issue.

When working on FFmpegInterop, I tend to write small methods directly in the header (such as method overloads, empty virtual methods). I only put larger methods into the cpp file.

I don't know if we should start inlining all methods. There is no real functional need for that, and I don't know if it would be accepted by for merge back into the official repo.

What personally bothers me a bit more is traditional c++ member prefixing. I hate having to type 3-4 letters before intellisense even starts coming up with relevant results. In the project, some files use prefixing, others don't. At least for new code, I'd propose to go without prefixing.

brabebhin commented 6 years ago

I do not think that compile times are still a problem with VS2017. If the C# compiler can do it, C++ can do it as well, the compilation processes are really not that different. Hell, .net native uses the same similar toolchain as C++ and still doesn't need split files. I think we can safely go to inline classes, it is not like this project will grow out of control.

Regarding merge-back into the original project, if they don't allow us back in it is fine after all. And they did allow me to merge an inline class (although a small one).

lukasf commented 6 years ago

I agree that we do not absolutely have to merge this back into the official repo. The lib is pretty good already, and if we can add multiple audio streams, there is not much missing anymore. Still, it would be good to automatically get fixes, if someone else finds something and contributes to official repo. So I would at least prefer to merge it back at some point. That is the reason why I would not rework existing classes right now when there is no functional need for it. For any new classes, we can go header-only, I'm all for it.

MouriNaruto commented 6 years ago

How about adding the video subtitle support?

Many video has inline-subtitles.

brabebhin commented 6 years ago

v-next of winRT.

lukasf commented 6 years ago

We will probably add support for multiple audio channels first. This is possible in current WinRT version already. The same infrastructure can then be re-used for (multiple) subtitle support, once WinRT vNext with MediaStreamSource subtile support is there.