Race-condition when writing 32-bit png images with text in parallel.

jnyrup commented 1 year ago

Prerequisites

[X] I have written a descriptive issue title
[X] I have verified that I am running the latest version of ImageSharp
[X] I have verified if the problem exist in both DEBUG and RELEASE mode
[X] I have searched open and closed issues to ensure it has not already been reported

ImageSharp version

3.0.0-alpha.0.73

Other ImageSharp packages and versions

SixLabors.ImageSharp.Drawing: 1.0.0-beta15.3, SixLabors.Fonts: 1.0.0-beta19.5

Environment (Operating system, version and so on)

Windows 10, 22H2, Build 19045.2486

.NET Framework version

.NET 7

Description

In unit test I experienced that when running test locally, the text in images would sometimes move a tiny bit, but only when running the tests in parallel, which made me think there was a race-condition somewhere.

Below I have created the smallest reproducing example I could come up with and commented with what changes removes what looks like a race-condition to me.

To repeat to conditions here:

Running in parallel
Using Rgba32
Writing some text with a space and some character, e.g. " A".
Saving to gif, tga, or png.

Steps to Reproduce

using SixLabors.Fonts;
using SixLabors.ImageSharp;
using SixLabors.ImageSharp.Drawing.Processing;
using SixLabors.ImageSharp.PixelFormats;
using SixLabors.ImageSharp.Processing;
using System;
using System.IO;
using System.Linq;
using System.Threading.Tasks;

// Works for: single thread
// Fails for: two threads
const int NumberOfThreads = 2;

Parallel.ForEach(Enumerable.Range(0, NumberOfThreads), _ =>
{
    var imageLength0 = Generate();
    var imageLength1 = Generate();

    if (imageLength0 != imageLength1)
        throw new InvalidOperationException($"Expected {imageLength0} to equal {imageLength1}");
});

static long Generate()
{
    using var memoryStream = new MemoryStream();

    // Works for: Rgb24
    // Fails for: Rgba32
    using var image = new Image<Rgba32>(30, 15);

    // Works for: " ", "  ", "A" or "AA"
    // Fails for: " A", "A "
    const string Text = " A";
    image.Mutate(x => x.DrawText(Text, SystemFonts.CreateFont("Arial", 15), Color.Black, new PointF(0, 0)));

    // Works for bmp, jpeg
    // Fails for png, tga, gif
    image.SaveAsPng(memoryStream);
    return memoryStream.Length;
}

<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net7.0</TargetFramework>
    </PropertyGroup>
    <ItemGroup>
        <PackageReference Include="SixLabors.ImageSharp" Version="3.0.0-alpha.0.73" />
        <PackageReference Include="SixLabors.ImageSharp.Drawing" Version="1.0.0-beta15.3" />
        <PackageReference Include="SixLabors.Fonts" Version="1.0.0-beta19.5" />
    </ItemGroup>
</Project>

Images

No response

JimBobSquarePants commented 1 year ago

Hi @jnyrup Can you please push some of the example output? Thanks!

I've moved the issue to the ImageSharp.Drawing repository as the behavior you are seeing will originate there.

jnyrup commented 1 year ago

Sure! See the difference in the amount of vertical space above the text.

thread_1_image0 thread_1_image1

JimBobSquarePants commented 1 year ago

Thanks!

jnyrup commented 1 year ago

It seems the problem might have been introduced or exposed in SixLabors.Fonts 1.0.0-beta17.

Using SixLabors.ImageSharp.Drawing 1.0.0-beta14 works. Upgrading only SixLabors.Fonts from 1.0.0-beta16 to 1.0.0-beta17 fails.

jnyrup commented 1 year ago

Bisected the commit to https://github.com/SixLabors/Fonts/commit/39773e680338b229cc285b52a3ba97ef9bf63182

JimBobSquarePants commented 1 year ago

Nice bit of detective work!

Unfortunately, I've given the commit a few reads over now, and I cannot spot what the introduced error would be. Something to do with the calculation of an ascender (likely within Textline/Textbox) but I'm not sure where.

jnyrup commented 1 year ago

I began with the previous commit and started adding parts of the failing commit until it failed.

Adding these few lines was enough.

if (metric.TopSideBearing < 0)
{
    // Adjust for glyphs with a negative tsb. e.g. emoji.
    ascender -= metric.TopSideBearing * scaleY;
}

https://github.com/SixLabors/Fonts/commit/39773e680338b229cc285b52a3ba97ef9bf63182#diff-d4b7f4ba1427f72f4e4ef98a983bee5e58250bc7260f41922b8647f3ff6514e3R878-R883

jnyrup commented 1 year ago

Some more findings.

Removing ascender -= metric.TopSideBearing * scaleY; from https://github.com/SixLabors/Fonts/commit/39773e680338b229cc285b52a3ba97ef9bf63182 fixes my problem and all SixLabor.Fonts test at that commit pass.

Similarly removing ascender -= tsbOffset * scaleY; from current main fixes my problem and all SixLabor.Fonts test at that commit pass.

JimBobSquarePants commented 1 year ago

Great work!

Test coverage isn't great there unfortunately. That line only exists because the Segoe UI emoji font has a negative TSB for some bizarre reason. I'll have a dig and try to figure out what reference is shared across threads.

jnyrup commented 1 year ago

I tried a new approach: wrapping code in a lock and gradually move the lock inwards.

If I in StreamFontMetrics.GetGlyphMetrics add a lock I no longer get an exception.

lock (this.glyphCache)
{
    // We overwrite the cache entry for this type should the attributes change.
    GlyphMetrics[]? cached = this.glyphCache[glyphId];
    if (cached is null)
    {
        this.glyphCache[glyphId] = new[]
        {
            this.CreateGlyphMetrics(
            codePoint,
            glyphId,
            glyphType)
        };
    }
}

Stack trace

SixLabors.Fonts.dll!SixLabors.Fonts.StreamFontMetrics.GetGlyphMetrics(SixLabors.Fonts.Unicode.CodePoint codePoint, ushort glyphId, SixLabors.Fonts.ColorFontSupport support)
SixLabors.Fonts.dll!SixLabors.Fonts.FileFontMetrics.GetGlyphMetrics(SixLabors.Fonts.Unicode.CodePoint codePoint, ushort glyphId, SixLabors.Fonts.ColorFontSupport support)
SixLabors.Fonts.dll!SixLabors.Fonts.GlyphPositioningCollection.TryAdd(SixLabors.Fonts.Font font, SixLabors.Fonts.GlyphSubstitutionCollection collection)
SixLabors.Fonts.dll!SixLabors.Fonts.TextLayout.DoFontRun(System.ReadOnlySpan<char> text, int start, System.Collections.Generic.IReadOnlyList<SixLabors.Fonts.TextRun> textRuns, ref int textRunIndex, ref int codePointIndex, ref int bidiRunIndex, bool isFallbackRun, SixLabors.Fonts.Font font, SixLabors.Fonts.Unicode.BidiRun[] bidiRuns, System.Collections.Generic.Dictionary<int, int> bidiMap, SixLabors.Fonts.GlyphSubstitutionCollection substitutions, SixLabors.Fonts.GlyphPositioningCollection positionings)
SixLabors.Fonts.dll!SixLabors.Fonts.TextLayout.ProcessText(System.ReadOnlySpan<char> text, SixLabors.Fonts.TextOptions options)
SixLabors.Fonts.dll!SixLabors.Fonts.TextLayout.GenerateLayout(System.ReadOnlySpan<char> text, SixLabors.Fonts.TextOptions options)
SixLabors.Fonts.dll!SixLabors.Fonts.TextRenderer.RenderText(System.ReadOnlySpan<char> text, SixLabors.Fonts.TextOptions options)
SixLabors.Fonts.dll!SixLabors.Fonts.TextRenderer.RenderText(string text, SixLabors.Fonts.TextOptions options)
SixLabors.ImageSharp.Drawing.dll!SixLabors.ImageSharp.Drawing.Processing.Processors.Text.DrawTextProcessor<SixLabors.ImageSharp.PixelFormats.Rgba32>.BeforeImageApply()
SixLabors.ImageSharp.dll!SixLabors.ImageSharp.Processing.Processors.ImageProcessor<SixLabors.ImageSharp.PixelFormats.Rgba32>.SixLabors.ImageSharp.Processing.Processors.IImageProcessor<SixLabors.ImageSharp.PixelFormats.Rgba32>.Execute() Unknown
SixLabors.ImageSharp.dll!SixLabors.ImageSharp.Processing.DefaultImageProcessorContext<SixLabors.ImageSharp.PixelFormats.Rgba32>.ApplyProcessor(SixLabors.ImageSharp.Processing.Processors.IImageProcessor processor, SixLabors.ImageSharp.Rectangle rectangle)  Unknown
SixLabors.ImageSharp.dll!SixLabors.ImageSharp.Processing.DefaultImageProcessorContext<SixLabors.ImageSharp.PixelFormats.Rgba32>.ApplyProcessor(SixLabors.ImageSharp.Processing.Processors.IImageProcessor processor)    Unknown
SixLabors.ImageSharp.Drawing.dll!SixLabors.ImageSharp.Drawing.Processing.DrawTextExtensions.DrawText(SixLabors.ImageSharp.Processing.IImageProcessingContext source, SixLabors.ImageSharp.Drawing.Processing.DrawingOptions drawingOptions, string text, SixLabors.Fonts.Font font, SixLabors.ImageSharp.Drawing.Processing.IBrush brush, SixLabors.ImageSharp.Drawing.Processing.IPen pen, SixLabors.ImageSharp.PointF location)
SixLabors.ImageSharp.Drawing.dll!SixLabors.ImageSharp.Drawing.Processing.DrawTextExtensions.DrawText(SixLabors.ImageSharp.Processing.IImageProcessingContext source, SixLabors.ImageSharp.Drawing.Processing.DrawingOptions drawingOptions, string text, SixLabors.Fonts.Font font, SixLabors.ImageSharp.Color color, SixLabors.ImageSharp.PointF location)
SixLabors.ImageSharp.Drawing.dll!SixLabors.ImageSharp.Drawing.Processing.DrawTextExtensions.DrawText(SixLabors.ImageSharp.Processing.IImageProcessingContext source, string text, SixLabors.Fonts.Font font, SixLabors.ImageSharp.Color color, SixLabors.ImageSharp.PointF location)
ConsoleApp4.dll!Program.<Main>$.AnonymousMethod__0_2(SixLabors.ImageSharp.Processing.IImageProcessingContext x)

JimBobSquarePants commented 1 year ago

Hmmm. That feels like a sticking plaster.

The worst thing that should happen is that there is a cache miss and that means that the metrics are parsed 2X. We clone the metrics in GlyphPositioningCollection.TryAdd and GlyphMetrics.TopSideBearing is read-only. I wonder if the issue is replicable with a debugger attached? Then we can see why that property is different.

JimBobSquarePants commented 1 year ago

I guess we could use a concurrent dictionary as the cache and put a note saying that it requires further investigation.

jnyrup commented 1 year ago

I wonder if the issue is replicable with a debugger attached? Then we can see why that property is different.

It is.

Also by invoking Generate() a single time to fill caches before the parallel loop, it also works.

_ = Generate();
Parallel.ForEach(Enumerable.Range(...)

JimBobSquarePants commented 1 year ago

Ah no, sorry I mean in the Fonts library itself.

I've managed to recreate the issue using the following test so should have enough to go on now.

[Fact]
public void RendererIsThreadsafe()
{
    const int threadCount = 10;
    Parallel.For(0, threadCount, _ =>
    {
        ColorGlyphRenderer renderer1 = new();
        TextRenderer.RenderTextTo(renderer1, "A ", new TextOptions(SystemFonts.CreateFont("Arial", 15)));

        ColorGlyphRenderer renderer2 = new();
        TextRenderer.RenderTextTo(renderer2, "A ", new TextOptions(SystemFonts.CreateFont("Arial", 15)));

        Assert.True(renderer1.ControlPoints.Count > 0);
        Assert.True(renderer2.ControlPoints.Count > 0);
        Assert.True(renderer1.ControlPoints.SequenceEqual(renderer2.ControlPoints));
    });
}

SixLabors / Fonts