search

skip2 / go-qrcode

:sparkles: QR Code encoder (Go)
http://go-qrcode.appspot.com
MIT License
2.58k stars 335 forks source link

Suboptimal segements chosen, causing bitmap generation to fail #37

Open awbacker opened 4 years ago

awbacker commented 4 years ago

I use the library heavily to generate qr bitmaps (manually creating the image), so thank you for creating it!

The following test case should generate correctly:

func TestShouldCreateTheQrCode(t *testing.T) {
    msg := "HTTPS://ABC.DE/F/393AABB6998877XYZ0518AUQCRVJN25"
    //                                 E    <-- change to E and no crash
    qrc, err := qrcode.NewWithForcedVersion(msg, 4, qrcode.Highest)
    if err != nil {
        t.Error(err)
    }
    qrc.Bitmap()
}

=== RUN   TestShouldCreateTheQrCode
2020/06/17 12:26:01 BUG: got len 296, expected 288
--- FAIL: TestShouldCreateTheQrCode (0.00s)
panic: BUG: got len 296, expected 288 [recovered]
    panic: BUG: got len 296, expected 288

I noticed that after the call to encoder.encode the length is 290 (and somehow numPaddingBits() == -2?). In any case, I am not familiar enough with the code to debug fully yet, but will be giving it a try this afternoon.

The multi-segments by default kind of thing is clever, it just appears to be failing in this case. I feel that most users will calculate their version/recovery-level based on the published data lengths which assume single-segment, especially for high volumes.

skip2 commented 4 years ago

Hi Andrew.

Thanks for the kind words.

You're right, the optimiser is being too clever, and a single alphanumeric segment would be a more efficient (and more predictable) encoding.

We currently use a single byte segment when it's more efficient than multiple segments. I've now added use of a single alphanumeric segment (only if the content allows it), when it's more efficient than multiple segments.

https://github.com/skip2/go-qrcode/commit/da1b6568686e89143e94f980a98bc2dbd5537f13

let me know if that helps,

thanks,

skip2

awbacker commented 4 years ago

I took a look at that and it seems like it would work, but I haven't had a chance to try it yet. Looking through your code taught me a lot about QR codes, even though I already knew enough to do the basic stuff like determine a good ecc. Well done :)

In the end I made below change in my vendored version. 

// rather than try only binary, try the "best" encoding based on the data as a 
// single segment in encode([]bytes). 
minEncoding := minDataEncoding(d.data)
singleSegment, err := d.encodedLength(minEncoding, len(d.data))
...

func minDataEncoding(data []byte) dataMode {
    m := dataModeNone
    for _, v := range data {
        if n := minByteEncoding(v); n > m {
            m = n
        }
        if m == dataModeByte {
            break
        }
    }
    return m
}

// smallest mode that applies to a single byte
func minByteEncoding(chr byte) dataMode {
    if chr >= '0' && chr <= '9' {
        return dataModeNumeric
    }
    if chr >= 'A' && chr <= 'Z' {
        return dataModeAlphanumeric
    }
    switch chr {
    case ':', '/', '.', '-', '+', '*', '$', '%':
        return dataModeAlphanumeric
    }
    return dataModeByte
}

I may fork this repo and create a PR, if thats helpful to you in some way. I'll and add this change, and possibly others to make writing "your own" NewWithFixedVersion analog easier, such as public functions for "utility" work, and public functions, and possibly decomposing encode a bit.

My particular case, generating an insane amount of qr codes:

Anyway, the code has been wonderful to work with. Its easy for me to put something together that does just the 1 thing I want, but you have so much more in there! Feel free to close this, as you see fit.

fenglijunnb commented 3 years ago

@awbacker Hi Andrew. I really appreciate your PR, but I have a question about how to generate the flower-style QR code. I really don't have a clue. I hope you can help me 1

fenglijunnb commented 3 years ago

@awbacker This is a similar image that I downloaded from some platforms