Fixes Library Crash When Using Release Mode

SkyeHoefling commented 3 years ago

Fixes: #3

Description

Fixed native C++ library crash when writing file under release mode by letting C#/.NET manage writing the image buffer.

Set FileOnQ.Imaging.Heif.Encoders to build under Debug or Release mode depending on build configuration of FileOnQ.Imaging.Heif project
FileOnQ.Imaging.Heif.Encoders builds every time the project build. This will prevent C++ changes from being cached. You no longer need to manually delete the binary

Merge Checklist

[x] Added unit or integration tests (if not explain)
[x] Benchmarks are equivalent or faster
No new unit tests because everything is covered by existing tests

github-actions[bot] commented 3 years ago

Benchmark Comparison

Benchmarking comparison between this Pull Request and the comitted values at benchmarks/results

thumbnail

 No differences found between the benchmark results with threshold 10%.

primary

 summary:
better: 1, geomean: 1.188
total diff: 1

No Slower results for the provided threshold = 10% and noise filter = 0.3ns.

| Faster                                                          | base/diff | Base Median (ns) | Diff Median (ns) | Modality|
| --------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| FileOnQ.Imaging.Heif.Benchmarks.PrimaryImage.PrimaryImage_Write |      1.19 |    2947325900.00 |    2481798700.00 |         |

No file given

Benchmark Results

thumbnail

``` ini BenchmarkDotNet=v0.13.0, OS=Windows 10.0.17763.2114 (1809/October2018Update/Redstone5) Intel Xeon Platinum 8171M CPU 2.60GHz, 1 CPU, 2 logical and 2 physical cores .NET SDK=5.0.400 [Host] : .NET 5.0.9 (5.0.921.35908), X64 RyuJIT Job-DNWESO : .NET 5.0.9 (5.0.921.35908), X64 RyuJIT Runtime=.NET 5.0 InvocationCount=1 LaunchCount=1 UnrollFactor=1 ``` | Method | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated | Allocated native memory | Native memory leak | |---------------- |---------:|---------:|---------:|------:|------:|------:|----------:|------------------------:|-------------------:| | Thumbnail_Write | 59.70 ms | 1.161 ms | 1.382 ms | - | - | - | 280 B | 5,186,889 B | 426,593 B |

primary

``` ini BenchmarkDotNet=v0.13.0, OS=Windows 10.0.17763.2114 (1809/October2018Update/Redstone5) Intel Xeon Platinum 8272CL CPU 2.60GHz, 1 CPU, 2 logical and 2 physical cores .NET SDK=5.0.400 [Host] : .NET 5.0.9 (5.0.921.35908), X64 RyuJIT Job-BSSTVK : .NET 5.0.9 (5.0.921.35908), X64 RyuJIT Runtime=.NET 5.0 InvocationCount=1 LaunchCount=1 UnrollFactor=1 ``` | Method | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated | Allocated native memory | Native memory leak | |------------------- |--------:|---------:|---------:|------:|------:|------:|----------:|------------------------:|-------------------:| | PrimaryImage_Write | 2.481 s | 0.0075 s | 0.0070 s | - | - | - | 248 B | 214,220,740 B | 20,145,761 B |

SkyeHoefling commented 3 years ago

This PR increases the existing memory leak for both primary image and thumbnails

PrimaryImage - Adds 2,00,000 B in leaked native memory
Thumbnail - Adds 200,000 B in leaked native memory

Prior to merging this, I would like to ensure any additional pointers are properly freed

Data below is copied from the last PR #23

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated	Allocated native memory	Native memory leak
PrimaryImage_Write	2.537 s	0.0060 s	0.0056 s	-	-	-	416 B	210,038,898 B	18,048,557 B
Thumbnail_Write	50.86 ms	0.571 ms	0.534 ms	-	-	-	112 B	4,937,343 B	295,481 B

SkyeHoefling commented 3 years ago

/benchmark

SkyeHoefling commented 3 years ago

