Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some improvements in the handling of headers and 16-/24-/32-bit data #19

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alecmev
Copy link

@alecmev alecmev commented May 8, 2018

Lots of changes, but most of them are just simplifications.

The major ones are the automation of extra header field detection and more spec-compliance in data parsing. For example, the alpha mask actually isn't included in BITMAPINFOHEADER / BITMAPV2INFOHEADER headers, unless asked by BI_ALPHABITFIELDS, unlike what is assumed in the current master (this is what made me fix this library, since I had to deal with images which were crashing because of this). Another improvement is the mask handling, especially so in the 16-bit mode: it's completely universal now, and there's no need in the 15-bit mode anymore, nor in is_with_alpha.

Also added a toRGBA option, switching the output to big endian RGBA, making it compatible with other libraries, like pngjs.

Using the Wikipedia article as the specification, for the lack of a better centralized doc.

Currently published as @jeremejevs/bmp-js, for those who need this ASAP.

@mooyoul
Copy link

mooyoul commented Sep 4, 2018

Hey @jeremejevs, Thanks for your amazing work!

I was looking for solution using bitmap as input on sharp.
It can takes raw uint8 rgb pixels, but RGBA sequence was required like pngjs. so the added option you made toRGBA option was just what i was looking for. and worked like charm.

anyway there are some issue still remains,

  • toRGBA option did'nt work on 1/2/4/8/16bit pixel depths
  • default padding / default alpha value - i had to modify these values to 0xff from 0x00 to get expected results (i experienced blue tinted color, empty converted image...)
  • missing typescript support

so i made some improvements based on your work:
https://github.com/balmbees/bmp-js

it resolves descried issues, provides typescript definition. check it out if you need ;)

@mooyoul mooyoul mentioned this pull request Sep 4, 2018
@alecmev
Copy link
Author

alecmev commented Sep 10, 2018

@mooyoul Thanks for picking this up, looks good!

Make sure to put @vingle/bmp-js in the readme, instead of bmp-js, so that people install the right version. Also, the TS interface isn't 100% in-sync with BmpDecoder, there's no is_with_alpha anymore, and it has toRGBA now. And reserved has been split into reserved1 and reserved2.

Honestly, this whole lib could use a rewrite in TS, I didn't do it to save some time (just wanted to get a single BMP-to-PNG test going, no production needs) and to keep the diff to a minimum (which appears to have been a waste of time, still not reviewed/merged).

@hipstersmoothie
Copy link

@jeremejevs I run https://github.com/oliver-moran/jimp/ and there are a bunch of features the community wants from BMP encoding/decoding. Would you like to work together to create a typescript bmp-js?

@hipstersmoothie
Copy link

hipstersmoothie commented Oct 10, 2018

@jeremejevs working here https://github.com/hipstersmoothie/bmp-js/tree/ts. I'm gonna try to integrate some of your code

@hipstersmoothie
Copy link

@jeremejevs I have now integrated your code into the TS bmp-js

@hipstersmoothie
Copy link

@shaozilee would you be open to merging a typescript rewrite? or should i maintain a new version of bmp-js?

@alecmev
Copy link
Author

alecmev commented Oct 11, 2018

@hipstersmoothie Hi, I think I wouldn't mind working on this, but we don't depend on this module in production at work, so there's no incentive to invest billed hours into it, and I'm currently focused on other things in my free time, so sorry, it's a no at this time.

I see that you went ahead and have done it yourself in a couple of hours though, so no loss 😛 I've taken a quick glance at your branch, and there are a few things I would refactor more radically, but seems good otherwise! Why didn't you base it on @mooyoul's fork though?

@hipstersmoothie
Copy link

@jeremejevs ooops. guess I'm gonna loose another few hours. I'll probably make a PR there.

@alecmev
Copy link
Author

alecmev commented Oct 11, 2018

Oh, it's not a big diff, but yeah, it does have a few useful fixes.

@hipstersmoothie
Copy link

@jeremejevs I'd appreciate it if you could take a look at https://github.com/hipstersmoothie/bmp-js. I have implemented encoding at all the bit sizes. Anything is helpful!

@alecmev
Copy link
Author

alecmev commented Oct 15, 2018

@hipstersmoothie Skimmed through the decoder, looks alright! One quick suggestion I have is to inline parseHeader and parseRGBA into the constructor, that way you'll be able to avoid doing the non-null assertion on most of the properties, I believe. Also, I think it's safe for you to remain in your own fork, you're clearly the most active of the four of us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants