Bacon / BaconQrCode

QR Code Generator for PHP
BSD 2-Clause "Simplified" License
1.88k stars 212 forks source link

Fail to generate more than one QR with different gradients #186

Closed j3j5 closed 1 month ago

j3j5 commented 2 months ago

Generating more than one QR with different gradients while using the SVG backend causes all the QRs to render the same gradient as the first QR.

Example code:

$types = ['HORIZONTAL', 'VERTICAL'];
foreach ($types as $type) {
    $gradient = new Gradient(
        new Rgb(0, 0, 0),
        new Rgb(255, 0, 0),
        GradientType::$type()
    );
    $renderer = new ImageRenderer(
        new RendererStyle(size: 400, fill: Fill::withForegroundGradient(
            new Rgb(255, 255, 255), 
            $gradient, 
            EyeFill::inherit(), 
            EyeFill::inherit(), 
            EyeFill::inherit())
        ),
        new SvgImageBackEnd()
    );
    $writer = new Writer($renderer);

    $strings[] = $writer->writeString('Hello World!');
}

Result: Screenshot_20240928_001125

Expected result: Screenshot_20240928_001231

The issue comes from the SVGImageBackend class, when generating the gradient, it gives it an id based on the number of gradients present on the same QR, but if more than one QR is on the same page, the id is the same (g1 or whatever the number).

There can be several ways to fix this, the easiest I can think of is to attach a random number to the id to make sure it is unique ($id = sprintf('g%d-' . random_int(PHP_INT_MIN, PHP_INT_MAX), ++$this->gradientCount)), another way could be creating a hash based on the gradient properties (type, start color and end color) and attach it to the id after the count. Something like:

$hashValue = $this->getColorString($startColor) . $this->getColorString($endColor) . $gradient->getType();
if ($startColor instanceof Alpha) {
    $hashValue .= (string) $startColor->getAlpha();
}
$id = sprintf('g%d-' . hash('xxh128', $hashValue), ++$this->gradientCount);

XXH128 is supposed to be really fast and 8.1 is now the minimum supported version which added support for it.

I'm happy to create a PR in order to fix this, let me know which way you prefer and I'll implement it.

Ps.- Thanks for this great library! :muscle:

DASPRiD commented 2 months ago

I think going with random_int might be preferred here, though preferably encoded as hex or even base64 for size reasons (basically a nano ID). Reason which speaks for that over a counting variable:

Consider generating two different QR codes in two different requests but storing them and displaying them again inline – they'd have the same ID if using an instance counter per request.

j3j5 commented 2 months ago

I just did some tests:

hash('xxh64', 'test')
// = "4fdcca5ddb678139"

hash('xxh128', 'test')
// = "6c78e0e3bd51d358d01e758642b85fb8"

dechex(PHP_INT_MAX)
// = "7fffffffffffffff"

base64_encode(PHP_INT_MAX)
// = "OTIyMzM3MjAzNjg1NDc3NTgwNw=="

If you are concerned about size, hex seems to be the way to go (although xxh64 is the same size), base64 and xxh128 are similar in size. The only reason for going with the hash is that we get rid of any (admittedly small) possibility of id collisions (they must be the same gradient to collide, which won't matter anyway), but I agree the chances are really small and it may not be worth it the extra complexity.

As I said, up to you, I'll push PR with your choice, just lmk!

j3j5 commented 2 months ago

Just as a curiosity, but I've done another (unscientific) test to try performance:

// hash xxh64
> $test = ['abc', 'cba', 'test', 'test1', 'abcdefghijklmnopqrstuv']; 
$start = microtime(true); 
foreach(range(1, 4000000) as $i) { 
    hash('xxh64', $test[$i%5]); 
} 
$end = microtime(true); 
echo ($end - $start);
// 0.93287110328674

// hash xxh128
$test = ['abc', 'cba', 'test', 'test1', 'abcdefghijklmnopqrstuv']; 
$start = microtime(true); 
foreach(range(1, 4000000) as $i) { 
    hash('xxh128', $test[$i%5]); 
} 
$end = microtime(true); 
echo ($end - $start);
// 1.3112859725952

// dechex(random_int())
$start = microtime(true); 
foreach(range(1, 4000000) as $i) { 
    dechex(random_int(PHP_INT_MIN, PHP_INT_MAX)); 
} 
$end = microtime(true); 
echo ($end - $start);
// 6.8923921585083
DASPRiD commented 2 months ago

xxh64 should be decent enough I'd say :)