Skip to content

Buffered Decoding. #1269

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.

Sign up for GitHub

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

Merged
JimBobSquarePants merged 20 commits into from
Jul 22, 2020
Merged

Buffered Decoding. #1269

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
545cf7d
BufferedReadStream WIP - MagickImage not working.
JimBobSquarePants Apr 17, 2020
bbf85c1
Update BufferedReadStream.cs
JimBobSquarePants Apr 17, 2020
f8a88e4
Remove casting, skip mocks
JimBobSquarePants Apr 17, 2020
5ca6063
Cleanup and refactor
JimBobSquarePants Apr 18, 2020
219a4fe
Merge branch 'master' into js/buffered-read
JimBobSquarePants Apr 18, 2020
48e1e8d
Merge branch 'master' into js/buffered-read
JimBobSquarePants Apr 23, 2020
94a3ea5
Merge branch 'master' into js/buffered-read
JimBobSquarePants Apr 24, 2020
889dfd5
Merge branch 'master' into js/buffered-read
JimBobSquarePants Jul 10, 2020
529d120
Handle slow streams. Fix #1268
JimBobSquarePants Jul 10, 2020
96ec070
Restore missing Guard
JimBobSquarePants Jul 10, 2020
badae1c
More efficient reading
JimBobSquarePants Jul 10, 2020
a546676
Re-enable skipped tests
JimBobSquarePants Jul 10, 2020
3b94e3d
Internalize unsafe component.
JimBobSquarePants Jul 16, 2020
4fe04b6
Use missed offset
JimBobSquarePants Jul 16, 2020
f850e07
Merge branch 'master' into js/buffered-read
JimBobSquarePants Jul 16, 2020
2f3b59f
Merge branch 'master' into js/buffered-read
JimBobSquarePants Jul 16, 2020
bbf5b3b
Use explicit stream instance in core decoders.
JimBobSquarePants Jul 17, 2020
d5f7e5a
Remove unrequired async/await
JimBobSquarePants Jul 17, 2020
30092db
Update PngEncoder.cs
JimBobSquarePants Jul 18, 2020
e6c78c2
Update StreamExtensions.cs
JimBobSquarePants Jul 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions src/ImageSharp/Common/Extensions/StreamExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@
// Licensed under the Apache License, Version 2.0.

using System;
using System.Buffers;
using System.IO;
using SixLabors.ImageSharp.Memory;
#if !SUPPORTS_SPAN_STREAM
using System.Buffers;
#endif

namespace SixLabors.ImageSharp
{
Expand Down Expand Up @@ -40,7 +38,7 @@ public static int Read(this Stream stream, Span<byte> buffer, int offset, int co
/// Skips the number of bytes in the given stream.
/// </summary>
/// <param name="stream">The stream.</param>
/// <param name="count">The count.</param>
/// <param name="count">A byte offset relative to the origin parameter.</param>
public static void Skip(this Stream stream, int count)
{
if (count < 1)
Expand All @@ -51,21 +49,22 @@ public static void Skip(this Stream stream, int count)
if (stream.CanSeek)
{
stream.Seek(count, SeekOrigin.Current); // Position += count;
return;
}
else

var buffer = ArrayPool<byte>.Shared.Rent(count);
while (count > 0)
{
var foo = new byte[count];
while (count > 0)
int bytesRead = stream.Read(buffer, 0, count);
if (bytesRead == 0)
{
int bytesRead = stream.Read(foo, 0, count);
if (bytesRead == 0)
{
break;
}

count -= bytesRead;
break;
}

count -= bytesRead;
}

ArrayPool<byte>.Shared.Return(buffer);

@antonfirsov antonfirsov Jul 21, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a try { ... } finally { ArrayPool<byte>.Shared.Return(buffer); } block.

@JimBobSquarePants JimBobSquarePants Jul 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, will do.

@JimBobSquarePants JimBobSquarePants Jul 22, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

public static void Read(this Stream stream, IManagedByteBuffer buffer)
Expand Down
6 changes: 3 additions & 3 deletions src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanBuffer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// Copyright (c) Six Labors.
// Licensed under the Apache License, Version 2.0.

using System.IO;
using System.Runtime.CompilerServices;
using SixLabors.ImageSharp.IO;

namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
{
Expand All @@ -11,7 +11,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
/// </summary>
internal struct HuffmanScanBuffer
{
private readonly DoubleBufferedStreamReader stream;
private readonly Stream stream;

@antonfirsov antonfirsov Jul 12, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably loose a some of the benchmarked performance gain here, since the instance used here is of the unsealed base class Stream, therefore the virtual methods being called on it are not inlined (unlike in benchmarks!).

@JimBobSquarePants JimBobSquarePants Jul 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t actually see any slowdown at all in jpeg but I’ll run/publish tests tomorrow

@JimBobSquarePants JimBobSquarePants Jul 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antonfirsov Before and after benchmarks. Short run so plenty of error but nothing that stands out. Ideally I'd be able to use the concrete implementation but I always worry about exposing a new class and breaking changes.

Method TestImage Mean Error StdDev Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
'Decode Jpeg - System.Drawing' Jpg/b(...)e.jpg [21] 5.123 ms 7.2206 ms 0.3958 ms 1.00 0.00 - - - 176 B
'Decode Jpeg - ImageSharp' Jpg/b(...)e.jpg [21] 12.219 ms 15.6786 ms 0.8594 ms 2.39 0.02 - - - 15904 B
'Decode Jpeg - System.Drawing' Jpg/b(...)f.jpg [28] 13.339 ms 0.5726 ms 0.0314 ms 1.00 0.00 - - - 176 B
'Decode Jpeg - ImageSharp' Jpg/b(...)f.jpg [28] 29.084 ms 6.2870 ms 0.3446 ms 2.18 0.02 - - - 16880 B
'Decode Jpeg - System.Drawing' Jpg/i(...)e.jpg [43] 329.528 ms 56.3769 ms 3.0902 ms 1.00 0.00 - - - 216 B
'Decode Jpeg - ImageSharp' Jpg/i(...)e.jpg [43] 268.073 ms 179.0279 ms 9.8131 ms 0.81 0.04 - - - 36022424 B
Method TestImage Mean Error StdDev Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
'Decode Jpeg - System.Drawing' Jpg/b(...)e.jpg [21] 4.965 ms 0.6894 ms 0.0378 ms 1.00 0.00 - - - 176 B
'Decode Jpeg - ImageSharp' Jpg/b(...)e.jpg [21] 11.881 ms 0.3953 ms 0.0217 ms 2.39 0.02 - - - 15808 B
'Decode Jpeg - System.Drawing' Jpg/b(...)f.jpg [28] 13.548 ms 1.5905 ms 0.0872 ms 1.00 0.00 - - - 176 B
'Decode Jpeg - ImageSharp' Jpg/b(...)f.jpg [28] 28.147 ms 1.6038 ms 0.0879 ms 2.08 0.01 - - - 16816 B
'Decode Jpeg - System.Drawing' Jpg/i(...)e.jpg [43] 332.870 ms 22.0796 ms 1.2103 ms 1.00 0.00 - - - 176 B
'Decode Jpeg - ImageSharp' Jpg/i(...)e.jpg [43] 286.208 ms 732.9871 ms 40.1775 ms 0.86 0.12 - - - 36022360 B

@antonfirsov antonfirsov Jul 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ~6% change for the largest image, which is suspicious. These are no microbenchmarks, so the result of a short run is relevant, unless there is other noise, which can ofc happen with local runs.

We have DecodeJpegParseStreamOnly as a relatively quick option reassess this.

@JimBobSquarePants JimBobSquarePants Jul 14, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before

Method Job Runtime TestImage Mean Error StdDev Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
'System.Drawing FULL' Job-COEKJJ .NET 4.7.2 Jpg/b(...)f.jpg [28] 16.09 ms 0.024 ms 0.001 ms 1.00 0.00 156.2500 - - 777784 B
JpegDecoderCore.ParseStream Job-COEKJJ .NET 4.7.2 Jpg/b(...)f.jpg [28] 17.96 ms 12.218 ms 0.670 ms 1.12 0.04 - - - 12544 B
'System.Drawing FULL' Job-UZXMLV .NET Core 2.1 Jpg/b(...)f.jpg [28] 15.25 ms 0.973 ms 0.053 ms 1.00 0.00 171.8750 - - 775208 B
JpegDecoderCore.ParseStream Job-UZXMLV .NET Core 2.1 Jpg/b(...)f.jpg [28] 12.52 ms 1.801 ms 0.099 ms 0.82 0.01 - - - 12416 B
'System.Drawing FULL' Job-PIDJAF .NET Core 3.1 Jpg/b(...)f.jpg [28] 14.43 ms 3.392 ms 0.186 ms 1.00 0.00 - - - 191 B
JpegDecoderCore.ParseStream Job-PIDJAF .NET Core 3.1 Jpg/b(...)f.jpg [28] 11.55 ms 1.149 ms 0.063 ms 0.80 0.01 - - - 12408 B

After

Method Runtime TestImage Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
'System.Drawing FULL' .NET 4.7.2 Jpg/b(...)f.jpg [28] 13.52 ms 1.621 ms 0.089 ms 1.00 171.8750 - - 777789 B
JpegDecoderCore.ParseStream .NET 4.7.2 Jpg/b(...)f.jpg [28] 13.01 ms 1.466 ms 0.080 ms 0.96 - - - 12416 B
'System.Drawing FULL' .NET Core 2.1 Jpg/b(...)f.jpg [28] 13.64 ms 3.331 ms 0.183 ms 1.00 171.8750 - - 775208 B
JpegDecoderCore.ParseStream .NET Core 2.1 Jpg/b(...)f.jpg [28] 11.89 ms 0.374 ms 0.021 ms 0.87 - - - 12248 B
'System.Drawing FULL' .NET Core 3.1 Jpg/b(...)f.jpg [28] 13.20 ms 1.116 ms 0.061 ms 1.00 - - - 176 B
JpegDecoderCore.ParseStream .NET Core 3.1 Jpg/b(...)f.jpg [28] 12.09 ms 0.310 ms 0.017 ms 0.92 - - - 12246 B

@JimBobSquarePants JimBobSquarePants Jul 17, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've got a good way now actually.... I updated the internal interface for the core decoders to use the buffered stream. I'll have something to show very soon.

@JimBobSquarePants JimBobSquarePants Jul 17, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antonfirsov Less of a difference than I thought. That's with all the core decoder directly referencing the concrete decoder.

Method Job Runtime TestImage Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
'System.Drawing FULL' Job-RSRDJA .NET 4.7.2 Jpg/b(...)f.jpg [28] 13.28 ms 0.976 ms 0.053 ms 1.00 171.8750 - - 777761 B
JpegDecoderCore.ParseStream Job-RSRDJA .NET 4.7.2 Jpg/b(...)f.jpg [28] 13.54 ms 0.792 ms 0.043 ms 1.02 - - - 12416 B
'System.Drawing FULL' Job-QLXCST .NET Core 2.1 Jpg/b(...)f.jpg [28] 13.24 ms 2.348 ms 0.129 ms 1.00 171.8750 - - 775208 B
JpegDecoderCore.ParseStream Job-QLXCST .NET Core 2.1 Jpg/b(...)f.jpg [28] 11.72 ms 1.564 ms 0.086 ms 0.89 - - - 12352 B
'System.Drawing FULL' Job-KQKRGS .NET Core 3.1 Jpg/b(...)f.jpg [28] 13.08 ms 2.922 ms 0.160 ms 1.00 - - - 176 B
JpegDecoderCore.ParseStream Job-KQKRGS .NET Core 3.1 Jpg/b(...)f.jpg [28] 11.49 ms 0.771 ms 0.042 ms 0.88 - - - 12372 B

@antonfirsov antonfirsov Jul 17, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On .NET Core 3.1:
11.55 (old) VS 12.09 (base Stream) VS 11.49 (concrete Stream)

I think it's quite good, or am I missing something?

@JimBobSquarePants JimBobSquarePants Jul 17, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking at the ratios compared to before but error could be creeping in there. It's definitely faster

I'm gonna push with the concrete implementation as it should bring us the best of everything.

@JimBobSquarePants JimBobSquarePants Jul 17, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antonfirsov OK. I think this is ready to review again now.


// The entropy encoded code buffer.
private ulong data;
Expand All @@ -22,7 +22,7 @@ internal struct HuffmanScanBuffer
// Whether there is no more good data to pull from the stream for the current mcu.
private bool badData;

public HuffmanScanBuffer(DoubleBufferedStreamReader stream)
public HuffmanScanBuffer(Stream stream)
{
this.stream = stream;
this.data = 0ul;
Expand Down
7 changes: 3 additions & 4 deletions src/ImageSharp/Formats/Jpeg/Components/Decoder/HuffmanScanDecoder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@
// Licensed under the Apache License, Version 2.0.

using System;
using System.IO;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using SixLabors.ImageSharp.IO;
using SixLabors.ImageSharp.Memory;

namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder
{
Expand All @@ -19,7 +18,7 @@ internal class HuffmanScanDecoder
private readonly JpegFrame frame;
private readonly HuffmanTable[] dcHuffmanTables;
private readonly HuffmanTable[] acHuffmanTables;
private readonly DoubleBufferedStreamReader stream;
private readonly Stream stream;
private readonly JpegComponent[] components;

// The restart interval.
Expand Down Expand Up @@ -65,7 +64,7 @@ internal class HuffmanScanDecoder
/// <param name="successiveHigh">The successive approximation bit high end.</param>
/// <param name="successiveLow">The successive approximation bit low end.</param>
public HuffmanScanDecoder(
DoubleBufferedStreamReader stream,
Stream stream,
JpegFrame frame,
HuffmanTable[] dcHuffmanTables,
HuffmanTable[] acHuffmanTables,
Expand Down
Loading
⚡