Add changeDpiBuffer method #8
Conversation
bmathews
force-pushed
the
changeDpiBuffer
branch
from
b4b539a
to
6b79edc
Compare
|
Hi @bmathews, i have a couple of questions i left in the PR code comments |
| @@ -51,6 +52,13 @@ var _H = 'H'.charCodeAt(0); | |||
| var _Y = 'Y'.charCodeAt(0); | |||
| var _S = 's'.charCodeAt(0); | |||
|
|
|||
| function changeDpiBuffer(buffer, dpi, type) { | |||
| var headerChunk = new Uint8Array(buffer.slice(0, 33)); | |||
There was a problem hiding this comment.
once the first 33 bytes are converted to array, can we autoDetect the type? the png signature is fixed and the jpeg more or less too. In this way we can find out the type and having the function require an argument the other do not required.
There was a problem hiding this comment.
added getType method that checks the relevant few header bytes.
There was a problem hiding this comment.
like this is the png start
137 80 78 71 13 10 26 10 <--- decimal values of the first 8 bytes.
| var rest = buffer.slice(33); | ||
| var changedHeader = changeDpiOnArray(headerChunk, dpi, type); | ||
| return Buffer.concat([changedHeader, rest]); | ||
| } |
There was a problem hiding this comment.
What happen when this gets used in a browsers? can we stop the code reaching this point if Buffer is not supported? Can we add as a peer dependency some module that would allow to use this function in the browser too?
There was a problem hiding this comment.
Updated this to be browser/node agnostic.
|
please let the dist/index.js out of the changes, i ll build it as soon as i bump up the version number. |
bmathews
force-pushed
the
changeDpiBuffer
branch
from
7577e38
to
dc18eea
Compare
|
Removed dist/index.js. Thanks! |
| var c = new a.constructor(a.length + b.length); | ||
| c.set(a); | ||
| c.set(b, a.length); | ||
| return c; |
There was a problem hiding this comment.
so basically this avoid us from referencing Buffer, but giving us buffer compatibility ? or will take in buffer and spit back a typed array?
I can imagine a.constructor to be always the Uint8Array, while the b.constructor can be both Buffer and Uint8Array?
I'm asking because i played very little with buffers in node.
There was a problem hiding this comment.
I m also wondering about ie11 compatibility here. the new x.constructor never works for me in ie11., this i will test.
There was a problem hiding this comment.
Ah, you're right about a.constructor always being a Uint8Array. My goal is to maintain whatever type they passed in originally. But I'm remembering now that in node, we wouldn't want to do something equivalent to new Buffer() anyways, since that's deprecated. I'll have to think about this more.
There was a problem hiding this comment.
if we could accept the fact that we mutate the original Buffer we could just use .set on the original object for the new head.
Do you know if Buffer.slice returns a copy or a reference to the data? if it returns a copy we can just copy the argument that comes and then set the new head over it, and return it.
Did not try it yet, so i m still throwing ideas out
There was a problem hiding this comment.
It returns a view into the original buffer. Mutating it will mutate the original.
There was a problem hiding this comment.
Well mutation approach would not work with PNG since the header becomes bigger if the phys chunk is not there.
|
Hi @bmathews let the dist unbuilt. i will go up with the version bump. We have still that Buffer question to solve. Monday i ll consult with someone since i do not run this library alone. Did you get some idea meantime? |
|
I was chatting about this a bit. What do you think? |
|
+1 |
|
i guess we can do something for this |
|
Is this PR going to be merged? |
|
hi @nateisleychamp i have to admit i forgot about it. |
|
Would appreciate this too, to avoid converting ArrayBuffer --> Blob --> ArrayBuffer in Node. (Edit: I ended up using bmathews's version directly as I couldn't get Blob polyfill to work. Works like a charm. 👍) |
|
Which pull request will now be merged? @bmathews ? And where is the readDPI method? 😉 |
|
Hello i lost access to this repo for a while, but i should be able to fix things now. |
This adds support for using buffers directly instead of FileReader/Blobs.