SVG data URI urlencoding revisit

[picasticks]

Hi! Thanks again for the other change and all your work on this project. I know this topic is one you already investigated in #180. I'm guessing you decided against adding an alternate URL encoder to Parse just for the combination of SVG data URIs in HTML/CSS.

It's a bit too bad because in all the files I've seen Minify always ends up base64-encoding every SVG data URI, which is a ~30% size increase when the file is already encoded using one or the other of the various tools out there.

Personally I've always worked around this by just commenting out the call to DataURI. With that in mind, what about just adding a new option to the HTML and CSS Minify modes to disable descending into data URIs? As a side effect it would save a little processing time by avoiding parsing data URIs and SVG when selected.

Maybe a better idea: DataURI could exit early and return the original bytestream unmodified as soon as it detects that the base64Len and asciiLen are larger than the original. I hacked together a quick version below, which unfortunately does a copy of the datauri bytestream. I didn't spend long on it but I can't think of an easy way to avoid this. On the good side, the function also exits early sometimes which avoids calls to URL/base64 encoders. So is it faster or slower? As a quick test I concatenated together 2 MB of CSS containing 400 svg urls but couldn't measure a difference in real/user/sys time given all the variance you see between trials testing on a laptop. But I don't think there's a performance penalty.

diff --git a/common.go b/common.go
index d722635..aed6d89 100644
--- a/common.go
+++ b/common.go
@@ -37,4 +37,7 @@ func Mediatype(b []byte) []byte {
 // DataURI minifies a data URI and calls a minifier by the specified mediatype. Specifications: https://www.ietf.org/rfc/rfc2397.txt.
 func DataURI(m *M, dataURI []byte) []byte {
+   original := make([]byte, len(dataURI))
+   copy(original, dataURI)
+   originalLen := len(original)
    if mediatype, data, err := parse.DataURI(dataURI); err == nil {
        dataURI, _ = m.Bytes(string(mediatype), data)
@@ -45,8 +48,11 @@ func DataURI(m *M, dataURI []byte) []byte {
                asciiLen += 2
            }
-           if asciiLen > base64Len {
+           if asciiLen > base64Len || asciiLen > originalLen {
                break
            }
        }
+       if base64Len > originalLen && asciiLen > originalLen {
+           return original
+       }
        if asciiLen > base64Len {
            encoded := make([]byte, base64Len-len(";base64"))

I'm don't work in Go but I can take a try at implementing something if you like, and send a pull request.

Or feel free to close this issue out you don't like any option, I won't be offended. Thanks!

[tdewolff]

Thanks for the suggestion, sounds like a good idea to prevent minifying data URIs if they result in longer strings. In fact, if we minify into a longer string that means the initial data URI is badly encoded, but I think the minifier should not try and fix errors but just ignore and pick the smallest. Some browsers allow for smaller data URIs to be possible (less characters need to be URL encoded), so the input may be already optimized for that. This cannot be said for all users in general though, so we need to remain conservative.

[picasticks]

Cool, yeah that's the idea of the patch.

If someone has manually minified or decided not to urlencode characters like spaces because the browsers don't mind this, and the result is smaller than what Minify produces, then keep that smaller version. Minify is there to make things smaller not act as a strict validator ... I mean it's nice when you can fix people's errors too but that's not the main point of running a minifier.

And I agree for all users, for general use cases and cases like streaming, what Minify already does is safest and should be the default.

[picasticks]

Hi again, I pulled your commit 315e0a7 and tested it.

There's a problem I don't think you noticed, you're running the URI through parse.DataURI at the beginning to decode it "%3Csvg" to "<svg". So then on line 56 you are returning an unencoded/unescaped version of the data URI which is bad.

For your test string you have got:

minify/common_test.go

Line 51 in 315e0a7

{`data:text/svg+xml,<svg height="100" width="100"><circle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" /></svg>`, `data:text/svg+xml,<svg height="100" width="100"><circle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" /></svg>`},

Instead you would want this to be:

{`data:text/svg+xml,%3Csvg xmlns="http://www.w3.org/2000/svg" height="100" width="100"%3E%3Ccircle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" /%3E%3C/svg%3E`, `%3Csvg xmlns="http://www.w3.org/2000/svg" height="100" width="100"%3E%3Ccircle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" /%3E%3C/svg%3E`},

The idea is that the data URI is already partially urlencoded. You want to return back that same partially urlencoded version (when it is the smallest), not the decoded version.

That's why in my patch I make a copy of the original value at the top of the function, to be able to return the original value before it is decoded by parse.DataURI.

[tdewolff]

Yes you're right! Thanks for the eagle-eye. The copy is a bit of a pity indeed, but data URIs are not too common so this should not be in a hot path. I don't suspect this to impact performance in a significant way. I don't see a good way to get around it either...

[picasticks]

Yeah, I did some testing, I was concerned about that too, I concluded the same after running it on a 20 MB CSS file I concatenated from 1000 real ones. It does cause the function to sometimes exit early (which is a benefit since it avoids doing either the base64 or url encode) but how often would depend on the file contents.

Also, another small thing. I think the copy would happen on all URLs, not just data URIs. At least for CSS because css.go is calling minify.DataURI on all URL tokens. So this copy would happen for all URLs even url("foo.png") but at least those kinds of urls are small.

Now that issue I think you could work around by checking the whether the bytestream is a data URI or not before doing the copy or running parse.DataURI. Just test whether the byte slice starts with "data:" and if so have minify.DataURI return immediately. Otherwise do the copy, then do parse.DataURI.

[picasticks]

Oh, also, I pulled your commit. Everything works good for me. Thanks!

[tdewolff]

I think we can be smarter with DataURIs. Right now we're decoding the URI, then checking the length of a base64 encoded URI and an URL encoded URI, then encoding in either of them. Instead, we should be able to count the length of base64/URLencoded WITHOUT doing any decoding first. Depending on the result we can either URLdecode and then base64encode, or URLdecode AND URLencode in one pass (if URL encoding is right we leave it be, if it is encoded but not necessary we decode it, if it is not encoded but should be we encode it). This dismisses the need to copy beforehand and still provides an early exit when the original is shorter than either.

[picasticks]

Another thing would be to branch on the mediatype, so that Minify does not bother base64 decoding common binary types that appear in web urls like png/jpeg/webp. Or any type that doesn't have a Minifier.

That would require a function to pull the mimetype from the url without running parse but should be pretty simple. Check if the first 5 bytes are "data:". Then read the next bytes until either ; or , delimiter. Finally, if the mimetype is registered to a minify function then continue processing, else exit.

That would take care of the vast majority of data URIs that are actually in use.

It looks to me like Minify supports:

  "css":  "text/css",
  "htm":  "text/html",
  "html": "text/html",
  "js":   "application/javascript",
  "json": "application/json",
  "svg":  "image/svg+xml",
  "xml":  "text/xml",

It is technically possible to put any of these in a data URI, but no one really does that with any of these types except svg. So vast majority of the time minify.DataURI function would just exit immediately because it doesn't see one of these types. For png, jpg, gif, webp etc.

Unless I am wrong, but when I was testing before it looked like Minify was base64 decoding image/png to a binary byte array and then reencoding it.

[picasticks]

I like your idea but it seems hard, if you still want Minify to descend into an SVG file and try to minify the XML then you have to decode it to try that. And you don't know whether or not XML/SVG minification will reduce the size of the SVG or not without trying it. That's where if there was a CLI option like --SkipDataURI to skip descending into data URIs then there would be no reason for to decode them at all when that option is used. Because base64 is always going to be smaller than a complete urlencode of SVG, because there are so many whitespace, <> "' / characters in SVG and no big blocks of text.

With that said I think something like this could work to estimate the lengths if you don't mind counting the "%" characters in the byte slice:

bytes := []byte("foo%20bar")
asciiLen := len(bytes) // asciiLen == 9 in example
for i := 0; i < len(bytes); i++ {
    if bytes[i] == 37 /* 37 == "%" ... */ {
        asciiLen = asciiLen - 2
        i += 2 // because you can skip ahead 2 extra bytes when % is found
    }
}
// asciiLen == 7
base64Len := len(";base64") + base64.StdEncoding.EncodedLen(asciiLen) // == 19
if base64Len < len(bytes) {
    // urldecode and base64 encode
} else {
    // return / exit
}

Sorry if that's bad, that is the most Go I have ever written. :)

[tdewolff]

The data URI minification isn't solely to compress known mimetypes. Also unknown mimetypes that do not have a minifier attached can be shortened by using base64 or URL encoding, even though this is probably usually not the case.

I think we have three cases that we need to consider:

• base64 encoded data: decode the base64 data (but keep the base64 encoded data), minify if minifier defined for mimetype, iterate over the data and see if ASCII (URL encoding) surpasses the original data URI length. As soon as it does, stop iterating over the data and return the already base64 original data URI. If ASCII is shorter, return that.
• ASCII URL encoded data without minifier: count the length of data it represents (ie. URL encoded characters such as %20 count for one character) and see if base64 would be shorter than the original data URI. If so, decode the URL encoding and then base64 encode. If the data contains unnecessary URL encoded characters (such as %20 which can be represented as +, or %41 which is A), remove those encodings. Otherwise keep the original (i.e. do not encode characters that we think should be encoded).
• ASCII URL encoded data with minifier: copy the data URI and decode the URL encoding (to keep the original), run the minifier attached to the mimetype. If base64 encoding is shorter than the original, base64 encode. If ASCII URL encoding is shorter than the original, URL encode. Otherwise keep the original.

What do you think?

[picasticks]

Right, I was thinking about that too, the first thing I did was go look at the IANA page that lists all registered mediatypes to see if I could generalize anything. Ideally, detect if the datauri is a binary vs. text format -- for binary like image/png or image/jpg (which is probably 90% of the data URIs that you see in real-life HTML/CSS content), why would we want to waste cycles decoding the data? (OK, well there could be, I mean sure you could run images through zopflipng or jpegrescan, but that is compute intensive.)

Anyway I couldn't really see an obvious/clean/general way to do that but a "good enough" way would just be to hardcode a list of known common binary types in data URIs: image/png, image/jpeg, image/gif, image/webp, image/jp2 (safari used to support jp2). Probably audio/* and video/*. Maybe application/pdf. There are also emerging image formats HEIF and AVIF (image/heif, image/heic, image/avif, image/avif-sequence) that may replace png/jpeg/webp. So to simplify it for image types I'd suggest to define it as image/* except image/svg+xml.

So maybe you like that approach better than what I suggested before? This way, Minify can still attempt to improve the encoding of any other random file type that could be present (say, something like text/calendar or text/vcard) but does not waste a base64 decode/encode step on binary.

I guess the flow would basically be:

1. Test if url() is data URI (look for data:), otherwise exit/return original.
2. Extract the mediatype or at least walk the first ~10 bytes without parsing/decoding the data URI. If type ~ '(audio|video|image)/' && ! type ~ 'image/svg' then:
   • if base64 encoded, exit/return original. (% encoding is always going to be worse than base64 for binary)
   • else (someone has %-encoded a PNG which is stupid but it is technically possible) continue ...
3. Proceed according to what you have described in your 3 bullets. I'll just add a couple comments:
   • When original is base64: Personally I'm still not sure it ever makes sense to decode base64 except when the underlying data is something that has a minifier function. Just because of the characters like spaces and all non-alphanumeric characters that have to be urlencoded. I'm just thinking of svg/html/xml, JSON like formats and the amount of whitespace, <> ,; [] {} etc they contain. Even normal text has got a space after every 5 characters, plus punctuation, \n\r etc. I guess you could look at a corpus of data URIs from web content. But that's up to you it's your project! Definitely I agree for exhaustive minification you would decode base64 and check. Personally I would vote to never decode base64 except when it's a type with a minifier.
   • Agree 100%. Side comment: You can't urlencode as +, that is a specific thing defined only for HTTP query parameters. I tested + in an SVG, it definitely doesn't work in the browser. It has to be %20. (The RFCs 1738, 2396, 3986 that define urlencode just say to % encode everything. As best as I can tell the spec allowing + to represent space in HTTP queries derives from this). (And when you put + in an SVG data URI the browser reads it as +)
   • Agree 100%