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

WIP: IMAGE: Gif decoder using stb_image #2911

Closed
wants to merge 1 commit into from

Conversation

@mgerhardy
Copy link
Contributor

@mgerhardy mgerhardy commented Apr 4, 2021

Added a GIF decoder needed in LBA1 floppy disk version.

This is wrapping stb_image.h (https://github.com/nothings/stb/blob/master/stb_image.h)

- [ ] Verified functionality in TwinE (Doesn't support palettes - needs conversion)

  • Added unittest for loading a small 2x2 gif buffer

Also see the related PR #2923 (Obviously only one should get merged)

@henke37
Copy link
Contributor

@henke37 henke37 commented Apr 4, 2021

"based on" is an understatement. It's flat out wrapping it. But more importantly, this does not support animated gifs. That would be nice if other engines were to use this feature.

@mgerhardy
Copy link
Contributor Author

@mgerhardy mgerhardy commented Apr 4, 2021

True - would be nice. But I don't need this in TwinE - and I'm currently concentrating on the stuff that I need. This is an explicit check at the moment and it will print out a warning if the gif has more than 1 frame. Are there any engines out there, that need this?

I suppose implementing support for animated mng and gif might also need some kind of api change for the decoders api. This is currently out of scope of this PR.

I tried to add a simple unittest but running make test currently segfaults for me right at the first test. Will investigate this.

@lephilousophe
Copy link
Member

@lephilousophe lephilousophe commented Apr 4, 2021

Animated GIFs are videos more than images.
And they are supported by the module. The API is not really good but they do link a way to do it.

@mgerhardy mgerhardy force-pushed the mgerhardy:pr/gif-decoder branch 3 times, most recently from 214bf0f to dc2e198 Apr 4, 2021
@mgerhardy
Copy link
Contributor Author

@mgerhardy mgerhardy commented Apr 4, 2021

@sev-
Copy link
Member

@sev- sev- commented Apr 5, 2021

@henke37 Why should we care about animated gifs in the first place? Your statement is absolutely out of place.

Also, it is not very pleasant to see thins kind of judgemental expressions. Why are you questioning the choice of words by a developer? Why are you reviewing this at all? We are looking for the team members' feedback, not from the outsiders.

@bluegr
Copy link
Member

@bluegr bluegr commented Apr 5, 2021

Nice work!

The stb_image library supports a lot of functionality that's covered by other libraries, so we should only keep the parts that we need from it. Thankfully, it's possible to just enable the parts that are needed using ifdefs
We've tried using our own implementation for some of these formats in the past, and ended using the respective libraries because of images that were found in games that were using more advanced features. This is also covered in the README:

  Primarily of interest to game developers and other people who can
      avoid problematic images and only need the trivial interface

The rest of the changes are straightforward, and are just fine to merge by me.
Also, we really don't need any support for animated GIFs at this point, so this PR should be OK to proceed without this feature.

Regarding the supported file formats of this library:

  • JPEG: We got a libjpeg / libjpeg-turbo wrapper
  • PNG: We got a libpng wrapper
  • TGA / BMP: We got separate decoders
  • PSD / PNM / HDR: Most probably not necessary for our needs
  • GIF: The main part of this PR
  • PIC: Perhaps these could be used in the future by older 3D games that may be using this format?

Thus, at this point we'll only need the GIF functionality of this library, which is the only part of it that's enabled at the moment.

Overall, clean and simple implementation :) +1 from me

@mgerhardy mgerhardy force-pushed the mgerhardy:pr/gif-decoder branch from dc2e198 to b323f95 Apr 6, 2021
@mgerhardy
Copy link
Contributor Author

@mgerhardy mgerhardy commented Apr 6, 2021

Maybe it would be better to present at least one different PR with libgif - because the stb_image library explicitly converts a palette image into RGBA. This might not be the best in our case - as a lot of games are using palettes.

@mgerhardy mgerhardy mentioned this pull request Apr 6, 2021
6 of 6 tasks complete
@mgerhardy mgerhardy changed the title WIP: IMAGE: Gif decoder WIP: IMAGE: Gif decoder using stb_image Apr 6, 2021
@bluegr
Copy link
Member

@bluegr bluegr commented Apr 10, 2021

The other PR has been merged, where GIF support has been added based on libgif. Thus, this one can be closed now

@bluegr bluegr closed this Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants