codebude / QRCoder

A pure C# Open Source QR Code implementation
MIT License
4.58k stars 1.09k forks source link

Consider a Streamlined/Slimmed-down Version 2.0 #242

Open csturm83 opened 4 years ago

csturm83 commented 4 years ago

Type of issue

[ ] Bug
[ ] Question (e.g. about handling/usage)
[X] Request for new feature/improvement

Expected Behavior

Current Behavior

Possible Solution = QRCoder v2.0

Rationale

In my mind, it wouldn't be a huge divergence from the current codebase - just mostly stripping out System.Drawing. Which would likely impact the embedded icons feature. Maybe layer a separate package (that depends on System.Drawing) on top of the "Core" package - so consumers can 'light up' features they need?

PhonicUK commented 4 years ago

I actually just ran into some headaches running this on Linux for some of the above reasons. The dependencies on PresentationFramework and PresentationCore prevented me from using it properly on Linux as things would fail trying to resolve those dependencies. I've forked the library ( https://github.com/CubeCoders/QRCoder ) to remove XAML/WPF support which gets it to play nice on Linux. libgdiplus isn't a huge deal, it's in the repos for most modern distributions. I may yet work on producing such a 'pure' build of this without any dependencies.

PhonicUK commented 4 years ago

@csturm83 turns out this was less work than I thought. My fork above has removed all references to System.Drawing and System.Drawing.Common, removes a handful of the renders (Base64QrCode is no-more, but as you noted you can just use the output from one of the other renderers to do that) and I've removed the cruft that allowed it to support .Net 3.5 so it's now the same codebase for .Net 4.5.2/Core (could be easily changed to target Standard instead) - I've not removed all the various random Payloads just yet but they didn't stop me from stripping the library down.

csturm83 commented 4 years ago

@PhonicUK I'm curious why you would have issues related to XAML/WPF on Linux using the current version. Were you trying to use a .NET Framework binary/dll directly, or using a PackageReference?

An application running on Linux should be targeting a .NET Standard version of the library, which doesn't include the XAML/WPF code and references. If you're targeting a .NET Standard version of the library and still have an error, that is likely a bug.

Do you have any additional error information for the issues you were experiencing on Linux?

PhonicUK commented 4 years ago

My application targets .Net Framework rather than .Net Core, so Nuget didn't give me the option to use the .Net Standard version which resulted in it trying to resolve assemblies at runtime on Linux that were never going to exist. I was also running into issues because I'm using this library indirectly via an upstream dependency. I also needed to remove all of the System.Drawing references anyway to not rely on libgdiplus on Linux.

CJPNActual commented 4 years ago

FWIW I have a heavily-optimised version of QRCoder which generates raw QR data (which is really all I'm interested in, as drawing images is a separate problem) between 10x (1 data input and low-ECC) and 25x (max-input and high-ECC) faster than baseline QRCoder.

Considering a fork or some such in case anybody needs to generate a huge number of QR codes in a hurry.

csturm83 commented 4 years ago

@CJPNActual I'd be curious if your speedup compared to QRCoder is strictly in comparison to its code that does the equivalent (raw data), or if you are including QRCoder rendering in your comparison. Also curious if your fork is netstandard2.0, or if it uses newer runtime features to achieve optimization. Also curious how far have you deviated from the current design (if at all). Could some of the optimizations be easily incorporated into the current code base?

I could see a potential use case where someone might want to throw simple QRCode generation behind a Web API. Maybe returning Base64 encoded strings?

CJPNActual commented 4 years ago

@csturm83

@CJPNActual I'd be curious if your speedup compared to QRCoder is strictly in comparison to its code that does the equivalent (raw data), or if you are including QRCoder rendering in your comparison.

This effort has focused entirely on the performance of QRCodeGenerator.CreateQrCode(string, ECCLevel) as in our usage scenario the generation of a QR image from binary is performed dynamically at vastly different points in time and space (servers in antipodean relation to each other). That said, the generation of an image from a raw QR is relatively trivial. The last time, long ago, I profiled .NETs image/graphics infrastructure it performed rather terribly, yet I understand it to have improved significantly in more recent times. Or, you know, the end user might be crazy and throw large numbers of raw QRs at a GPU and perform the final rendering in that manner. IIRC our production code utilises a custom method for image rendering, so beyond the scope of this optimisation work.

Also curious if your fork is netstandard2.0, or if it uses newer runtime features to achieve optimization. Also curious how far have you deviated from the current design (if at all). Could some of the optimizations be easily incorporated into the current code base?

Observed improvements in performance are achieved only by eliminating vastly-redundant CPU-thrashing along with improvements in algorithmic efficiency and memory use. No reliance on differences in optimisations of the underlying flavour of .NET, or any other such "black box" optimisations in the JITer or whatnot. Indeed I observe only slight improvements in performance between the various sub-flavours of reference-QRCoder, so one wonders whether optimisations implemented in .NET Standard/Core are being retroactively applied to .NET Framework 4.7.2 (really not my field of expertise).

You would instantly recognise the optimised version owing to the fact that the high-level structure is essentially identical to existing QRCoder. As such it is essentially a plug-and-play affair to incorporate in to current code base.

I could see a potential use case where someone might want to throw simple QRCode generation behind a Web API. Maybe returning Base64 encoded strings?

csturm83 commented 4 years ago

Considering a fork or some such in case anybody needs to generate a huge number of QR codes in a hurry.

You would instantly recognise the optimised version owing to the fact that the high-level structure is essentially identical to existing QRCoder. As such it is essentially a plug-and-play affair to incorporate in to current code base.

@CJPNActual would you in fact consider sharing your fork (or submitting a PR)? I'm sure the community would be most appreciative 😄 .

CJPNActual commented 4 years ago

@csturm83 By all means, although I'm not yet finished with the project. As of today, alphanumeric V40(H) codes are issuing forth 36x faster.

csturm83 commented 4 years ago

As of today, alphanumeric V40(H) codes are issuing forth 36x faster.

@CJPNActual That's awesome!

I'm curious if you had experimented at all with flattening QRCodeGenerator.ModuleMatrix into a single BitArray? Seems like a slight increase in lookup overhead (calculating a row offset) might be worth the decreased allocations and better data locality. I had thought about experimenting with it myself, but just haven't taken the time.

cc @jnyrup who has previously shown interest in performance within this repo.

CJPNActual commented 4 years ago

@csturm83

I'm curious if you had experimented at all with flattening [QRCodeGenerator.ModuleMatrix]

Indeed that's one of the very first strategies adopted, in fact going as far as abandoning BitArrray altogether for intensive operations. Instead I've (re)implemented BitArray's core functionality as follows:

public byte[] compactModuleMatrix;
public QRCodeData(int version)
{
    this.Version = version;
    Size = ModulesPerSideFromVersion(version);
    int compactSize = (int)Math.Floor((Size * Size) / 8f) + ((Size * Size) % 8);
    compactModuleMatrix = new byte[compactSize];
}

Then

public bool this[int Y, int X]
            {
            get {
                if (X >= Size || Y >= Size)
                    throw new Exception("Attempt to access QR module beyond bounds of array.");

                int baseAddress = ((Y * Size) + X) / 8;
                int interByteOffset = ((Y * Size) + X) % 8;
                byte offsetMask = (byte)(128 >> interByteOffset);
                int thisBit = compactModuleMatrix[baseAddress] & offsetMask;
                return thisBit == offsetMask;
            }
            set {
                if (X >= Size || Y >= Size)
                    throw new Exception("Attempt to access QR module beyond bounds of array.");

                int baseAddress = ((Y * Size) + X) / 8;
                int interByteOffset = ((Y * Size) + X) % 8;

                if (value) // true
                    compactModuleMatrix[baseAddress] |= (byte)(128 >> interByteOffset);
                else
                    compactModuleMatrix[baseAddress] &= (byte)~(128 >> interByteOffset);
            }
        }

Which allows:

var ourQRData = new QRCodeData(versionWhatever);
bool ourModuleValue = ourQRData[12, 34];
ourQRData[56, 78] = true;

There are some very good reasons for manually managing the QR's core memory structure further down the line.

P.S. Just cracked 40x faster for V40(H).

CJPNActual commented 4 years ago

Today's update:

68x faster/6.7ms per code for V40(H) and 16x faster/120μs per code for V1(L) on a single thread on an AMD 3900X.

62x faster/7.0ms per code for V40(H) and 15x faster/123μs per code for V1(L) on a single thread on an Intel Xeon E3-1270 v6.

Significant improvements still likely at the V1(L) end, although close to entering the validation phase as it's unlikely there will be further gains without resorting to code-gymnastics with CPU-intrinsics, SIMD or a fundamental reconsideration of the entire structure of QRCoder.

CJPNActual commented 4 years ago

@csturm83

Another update (AMD 3900X):

V1(L): 25x faster @ 76μs/code

V40(H): 77x faster @ 5.85ms/code

codebude commented 4 years ago

Hi @CJPNActual ,

I really appreciate the efforts you put into the performance optimization and I would be really happy if you share/merge your results at the end with us.

One question: Did you all optimization without touching the public api/methods or would merging your optimization also mean breaking changes for users of the current version of the lib?

CJPNActual commented 4 years ago

Hi @codebude

None of the optimisations affect the public methods or API in any way. Indeed, in order to generate the benchmarks and validate the optimised code itself I spin up one new and one old version of QRCoder via assembly aliases (because all classes and methods etc. are identically-named), then iteratively spit an array of random data of appropriate (optionally random) length at each version of the the QR generator and compare the resulting data.

//Abridged testing method

newVer::QRCoder.QRCodeGenerator newQrGenerator = new newVer::QRCoder.QRCodeGenerator();
original::QRCoder.QRCodeGenerator origQrGenerator = new original::QRCoder.QRCodeGenerator();

int numtests = 123; //Depending on how long one is prepared to wait
byte[][] newResults = new byte[numtests][];
byte[][] origResults = new byte[numtests][];
string[] testStrings = new string[numtests];

newVer::QRCoder.QRCodeGenerator.ECCLevel eccLevelnew = newVer::QRCoder.QRCodeGenerator.ECCLevel.L/M/Q/H;
original::QRCoder.QRCodeGenerator.ECCLevel eccLevelold = original::QRCoder.QRCodeGenerator.ECCLevel.L/M/Q/H;

for (int i = 0; i < numtests; i++)
{
    testStrings[i] = GenRandomData(int MaxLength, bool RandomLength);
}

//Benchmark new
for (int i = 0; i < numtests; i++)
{
    newVer::QRCoder.QRCodeData qrCodeData = null;
    qrCodeData = newQrGenerator.CreateQrCode(testStrings[i], eccLevelnew);
    newResults[i] = qrCodeData.GetRawData(newVer::QRCoder.QRCodeData.Compression.Uncompressed);
}
//End benchmark new

//Benchmark original
for (int i = 0; i < numtests; i++)
{
    original::QRCoder.QRCodeData qrCodeData = null;
    qrCodeData = origQrGenerator.CreateQrCode(testStrings[i], eccLevelold);
    origResults[i] = qrCodeData.GetRawData(original::QRCoder.QRCodeData.Compression.Uncompressed);
}
//End benchmark original

//Validate
for (int i = 0; i < numtests; i++)
{
    assert(newResults[i].SequenceEqual(origResults[i]));
}
Tragen commented 4 years ago

@CJPNActual, where can I find your changes? Can we merge them?

codebude commented 3 years ago

Hi @CJPNActual , are you still active/working on the optimization? Do you mind sharing your results or sending in a PR?

@csturm83 Even if the discussion has drifted a bit into the performance track (further efforts on performance should be outsourced to a separate issue), I'd like to take up your original idea of a "streamlined" version again. I admit, I had a little trouble with it in the beginning, but I can slowly got used to the idea. However, I can't do this alone and need input from the community.

In essence, I see four issues that need to be clarified:

  1. Structuring of the Library/Nuget packages
  2. Structuring of the github repository(s)
  3. Framework/targets
  4. Transition phase and side-effects

When we have fully clarified these three topics (and more if the community can think of more), we can start with a QRCoder 2.0.

To give the whole thing some drive, here are my thoughts/questions about the individual topics.

  1. Structuring of the Library/Nuget packages
    • What are the criteria for splitting? I see different variants here:
      • QRCoder2Core, QRCoder2Tools (whereby Core takes QR generation topics and Tools contains renderers and Payload generators)
      • QRCoder2Core, QRCoder2Renders, QRCoder2PayloadGenerators (As above, but with separate lib for payload generators)
      • QRCoder2Core, QRCoder2Renders, QRCoder2RendersDrawingDependent, QRCoder2PayloadGenerators (As above but with two packages for rendering - one for renderes with dependencies to System.Drawing)
    • Where to "place" QRCoder.Unity here? As an additional package? As part of the rendering packages?
    • How should the packages be named? Something like QRCoder2PayloadGenerators seems a bit clumsy.
  2. Structuring of the github repository(s)
    • Shall we "evolve" from QRCoder to QRCoder V2 in this repository or is it wiser to set up a new github repository?
    • All packages/subprojects in one Github repo or should we setup a repo per subproject? (That's what I currently would prefer.)
  3. Framework/targets
    • Which frameworks should be targeted? .NET Core?, .NET 5.0, .NET Standard, .NET Framework X? Currently I tend to .NET Standard, because (in my understanding) this would ensure that QRCoder works in .NET Core as well as in .NET 5 and .NET Framework(?). Do I see this correctly?
  4. Transition phase and side-effects
    • How shall/can a conversion from QRCoder to V2 be done?
      • Can we publish the 2.0 package in the same Nuget feed or should we create a new Nuget package/feed? (I guess a new one would avoid unwanted breaking changes, when someone updates without care. But on the other side - how to make users of the old version then aware that there's a new QRCoder V2?)
      • Shall we publish a "last" QRCoder V1 where all functions are marked as obsolete with a notice to the new package?
    • Depending on the target framework version (see 3.), I fear we will drop some users that use QRCoder in their legacy apps/code. We should discuss those side effects, especially:
      • How big is the userbase we lose, when cutting off for Framework version X.Y.Z?
      • How to handle the "legacy version" (V1) of QRCoder then? (In terms of updates strategy. None? Security? Performance-only?)

I think it is already clear that there are a few things to clarify for a V2. I will be happy to work on it with you, but I need your help. Too big are the consequences of a wrong decision in the design of the new version for me to want to make these decisions all alone.

Looking forward to a productive discussion!

georgefst commented 3 years ago

Any chance of a resolution here? @PhonicUK's fork is the only QR library I've successfully managed to import in to a cross-platform Unity project, so I'd love to see something like that become more official.

natehitze commented 3 years ago

.NET 6 may drop support for System.Drawing.Common on non-Windows platforms: https://github.com/dotnet/designs/pull/234

To keep the conversation moving on the original topic, here's my 2c.

  1. Structuring of library: Separate packages by dependency (e.g. one for System.Drawing, one for ImageSharp, etc.).
  2. Structuring of repo: I'd vote keep it all together in one repo. As a library consumer it's easier to have everything in one place: issues, documentation, versioning, releases, etc.
  3. Targeting: This is the guidance from MS: https://devblogs.microsoft.com/dotnet/the-future-of-net-standard/#what-you-should-target
  4. Transition: I think a transition in the current package (assuming major version number change) can work. As long as there is documentation in place for the users who upgrade, run into missing references, and then search for why their QRCoder references don't work anymore.
georgefst commented 2 years ago

@PhonicUK's fork is the only QR library I've successfully managed to import in to a cross-platform Unity project

I haven't been keeping an eye on what changed, but the latest upstream version does now work. Thanks!

emorell96 commented 2 years ago

Is there any interest in this still? Because I would be happy to help adding ImageSharp to this library specially for making the renderers more adaptable to many platforms now that NET 6.0 can be pretty much anywhere.

I have actually started porting the ArtQRCode to ImageSharp, and it works:

With ImageSharp: (it seems like it adds Anti-Aliasing) qrCode2

With System.Drawing (old): original

I structured the new project as @natehitze said, i.e. a new project called QRCoder.ImageSharp. And kept it separate from the rest. I'd be happy to open a pull request if there's still interest in this :)

CJPNActual commented 2 years ago

Hi all,

As of today I've succeeded in completely removing all unnecessary string manipulation (treating binary data as string = "1110100101110101010101whatever") from QRCodeGenerator.cs. Have some bit-bashing performance tweaks to apply in order to eke out further speed on the new binary-represented end, however in addition to the x30 - x100 speedup when generating codes, depending on size and ECC level, the overall memory footprint / string cache spam / garbage collector effort is obviously significantly improved. This is something I consider extremely important in high-volume / server-side applications, which is my personal use-case of QRCoder.

In short, our data bits are now bits, not 16-bit characters in a string.

It's works with Alphanumeric and Numeric code types of any size and ECC level and produces identical results to the non-optimized reference.

Additionally, owing to local requirements, am ensuring QRCodeGenerator.cs functions identically in .NET Classic and .NET Core 6. As mentioned previously, I consider actual image generation a separate effort, as it has been noted here that certain System.Drawing.XYZ components have now been officially declared windows-only for the foreseeable, if not indefinitely.

Expect further updates in the next day or so.

:)

-CJPN

Edit : Further clarity.

emorell96 commented 2 years ago

@CJPNActual that's really nice. I sadly haven't gotten any feedback from the original contributors here so I am not sure that things will be merged.

I agree with the image generation, I think it should be in their separate packages depending on what renderer is used.

CJPNActual commented 2 years ago

@emorell96 Well... as of today QRCodeGenerator.cs (106.4x faster with further reduced memory gymnastics!) is a completely different piece of software when compared to QRCoder's ("reference") equivalent, which itself is a like-for-like (one way or another) transcoding of a Python QR library - line for line. There's nothing preventing me from releasing it as an independent entity, however I don't much fancy the administrative overhead of maintaining a public domain code project. Much prefer my fortress of optimisation solitude.

There occasionally occurs a burst of activity via this GitHub. I've responded directly to some emails from @codebude, including providing a hasty dump of the semi-optimised code, as was busy, but tumbleweeds and distant tolling bells are the norm.

Will contemplate how to proceed.

emorell96 commented 2 years ago

@CJPNActual I'd be happy to take on the maintaining aspects of it. I've been mainly updating the other aspects of this library: the renderers (adding ImageSharp and SkiaSharp support on https://github.com/StockDrops/QRCoder - I was also trying to get it merged here but it's been a few weeks and no response).

So if I can help let me know. I'd be happy to help maintain this library. It has been of great help to my own personal project.

@codebude if you need more helping hands on this projects let me know, I'd be happy to help.

CJPNActual commented 2 years ago

@emorell96 OK. Here are some thoughts on how to proceed and some background:

1) After a good code-scrub, to remove a huge amount on unused junk and commented-out stuff, and adding comments for anybody not within my own mind, I'm happy to share new-QRCodeGenerator.cs as alpha/beta status. I've reliably tested millions and millions of QR codes, but something will break in way I cannot anticipate. Somebody else needs to break it for me.

2) We need a name other than new-QRCodeGenerator.cs. "QRCodeGeneratorSlim"? "DietQRCode"? "QRCodeZero™"?

3) As the optimisation effort on this end was inside-out, starting with the hottest code paths, which happen to be the MaskPattern tests (or was it the polynomial stuff?), the irony is that I've subsequently developed techniques which can be back-applied to hotter code paths rendering perhaps another 1.5x - 2x speedup over the current version. Now that we've got rid of the strings, the slight memory expense (everything now fits comfortably in L2/L3 cache) is essentially free overall. However, we should consider the current, as of today, version as PaperThinQRCodeGenerator v0.9, with v1.0 having had said additional optimisations applied further in to the future. It's more important to get the ball rolling overall, not end up optimising to the point of screaming in machine code at the computer in 2038.

P.S. Er... "JoinMyDietingDefinietelyNotAPyramidSchemeQRCodeGenerator.cs today"???

CJPNActual commented 2 years ago

Out of interest, @emorell96, given that you're working on the data-to-graphics aspect, are there any preferred in-memory formats that could be more easily/readily consumed other that iterating through the x and y of the QR code emitted by the core?