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

AccessViolationException calling avio_close #217

Open
ispysoftware opened this issue Aug 10, 2022 · 27 comments
Open

AccessViolationException calling avio_close #217

ispysoftware opened this issue Aug 10, 2022 · 27 comments

Comments

@ispysoftware
Copy link
Sponsor

ispysoftware commented Aug 10, 2022

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
Copy link
Owner

Ruslan-B commented Aug 10, 2022

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
Copy link
Sponsor Author

ispysoftware commented Aug 10, 2022

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
Copy link
Sponsor Author

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

@ispysoftware ispysoftware reopened this Aug 15, 2022
@Ruslan-B
Copy link
Owner

Ruslan-B commented Aug 15, 2022

I see meanwhile can you try new packages #210 (comment)?

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
Copy link
Owner

Ruslan-B commented Aug 15, 2022

I'll test sniped you wrote.

@ispysoftware
Copy link
Sponsor Author

ispysoftware commented Aug 15, 2022

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
Copy link
Owner

Ruslan-B commented Aug 15, 2022

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
Copy link
Sponsor Author

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
Copy link
Owner

Okay I'll add it to targets then.

@ispysoftware
Copy link
Sponsor Author

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

@Ruslan-B
Copy link
Owner

Ruslan-B commented Aug 25, 2022

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
Copy link

hglee commented Aug 31, 2022

Hi,

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

unsigned long checksum;

public ulong @checksum;

@Ruslan-B
Copy link
Owner

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

@hglee
Copy link

hglee commented Aug 31, 2022

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
Copy link
Owner

@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
Copy link
Owner

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
Copy link
Owner

Ruslan-B commented Aug 31, 2022

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
Copy link

hglee commented Aug 31, 2022

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

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
Copy link
Owner

Ruslan-B commented Sep 1, 2022

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
Copy link

FrostKiwi commented May 16, 2023

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
Copy link

SuRGeoNix commented May 18, 2023

@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
Copy link

SuRGeoNix commented May 19, 2023

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:

  • CppSharp will fix the issue (if they have any, might is just config) and FFmpeg.AutoGen will have different packages for each platform (so we loose the platform independency)
  • FFmpeg replaces (the few) longs from public API
  • FFmpeg.Autogen hardcodes this (int for windows, intptr for linux, long for mac) (so we loose the platform independency)

@Ruslan-B
Copy link
Owner

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 Ruslan-B added the bug label Nov 17, 2023
@Ruslan-B
Copy link
Owner

Ruslan-B commented Dec 1, 2023

@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
Copy link
Owner

Ruslan-B commented Dec 1, 2023

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

@Ruslan-B
Copy link
Owner

Ruslan-B commented Dec 1, 2023

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
Copy link

SuRGeoNix commented Jul 2, 2024

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",

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

No branches or pull requests

5 participants