Page MenuHomePhabricator

tga loader: implement handling of palette
ClosedPublic

Authored by nyir on Thu, Nov 7, 11:22 AM.

Details

Reviewers
kwo
Summary

implemented 8 bit color mapped images with 24/32bpp palette.

This is coming from an issue on feh to support indexed TGA images. Written by rofl0r, I'm forwarding it because I already have the tools for submission set up.

N.b. I'm not terribly familiar with the particular code style, if there's anything to format differently I'm happy to update.

Test Plan
  • Compared example file with known good in GIMP, looks okay.
  • Tested that said example file doesn't load without this patch (in feh, which uses Imlib2).

Diff Detail

Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14495
Build 9970: arc lint + arc unit
nyir requested review of this revision.Thu, Nov 7, 11:22 AM
nyir created this revision.
nyir edited the summary of this revision. (Show Details)Thu, Nov 7, 11:25 AM
nyir edited the test plan for this revision. (Show Details)
nyir added a reviewer: kwo.
nyir edited the summary of this revision. (Show Details)
kwo added a comment.Fri, Nov 8, 10:49 AM

Looks good to me except that all of the error exits need proper cleanup (munmap(), __imlib_FreeData()).
I have made some cleanups in the tga loader so now you should just goto quit on error.
I have also eliminated the somewhat nasty WRITE_RGBA() macro - please use PIXEL_ARGB instead.
As for indentation - I use indent (version 2.2.12 - matters!), there is an .indent.pro in the top level directory, so you can just do like
$ indent src/modules/loaders/loader_tga.c
But no problem - I'm fine with amending the indent.

Could you please redo your changes (with proper exit handling etc.) on top of mine?

Thanks! :)

nyir updated this revision to Diff 26874.Wed, Nov 13, 2:37 PM
  • implement loading of 8bit palette TGAs
  • Update indentation.
nyir added a comment.Wed, Nov 13, 2:41 PM
In D10618#198592, @kwo wrote:

Could you please redo your changes (with proper exit handling etc.) on top of mine?

Cool, thanks; rofl0r did do so, I'm just forwarding their changes again; I ran indent as well, but committed that as a separate commit because of the IMO questionable change on the line break. Hope this looks good then.

kwo accepted this revision.Fri, Nov 15, 8:17 AM

Pushed (with minor formatting adjustments).
Thanks :)

This revision is now accepted and ready to land.Fri, Nov 15, 8:17 AM
kwo closed this revision.Fri, Nov 15, 8:17 AM