[WIP] SVG support #30
Conversation
| }; | ||
|
|
||
| function isSVG (buffer) { | ||
| return ('<' === utils.ascii(buffer, 0, 1)); |
There was a problem hiding this comment.
The file format detection is really naive and should be improved. For instance, it wouldn’t work for SVGs that have whitespace at the beginning. I would rather test the whole file for regExps.rootTag and assume its an SVG if it matches.
|
@mgartner what do you think? Any advice on how to improve it? |
|
I'm not too familiar with the SVG format, but it would be nice to not have to read the entire file to detect whether or not it is an SVG and to measure it. I'm not sure that's possible. I need to research the SVG format more. If a full read on the file is necessary, I'm not sure supporting SVG would make sense for Calipers, and maybe a separate SVG parser should be used. If a good case can be made for Calipers to include it, I think that at the very least, the API should be updated so that supported file types can be whitelisted so that someone who has no need to detect SVG won't see a performance hit from a full file read. |
|
@mgartner here is an article describing the basic logic for determining SVG size: https://developer.mozilla.org/en-US/docs/Web/CSS/Scaling_of_SVG_backgrounds. I guess I have some ideas on how to not load the whole file to get the dimensions: since the only thing needed to get those is the opening As for the file type detection it could be agreed that Calipers understands only SVGs where that tag was introduced in the first 32 bytes — otherwise it can't detect it (but the file could still be generally valid) and SVG should be fixed. This could be improved in the future if some other kind of detection is implemented, like looking at the file extension or manually passing the type. What do you think? As for the reasons of why I am eager to help getting SVG work is my postcss-assets lib, which among other things helps to automatically insert image dimensions into CSS. Since SVG is a very popular Web format I care about supporting it. Currently the lib is using image-size, but SVG support there has some serious bugs and the project itself looks abandonned, so I'm looking at Calipers to migrate to it. |
|
@mgartner I've refactored the code attempting to make it more readable and made the file type detection more sane. However, dimension calculation still needs to be improved to not read the whole file. I need to find a way to search for the first |
|
@mgartner the other option could be using third-party XML parsers that parse streams and are able to stop when the object of interest is found. I've quickly searched for such libs and noticed these two:
I guess I'll try to update the code to use those and see what would happen. |
| return pread(fd, new Buffer(BUFFER_SIZE), 0, BUFFER_SIZE, offset) | ||
| .spread(function (bytesRead, buffer) { | ||
| string += utils.ascii(buffer, 0, bytesRead); | ||
| var rootMatches = string.match(ROOT_MATCHER); |
There was a problem hiding this comment.
RegExp match is done for the whole part read from the file on every fs.read pass.
|
Okay, that was a tough one. I've tried some XML parsers, but eventually I felt that those are an overkill for such a minor task, so I kept the parsing with regular expressions. However, files are no longer read entirely with no need - they are read chunk by chunk until the RegExp matches and then it stops. @mgartner could you review the code and share your thoughts and advice? |
|
Okay, I've did some benchmark and it looks slower than the others: |
|
That's actually much more performant than I thought it would be. Have you Let me run some more benchmarks myself, but I'm feeling more confident this I also am planning on allowing an optional parameter in On Saturday, November 7, 2015, Vadym Borodin notifications@github.com
|
|
Since we're going to introduce modules, I've extracted the code from this PR to a separate repo: https://github.com/calipersjs/calipers-svg |
Solves #29
This PR:
However, there are stuff to improve performance-wise and very lame format detection.