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

Ffmpeg abstractions #219

Closed

Conversation

digivod
Copy link
Contributor

@digivod digivod commented Aug 17, 2022

Played a bit with ffmpeg_abstractions branch. Looks great!
Some suggestions for improvements as pull request

  1. I use ffmpeg bitstreamfilters defined in avcodec/bsf.h. So I added that. (not reallay related to abstractions)
  2. For the "DynamicallyLinked" case, I adjusted to make 3 classes: for Win,Mac and Linux. With library filenames as constants at start of file.
  3. [Edit: my bad, see comment below] downgraded from netstandard2.1 to 2.0. Because NET Framework only supports 2.0, but not 2.1. And I don't see real need for 2.1.

@digivod
Copy link
Contributor Author

digivod commented Aug 17, 2022

I only just realised, that netstandard2.0 instead of 2.1 is not as "no difference at all" as I thought, as mentioned in #217. I didn't see the #if NETSTANDARD2_1_OR_GREATER stuff. So i'd opt for multitargeting, too.

@Ruslan-B
Copy link
Owner

I got the idea - I see the implementation a bit in different way - as it better to split platforms to packages - as they don't cross to each other - package size is also important for some folks.
As for netstandard2.0 vs 2.1 You don't fell it as this macro works #if NETSTANDARD2_1_OR_GREATER [MarshalAs(UnmanagedType.LPTF8Str)]... technically not a big performance hit but none the less all string conversion will go though UTF8Marshaler as UnmanagedType.LPTF8Str is available only since netstandard2.1.
I already got feedback from active members - so I still have to support at least net4.5 (4.6.1 variations).

@digivod
Copy link
Contributor Author

digivod commented Aug 18, 2022

I see.
If dll size is an issue, then multitargeting seems the way to go: you end up having bigger .nupkg's because of multiple dlls, but the platform specific DLL itself is smaller. And only the one matching the user's platform is used. My choice would be to go for net48;net6.0;netstandard2.0 So you have proper versions for the current LTS platforms net6.0 and .NET Framework 4.8 and a nestandard2.0 fallback for the people stuck on older platforms. And net48 (more exactly: 472) already has the LPTF8Str Marshaller, so you can do

    #if NET
    [MarshalAs(UnmanagedType.LPUTF8Str)]
    #elif NET48
    [MarshalAs(UnmanagedType.LPUTF8Str)]
    #else
    [MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef = typeof(UTF8Marshaler))]
    #endif

This means all NET Framework 4.8 and NET6.0 users (my guess: 95%) get the best possible code.

Creating three DynamicBindings classes is an over the top, I agree. I'm just still in awe of the powers of code generation, so I tend to overuse it :-)

In the end, all options are fine with me: having the abstractions in a separate package did the trick. It allows for easy packaging the actual DLL related stuff into an own assembly however one sees fit. And a user needs some place to distribute the binaries anyhow, so an own assembly is required in any case.

Just the "const string libraryname" I'd like because it makes maintaining different versions of the bindings easier and the bsf.h integration is important to me, because I use it in production.
If I should update the pull request let me know.

@Ruslan-B
Copy link
Owner

I found this one:
https://docs.microsoft.com/en-us/dotnet/standard/native-interop/cross-platform
It says that on Linux or macOS and prepend and tail everything by some common rules - from my experience it even threats hyphens as dots on Linux and macOS. But again it used fail on CentOS like in 2017 🤣.
This article also refers to SetDllImportResolver but this one is available only for Core 3.0+.
Which means for modern environment you don't extra packages and you can survive with DllImport only.
Abstraction is still the way to go as for iOS ahead of time compilation and linking is being used.

I would think we can stop at the point were we already are.

I'll obviously add "avcodec/bsf.h" to generator.
The only other two things left - function overloads and indirections by ref from my list.
All this ideas came from here #187

I'll keep this PR open and look on changes once again.

P.S. I tested constants are not really necessary compiler creates a string table anyway - doesn't impact on library size.

@Ruslan-B
Copy link
Owner

Ruslan-B commented Aug 25, 2022

@digivod I added libavcodec/bsf.h and did some semantic renames - the rest seems to be sugar as no function overloads and only some inderections would be nice to have - in general it only hides what happens under the hood.
I still thinking as for DynamicallyLinkedBindings - I'll test on OS X how it works with hyphen to dot conversion - if it does no changes needed.
However, I hardly can imagine that some one will use DynamicallyLinkedBindings as it is cross platform pain to point the path to FFmpeg binaries.

P.S.
Starting to recall - hyphen to dot conversion - might be mono perk which was lost.

@digivod
Copy link
Contributor Author

digivod commented Aug 27, 2022

Ups, I synced my own branch to the ffmpeg_abstractions latest here and this automatically closed my pull request here. Will create a new pull request based on lastest changes...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants