Ruslan-B / FFmpeg.AutoGen

FFmpeg auto generated unsafe bindings for C#/.NET and Core (Linux, MacOS and Mono).
GNU Lesser General Public License v3.0
1.37k stars 322 forks source link

AccessViolationException calling avio_close #217

Open ispysoftware opened 2 years ago

ispysoftware commented 2 years ago

Just updated to the latest version and using the latest build from ffmpeg and my code is throwing AccessViolationException wherever i'm calling avio_close. Are you seeing the same thing? Not sure if this is a problem with ffmpeg or the wrapper

Ruslan-B commented 2 years ago

I don't see any difference in bindings for avio_close. The main difference between 5.0 and 5.1 is in macros part. Can you create a minimal test?

ispysoftware commented 2 years ago

thanks for getting back to me.

I found the issue, setting formatContext->pb->seekable = 1 breaks it now for some reason

           var outputFormat = av_guess_format("mp4", @"D:\test.mp4", null);
            var formatContext = avformat_alloc_context();
            formatContext->oformat = outputFormat;
            var videoStream = avformat_new_stream(formatContext, null);
            var videoCodec = avcodec_find_encoder(AVCodecID.AV_CODEC_ID_H264);
            var videoCodecContext = avcodec_alloc_context3(videoCodec);
            avcodec_open2(videoCodecContext, videoCodec, null);
            avio_open(&formatContext->pb, @"D:\test.mp4", AVIO_FLAG_WRITE);
            formatContext->pb->seekable = 1; //<-- comment out to fix
            avformat_write_header(formatContext, null);
            avio_close(formatContext->pb);  //<-- oopsie
ispysoftware commented 2 years ago

i logged a ticket with ffmpeg and they said it's working fine on their side so possibly some issue with the wrapper?

https://trac.ffmpeg.org/ticket/9871?cversion=1&cnum_hist=2

Ruslan-B commented 2 years ago

I see meanwhile can you try new packages https://github.com/Ruslan-B/FFmpeg.AutoGen/issues/210#issuecomment-1214409331?

DynamicallyLoaded is the closest to what we have now. StaticallyLinked - is mostly for iOS as ffmpeg is linked statically. DynamicallyLinked - is a DllImport import version.

In the very beginning you have to initialize the API then the rest is similar to what you have already: DynamicallyLoadedBindings.Initialize();

Ruslan-B commented 2 years ago

I'll test sniped you wrote.

ispysoftware commented 2 years ago

i can't test that update unfortunately as my project is .net standard 2.0 and i can't update that to 2.1 as that breaks compat with the 4.8 framework project that uses it.

Is there a reason you're using the 2.1 version of net standard? It limits the usability of the library quite a lot.

Ruslan-B commented 2 years ago

Well UnmanagedType.LPUTF8Str is in only in 2.1 standard - and this going to be newest version which supposed to support only net 6.0 as the rest is unsupported by MS any more. Custom marshaller - UTF8Marshaler can help here however in general I would prefer to drop anything lower than 2.1 standard.

ispysoftware commented 2 years ago

the full .net framework is still supported by MS

Support lifecycle .NET Framework 4.8 is the latest version of .NET Framework and will continue to be distributed with future releases of Windows. As long as it is installed on a supported version of Windows, .NET Framework 4.8 will continue to also be supported.

https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-framework

Ruslan-B commented 2 years ago

Okay I'll add it to targets then.

ispysoftware commented 2 years ago

Hey @Ruslan-B did you manage to repro the original issue?

Ruslan-B commented 2 years ago

Confirming same exception while using .NET 6.0.400 and GPL FFmpeg 5.1. Using new or old bindings. Update: It works for .NET 6.0.400 and GPL FFmpeg 4.4

Here is the log from 5.1

FFmpeg version info: 5.1-full_build-www.gyan.dev
[libx264 @ 00000148f57217c0] The encoder timebase is not set.
[file @ 00000148f5721bc0] Setting default whitelist 'file,crypto,data'
[mp4 @ 00000148f5720c80] Could not find tag for codec none in stream #0, codec not currently supported in container
[AVIOContext @ 00000148f5761dc0] Statistics: 0 bytes written, 0 seeks, 0 writeouts
Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at FFmpeg.AutoGen.Bindings.DynamicallyLoaded.DynamicallyLoadedBindings+<>c.<Initialize>b__6_583(FFmpeg.AutoGen.Abstractions.AVIOContext*)
   at FFmpeg.AutoGen.Abstractions.ffmpeg.avio_close(FFmpeg.AutoGen.Abstractions.AVIOContext*)
   at FFmpeg.AutoGen.Example.Program.Main(System.String[])

