Add optimised drawing path with cached glyph rendering #614
Conversation
|
Ah you know this already though, didn't spot the unticked checkbox. |
Codecov Report
@@ Coverage Diff @@
## master #614 +/- ##
==========================================
+ Coverage 88.55% 88.56% +0.01%
==========================================
Files 881 883 +2
Lines 36994 37114 +120
Branches 2627 2665 +38
==========================================
+ Hits 32759 32869 +110
- Misses 3452 3456 +4
- Partials 783 789 +6
Continue to review full report at Codecov.
|
|
@tocsoft I think what you need is to increase the tolerance a bit for the tests still failing. There are floating point implementation differences, leading to super-minor differences in the output. |
|
when outlining large numbers of characters we now even beat system.drawing 😃 |
| /// <summary> | ||
| /// Gets or sets a value indicating the DPI to render text along the X axis. | ||
| /// </summary> | ||
| public float DpiX { get => this.dpiX ?? DefaultTextDpi; set => this.dpiX = value; } |
There was a problem hiding this comment.
I snuck this one in... this will allow users to render at DPIs other that 72.
tocsoft
changed the title
…abors/ImageSharp into sw/draw-text-optermizations
| /// </summary> | ||
| public IBrush<TPixel> Brush { get; } | ||
| public IBrush<TPixel> Brush { get; set; } |
There was a problem hiding this comment.
As far as I understand, in most cases image processor properties are readonly and initialized from the constructors. Why are there exceptions, and are we sure we need them?
I think we need a guideline for this.
Hint:
Readonly properties will allow input check at constructor time + readonly logic is more in sync with the "one-shot" nature of our processors.
There was a problem hiding this comment.
Processors should be immutable. I've already gone through and changed all others before.
There was a problem hiding this comment.
ok.. i'll make them immutable again... was trying to find ways of reducing allocations by allow users to reuse as required, but its not a major gain really
There was a problem hiding this comment.
Most of the processors were not designed to be reusable, they cache stuff of their current execution. Making them reusable would mean that we need to implement and unit test proper reset logic for them, which is a statefulness hell IMO.
| TPixel colorOutline = NamedColors<TPixel>.Black; | ||
| TPixel colorFill = NamedColors<TPixel>.Gray; | ||
|
|
||
| provider.VerifyOperation( |
There was a problem hiding this comment.
This is the expected output right?
Don't we need another test? More complex path (eg. circle) but simpler brush/pen. I'm happy to add it.
| } | ||
| } | ||
|
|
||
| if (this.Pen != null) |
There was a problem hiding this comment.
What if Brush and Pen are both null? Shouldn't we throw?
(Same for all text processors.)
| this.builder.StartFigure(); | ||
| } | ||
|
|
||
| public bool BeginGlyph(RectangleF bounds, int cacheKey) |
There was a problem hiding this comment.
Don't we need a dedicated GlyphParameters struct/class with proper equals + gethashcode implementations instead of an integer key here? Collisions may lead to bugs!
There was a problem hiding this comment.
your right... i'll add it to Fonts get it wired up.
| TPixel color = NamedColors<TPixel>.Black; | ||
|
|
||
| provider.VerifyOperation( | ||
| ImageComparer.Tolerant(imageThreshold: 0.1f, perPixelManhattanThreshold: 20), |
There was a problem hiding this comment.
I think this threshold is too high now, will do some experiments lowering it.
| { | ||
| int w = (int)(img.Width * 0.6); | ||
| int h = (int)(img.Height * 0.6); | ||
| IPath path = new EllipsePolygon(img.Width/2, img.Height/2, w, h); |
There was a problem hiding this comment.
@tocsoft can you confirm, if the output is correct?
The first point in path is X=120, Y=300, but the text starts on the top, so I'm a bit confused. Is it center aligned?
There was a problem hiding this comment.
I've been thinking about this and think we should just remove this set of APIs... its just a thin wrapper around TextBuilder.GenerateGlyphs from Shapes and you can already draw shapes... it would also encourage users to investigate the shapes apis and also allow us to drop the SixLabors.Shapes.Text package and switch down to just Fonts & Shapes directly, then is users want to do this we can direct them to the optional package that they can use.
There was a problem hiding this comment.
it doesn't harm that its one less API to make sure is displaying correctly before we launch too.
There was a problem hiding this comment.
You mean the .DrawText() overloads taking IPath? That might be fine, it's an advanced scenario.
Having these image-based tests for TextBuilder.GenerateGlyphs(text, path, style) here might be still useful!
There was a problem hiding this comment.
The TextBuilder.GenerateGlyphs are really tests that belong in the Shapes project and not really Imagesharp concerns... as far and Imagesharp is concerned they will be paths and that it renders paths correctly.
|
I've dropped the |
|
@tocsoft I pushed a few refactors, if you're fine with them, I'm happy to merge this. |
|
Btw. defining a library-wide cache infrastructure is a good idea! (Not easy though.) I discourage mixing caching concern with the allocator concern however. A short proposal: public interface ICache
{
bool TryLookup<TKey, TResult>(TKey key, out TResult);
void Clear();
}
public class Configuration
{
// ...
public ICache Cache { get; set; } = new NullCache(); // no caching by default?
// ...
}
public interface IImageProcessingContext<TPixel>
{
// ...
public ICache Cache { get; set; } = new DefaultCache();
// ...
} |
|
|
…ations Add optimised drawing path with cached glyph rendering
TODO
Description
I've added a much more optimised text drawing path which makes use of caching individual glyphs the first time they are rendered. This has 2 benefits;
The cache could also be expanded in the future to live for a variety of extended life times (instead of the per drawing operation its using now) i.e. we could cache the glyphs for the life time of the
ImageProcessingOperationor allow for a user to pass into the mutate/clone method some form of lifetime scope that can be used to manage the lifetime of the cache. Or could this be an operation onMemoryManagereven??This drastically reduce rendering time on large amount of text, with larger gains the large the corpus. In addition we also drastically reduce our allocations (we will only now allocate a ComplexPath once per unique glyph now as apposed to before we would allocate one per visible character in the text).
These are the numbers, in each case we are rendering a word wrapped piece of text with "Hello World" rendered "TextIterations" number of times as a single space separated string.
Before and after changes to visual output
Before
After
Issues fixed
fixes #421