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

IMAGE: Improve BMP support #3127

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

Conversation

@lolbot-iichan
Copy link
Contributor

@lolbot-iichan lolbot-iichan commented Jul 3, 2021

  1. Allow various DIB headers (Windows v1-v5), see https://en.wikipedia.org/wiki/BMP_file_format#DIB_header_(bitmap_information_header) and fix loading palette for formats with extended DIB header
  2. Allow 4bpp BMP images and fix pitch calculation for 4bpp
  3. Extend raw decoder to support BMPs with alpha channel and use it for BMP compression method = 3
@criezy
criezy approved these changes Jul 3, 2021
Copy link
Member

@criezy criezy left a comment

This looks good to me.
I only have one small doubt with one addition for which I added a comment. It looks correct to me, but might need to be double checked.

@@ -109,6 +109,8 @@ bool BitmapDecoder::loadStream(Common::SeekableReadStream &stream) {
_paletteColorCount = stream.readUint32LE();
/* uint32 colorsImportant = */ stream.readUint32LE();

stream.seek(infoSize - 40, SEEK_CUR);

This comment has been minimized.

@criezy

criezy Jul 3, 2021
Member

This addition makes sense, but it makes me wonder whether I am missing something, or if the v4 BMP (with infoSize = 108) was not working properly before. Since @dreammaster added it in 154ec75, I am guessing he needed it for something and may have tested it, so he might be able to confirm.

This comment has been minimized.

@lolbot-iichan

lolbot-iichan Jul 3, 2021
Author Contributor

@criezy
Since pixel data is read from subStream, the only thing affected by this additional stream.seek() is palette (if present).
So it only matters in case if 8bpp BMPv4 was used. In my tests, exactly those kind of images looked broken before current PR: изображение
However, you are correct, @dreammaster might have been dealing with some special resources that might be broken by this PR.

This comment has been minimized.

@criezy

criezy Jul 3, 2021
Member

It seems more likely that his test case only included images higher than 8bpp.

This comment has been minimized.

@dreammaster

dreammaster Jul 3, 2021
Member

Right. I was using the bitmaps that came with the Nuvie project, and I guess they were/are non-paletted, since they all seemed to render fine afterwards.

This comment has been minimized.

@yuv422

yuv422 Jul 4, 2021
Member

I think the Nuvie BMPs are paletted non-compressed IIRC.

This comment has been minimized.

@lolbot-iichan

lolbot-iichan Jul 4, 2021
Author Contributor

@yuv422 Paletted BMPv1 (40 bytes header) images are not affected by this change as well.
Could you please point the location of Nuvie BMPs so that I could check their types?

This comment has been minimized.

@bluegr

bluegr Jul 11, 2021
Member

@lolbot-iichan The Nuvie BMPs are here:
https://github.com/nuvie/nuvie/tree/master/data
Not sure where they are located in ScummVM

This comment has been minimized.

@mduggan

mduggan Jul 11, 2021
Contributor

The same files are in devtools/create_ultima/files/ultima6 in the ScummVM tree

@bluegr
Copy link
Member

@bluegr bluegr commented Jul 17, 2021

@lolbot-iichan any news on this?

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