drewnoakes / metadata-extractor-dotnet

Extracts Exif, IPTC, XMP, ICC and other metadata from image, video and audio files
Other
915 stars 158 forks source link

AOT analyser warnings in the Tiff reader code #394

Closed Numpsy closed 5 months ago

Numpsy commented 5 months ago

Hi, I'm not sure if there is a real issue here, but in case.

I had a go at building an application using MetadataExtractor as NativeAOT (in .NET8), and got this analyzer warning in the results:

MetadataExtractor.Formats.Exif.ExifTiffHandler.ProcessGeoTiff(UInt16[],ExifIfd0Directory): Using member 'System.Array.CreateInstance(Type,Int32)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available.

I assume the warning is coming from https://github.com/drewnoakes/metadata-extractor-dotnet/blob/af0aa357b6efd865d08776a4ef668841ddf63a99/MetadataExtractor/Formats/Exif/ExifTiffHandler.cs#L451 and as that seems to be creating an array of a type that matches an existing array I guess that The code for an array of the specified type would be available, but i'm not certain of that.

drewnoakes commented 5 months ago

I agree that the array type in question is almost certainly going to be available. Still it'd be nice to avoid this warning.

Is there a diagnostic ID that we can use to suppress the warning in code?

I'd like to try NativeAOT on the library one day myself.

Numpsy commented 5 months ago

Is there a diagnostic ID that we can use to suppress the warning in code?

I had a go at building MetadataExtractor itself as .NET 8, with the AOT analyzer enabled, and got two IL3050 warnings -

1) The previously mentioned

IL3050  Using member 'System.Array.CreateInstance(Type, Int32)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available.    MetadataExtractor (net8.0)  S:\Github\metadata-extractor-dotnet\MetadataExtractor\Formats\Exif\ExifTiffHandler.cs   451 

But also this one

IL3050  Using member 'System.Enum.GetValues(Type)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. It might not be possible to create an array of the enum type at runtime. Use the GetValues<TEnum> overload or the GetValuesAsUnderlyingType method instead. MetadataExtractor (net8.0)  S:\Github\metadata-extractor-dotnet\MetadataExtractor\Formats\Jpeg\JpegSegmentType.cs   224 

from https://github.com/drewnoakes/metadata-extractor-dotnet/blob/a95e8f345781affa61db26677d69dbe0e3357d49/MetadataExtractor/Formats/Jpeg/JpegSegmentType.cs#L224

For the second one a .NET 6 + build might be able to do

public static IReadOnlyList<JpegSegmentType> CanContainMetadataTypes { get; } = Enum.GetValues<JpegSegmentType>().Where(type => type.CanContainMetadata()).ToList();

But for the array one it might be a case of either proving it's not a problem and annotating it with UnconditionalSuppressMessage or anotating it with RequiresDynamicCodeAttribute (which might be rather viral), or some other approach (I'm learning this stuff myself, so I don't know the best approach in all cases).

Numpsy commented 5 months ago

I'd like to try NativeAOT on the library one day myself.

Maybe it'd be possible to do an AOT build of the regression tests and run it over all the test images to see what happens? (I haven't got as far as trying anything like that yet)

drewnoakes commented 5 months ago

I think that's a very good idea.

Numpsy commented 5 months ago

Ok, I tried running the regresision test app as AOT and it seemed to complete ok, though when i run it as .NET 8.0 I do get a significant amount of differences in the results that look like small differences in floating point numbers, which make it hard to review all the differences - e.g.

image

Numpsy commented 5 months ago

Ok, I tried running the regresision test app as AOT and it seemed to complete ok,

Though saying that, when I try debugging the test app, it looks like all the GeoTiff simple images go through the StringValue path, not the Array path where the warning is

Numpsy commented 5 months ago

I do get a significant amount of differences in the results that look like small differences in floating point numbers,

Maybe down to https://devblogs.microsoft.com/dotnet/floating-point-parsing-and-formatting-improvements-in-net-core-3-0/#making-the-formatter-ieee-754-2008-compliant ? I don't know how the results being different in different .NET versions is supposed to be handled

drewnoakes commented 5 months ago

I think you're right and that the floating point errors are misleading.

Currently we produce metadata text files for Java and .NET, then produce a diff between them.

However there are potentially many forms for both the Java and .NET libraries, depending upon the framework/runtime versions used. The text files in the repo are currently created using net48. I'd like coverage of net8.0 (and other Java versions too) but haven't worked out what to do with the diff files in such cases.

For your scenario I'd suggest generating data using net8.0 without your code changes, and staging/committing that in order to get a clean baseline. Then, you can introduce your changes and re-run the generator to see the diff specific to your actual changes. I have to do this from time to time, and I should document it on the wiki page that discusses using the test suite.

drewnoakes commented 5 months ago

https://github.com/drewnoakes/metadata-extractor/wiki/Working-with-test-images#java--net-runtime-versions

drewnoakes commented 5 months ago

@Numpsy please take a look at https://github.com/drewnoakes/metadata-extractor-dotnet/pull/397

Numpsy commented 5 months ago

Nice, thanks.