And working 4.4

FFmpeg version info: 4.4-full_build-www.gyan.dev
[libx264 @ 000001cdf5124200] The encoder timebase is not set.
[mp4 @ 000001cdf5122d00] Could not find tag for codec none in stream #0, codec not currently supported in container

It doesn't seems like bindings or marshaling - something different.

hglee commented 2 years ago

Hi,

I found different size of checksum in AVIOContext. So it has different offset after checksum.

https://github.com/Ruslan-B/FFmpeg.AutoGen/blob/5194d118f8e8f4e0f3ae83ffd311aacb2d0f1c66/FFmpeg/include/libavformat/avio.h#L246

https://github.com/Ruslan-B/FFmpeg.AutoGen/blob/d7f6e2309a46f0814067152ac1ce58aa84abe251/FFmpeg.AutoGen/FFmpeg.structs.g.cs#L2060

Ruslan-B commented 2 years ago

@hglee I didn't get could you please elaborate?

hglee commented 2 years ago

In FFmpeg.Autogen, size of checksum is 8 bytes (ulong = uint64).

But in native version, size of checksum is 4 bytes in common case (unsigned long = uint32).

So it has different offset in managed version and if you write seekable field in managed,

it corrupts memory around seekable field (maybe protocol_whitelist ?).

So it crashes in avio_close, actually, in av_freep (avio_close -> av_opt_free -> av_freep).

Ruslan-B commented 2 years ago

@hglee it is actually brilliant analysis - we have to re-test - this any way - but I am scratching my head - seems it has to be hinted to solve this - but how many this kind of thing we have at the moment- I think report back to FFmpeg - but again it has to be a collective claim as they wave it out otherwise.

Ruslan-B commented 2 years ago

To make it clear - it is Clang - without any standard so it will work like a magic as long you on platform which can compile it for years and then it breaks)))

Ruslan-B commented 2 years ago

I am not convinced - as double checked - as in 4.4 we have as well - it is possible to redefine ulong in Clang, but why you should - it is 64 bit world not 4 bytes - if it is - then is a bug in ffmpeg - it used to work in 4.4

        public ulong @checksum;
hglee commented 2 years ago

In 4.x, there was no critical fields around seekable in my guess.

https://github.com/Ruslan-B/FFmpeg.AutoGen/blob/e0d26f8a24e36b6cec25c703336eddb38a6d017e/FFmpeg/include/libavformat/avio.h#L260

And you need to consider size of unsigned long by platforms.

In .NET, ulong is always 8 bytes.

In Windows x86/x64 native, unsigned long is 4 bytes in common case.

But other platforms like Linux, OSX x86/x64 have different size model.

Ruslan-B commented 2 years ago

I know -but I am don't buy it as it would be a change for everting - the rest is working - I don't see it in headers - don't say it isn't the case - but what wise to do here disable structure alignment first...

FrostKiwi commented 1 year ago

offtopic I'm having a tough time debugging it, but certain C# packaging combinations have caused Windows Defender to trip, because FFmpeg.AutoGen works by calling into a .dll of unmanaged code, I guess. It depends on software layout, but I have Windows Defender sometimes refusing to give FFmpeg permission to write to the file it creates. It's bizzare and I'm not even sure what causes it. So if anyone sees Access Violations, this is one thing I encountered. /offtopic

SuRGeoNix commented 1 year ago

@Ruslan-B I can confirm (for Windows) that by changing the checksum from ulong to UInt32 (uint) resolves the issue (all the next attributes read properly). I remember even with v4 having an issue when I was using HLSContext private structure which had AVIOContext and it was causing an issue with my alignment!

I will do more testing tomorrow and come back (AVCodecContext does not have this issue which makes things more complicated my mistake that was not long it was uint64_t so it's normal that it does not have an issue)

This is critical and should probably update all long/unsigned long (from C) in structs with Int32/UInt32 (int/uint).

Update: I've a created a sample C lib and confirmed that the size of long and unsigned long is 4 bytes for Windows x64 (imported to c# x64 with a testing struct to confirm it)

SuRGeoNix commented 1 year ago

The good news is that only AVIOContext (and _GUID?) struct is affected. I used your generator to see from the included c headers which have types long / unsigned long : -

DEBUGLONG: AVIOContext unsigned long checksum
DEBUGLONG: _GUID unsigned long  Data1

The bad news should also check functions for in/out params with longs (wasn't able to find any)

Opened also a 'wish' ticket in FFmpeg trac

Possible solutions:

Ruslan-B commented 1 year ago

Seems I have free weekend so I'll try to retest this on linux and mac. Right now it is passible to workaround this issue using type punning just to shadow incorrect structure. Besides, as another option it is possible to create hidden flavored types to avoid loosing platform independency and do checks in runtime - cumbersome but API wise would be clean.

Ruslan-B commented 12 months ago

@iamcarbon need an assistance here as seems if I do exclusion to this structure all works well - from all test I maid it seems structure size is no different across intel platforms, cant verify it for arm based recent macs

Ruslan-B commented 12 months ago

it sounds funny but if have somewhere definition in preprocessor true = false I have to put more effort to to be handled

Ruslan-B commented 12 months ago

ghosting structures might play here if don't blow library size and do cross platform tricks. Do have definitive felling it has to be reported back to ffmpeg team

SuRGeoNix commented 4 months ago

Found a solution for this, tested and works as it should... https://github.com/Ruslan-B/FFmpeg.AutoGen/blob/master/FFmpeg.AutoGen.CppSharpUnsafeGenerator/Processing/TypeHelper.cs#L46

// https://github.com/dotnet/runtime/issues/13788
PrimitiveType.Long => "CLong",
PrimitiveType.ULong => "CULong",
Ruslan-B commented 4 months ago

Looks like I have to create platform specific versions as looked in this: https://github.com/dotnet/runtime/blob/5535e31a712343a63f5d7d796cd874e563e5ac14/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/CULong.cs

Boils down on windows C(U)Long is actually int.

And again we can forget about .net standard 2 it is dotnet 6 change.

May be forking this types from dot net core source code would be a solution.

SuRGeoNix commented 4 months ago

@Ruslan-B Found more issues (mainly for x86) with ptrdiff_t (which seems to be resolved with nint) / size_t data (which seems to be resolved with nuint). Tested with AVFrame that has crop variables as long in x86 it will have wrong values (eg duration field at latest master build).

Ruslan-B commented 4 months ago

great here we go again, means all longs are probably ints but only on windows. I'll check how to separate platforms in nuget - the silly things is it has to be per platform expect windows all the same.

Ruslan-B commented 4 months ago

none the less after 12 years we solving this "mystery"

SuRGeoNix commented 4 months ago

I don't think that you need to separate the nuget to platforms-OSes, using CULong / CLong and nuint / nint should be fine for both x86/x64 and for windows, linux, macos.

Ruslan-B commented 4 months ago

No wont work - given binaries as for .net has to have different structures. I'll look into this. Easy fix for windows - however, will break any *nix system. Will figure out. Besides can be solved with shadow structures. Maybe an option.

SuRGeoNix commented 4 months ago

@Ruslan-B I'm confused, you might be too. I create a NuGet package from Windows with AVIOContext struct (with CULong) and then use it from Ubuntu. They have different size when I run sizeof(AVIOContext) and they should work fine. So:-

#if TARGET_WINDOWS
using NativeType = System.UInt32;
#else
using NativeType = System.UIntPtr;
#endif

The confusing part for me is when the preprocessor directives will be calculated/executed. During Pack/Compile/Build of the package library, during the compilation of the parent project or during runtime. I'm still not sure to be honest but my testing shows that is not happening during the packing of the initial library which means that you don't need to create multiple platforms-os packages.

Update: Based on above thoughts, I even think that there is no reason to check OS at runtime. For example for EAGAIN https://github.com/Ruslan-B/FFmpeg.AutoGen/blob/e129a3128b570e26f4c934cecbb9c00bab3c5dc4/FFmpeg.AutoGen/FFmpeg.cs#L17

This could be define as preprocessor directive https://blog.magnusmontin.net/2018/11/05/platform-conditional-compilation-in-net-core/

This is how donet actually does that for TARGET_WINDOWS https://github.com/dotnet/runtime/blob/703df3b3d7ef6c92ffdf3e9221d82539a9dd5fce/src/libraries/System.Net.Security/src/System.Net.Security.csproj#L16

So, my understanding is that all those will be calculated during the final binary which it will be OS specific (you cannot have the same binary for windows/linux/macos). This might not true only for AnyCpu but again we don't need to worry about this as far as we use the proper types (eg. nint).

Ruslan-B commented 4 months ago

I guess I will create another package which target windows OS. we already have an abstraction, kind of clutch but transparent.