The benchmark failed because the application was crashing sometimes when freeing corrupted native pointers. The latest code push calculates out the image width and height and then malloc the pointer size. In my initial tests it appears we are now able to deallocate the memory used by the memory buffer

int width = heif_image_get_width(image, heif_channel_Y);
int height = heif_image_get_height(image, heif_channel_Y);

// malloc the size of the pointer so it can be released
*data = (unsigned char*)malloc(width * height);
*data_size = width * height;

jpeg_mem_dest(&cinfo, data, data_size);

Benchmark results from my laptop

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.19043.1165 (21H1/May2021Update)
Intel Core i7-8650U CPU 1.90GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET SDK=5.0.102
  [Host]     : .NET 5.0.2 (5.0.220.61120), X64 RyuJIT
  Job-NJDGXJ : .NET 5.0.2 (5.0.220.61120), X64 RyuJIT

Runtime=.NET 5.0  InvocationCount=1  LaunchCount=1
UnrollFactor=1

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated	Allocated native memory	Native memory leak
OriginaBenchmark_PR	59.70 ms	1.161 ms	1.382 ms	-	-	-	280 B	5,186,889 B	426,593 B
Thumbnail_Write_Last_Merge	50.86 ms	0.571 ms	0.534 ms	-	-	-	112 B	4,937,343 B	295,481 B
Thumbnail_Write	59.88 ms	2.060 ms	5.877 ms	-	-	-	280 B	5,124,743 B	295,469 B

Consider the last merge wasn't using memory buffers, the current native memory leak number appears to be within the margin of error.

SkyeHoefling commented 3 years ago

/benchmark

github-actions[bot] commented 3 years ago

Benchmark Comparison

Benchmarking comparison between this Pull Request and the comitted values at benchmarks/results

thumbnail

 No differences found between the benchmark results with threshold 10%.

primary

 No differences found between the benchmark results with threshold 10%.

Benchmark Results

thumbnail

``` ini BenchmarkDotNet=v0.13.0, OS=Windows 10.0.17763.2114 (1809/October2018Update/Redstone5) Intel Xeon Platinum 8171M CPU 2.60GHz, 1 CPU, 2 logical and 2 physical cores .NET SDK=5.0.400 [Host] : .NET 5.0.9 (5.0.921.35908), X64 RyuJIT Job-LYXNTS : .NET 5.0.9 (5.0.921.35908), X64 RyuJIT Runtime=.NET 5.0 InvocationCount=1 LaunchCount=1 UnrollFactor=1 ``` | Method | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated | Allocated native memory | Native memory leak | |---------------- |---------:|---------:|---------:|------:|------:|------:|----------:|------------------------:|-------------------:| | Thumbnail_Write | 57.84 ms | 1.152 ms | 1.652 ms | - | - | - | 280 B | 5,124,881 B | 295,469 B |

primary

``` ini BenchmarkDotNet=v0.13.0, OS=Windows 10.0.17763.2114 (1809/October2018Update/Redstone5) Intel Xeon Platinum 8171M CPU 2.60GHz, 1 CPU, 2 logical and 2 physical cores .NET SDK=5.0.400 [Host] : .NET 5.0.9 (5.0.921.35908), X64 RyuJIT Job-MDNOWK : .NET 5.0.9 (5.0.921.35908), X64 RyuJIT Runtime=.NET 5.0 InvocationCount=1 LaunchCount=1 UnrollFactor=1 ``` | Method | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated | Allocated native memory | Native memory leak | |------------------- |--------:|---------:|---------:|------:|------:|------:|----------:|------------------------:|-------------------:| | PrimaryImage_Write | 2.996 s | 0.0111 s | 0.0104 s | - | - | - | 296 B | 222,030,204 B | 18,048,557 B |

SkyeHoefling commented 3 years ago

I Just looked through all the changes again and this is good to merge. 🛳🛳

FileOnQ / Imaging.Heif