Molly Rocket :: View topic - public domain jpeg/png decompressor

Molly Rocket
Molly Rocket games and everything even tangentially related

FAQ

Usergroups

public domain jpeg/png decompressor
Goto page Previous 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 Next

Molly Rocket Forum Index -> Free Libraries and Tools

View previous topic :: View next topic

Author

Message

tom_seddon

Joined: 30 Oct 2005
Posts: 23
Location: sheffield, england

	Posted: Sat Sep 12, 2009 1:22 am Post subject:

I added in SoftImage PIC format reading support to my copy. If you're interested in this, please find modified stb_image.c and unified diff patch (are you into that sort of thing?) here: http://www.tomseddon.plus.com/.stb_image/ Whilst I was there, I also added an extra const to the TGA and BMP save functions. I think this would be a useful change and can't think of any problems it would cause for people that don't care about this stuff. Unfortunately, I only have a few PIC files, and don't have any programs that make them. However, it works fine for the ones I have... _________________ --Tom

sean

Joined: 01 Feb 2005
Posts: 1392
Location: Kirkland WA

Posted: Sat Sep 12, 2009 2:56 pm Post subject:

tom_seddon wrote:

I added in SoftImage PIC format reading support to my copy. If you're interested in this, please find modified stb_image.c and unified diff patch (are you into that sort of thing?) here:

http://www.tomseddon.plus.com/.stb_image/

Sure, thanks, as long as you're ok with your code becoming public domain.

One question...

Code:

*comp = bitcount(act_comp & 0xF0);//erm...

Care to explain the comment?

tom_seddon

Joined: 30 Oct 2005
Posts: 23
Location: sheffield, england

Posted: Sat Sep 12, 2009 4:17 pm Post subject:

sean wrote:

Sure, thanks, as long as you're ok with your code becoming public domain.

Yup, no problem.

Quote:

One question...

Code:

*comp = bitcount(act_comp & 0xF0);//erm...

Care to explain the comment?

Hmm... I suspect I was thinking about your comment "if you set req_comp to 4, you will always get RGBA output, but you can check *comp to easily see if it's opaque.", which wouldn't be quite true for a (say) RGA PIC file. You'd get comp set to 3 for that, because it's just counting the components. Do you think this behaviour is an issue?

By the way looking at my code with this in mind the "memset(result,0xFF,*x**y*4);" in pic_load should be replaced with something more like this:

Code:

for (i = 0; i < *x * *y * 4; i += 4) {
result[i + 2] = result[i + 1] = result[i + 0] = 0;
result[i + 3] = 255;
}

This starts the image out as (0,0,0,255) so if you did have this strange RGA file then all the B components would be set to 0 rather than (as before) 255. I now realise I should probably have written the code so it goes through the image to set the components that weren't in the file AFTER loading... but I didn't :(
_________________
--Tom

sean

Joined: 01 Feb 2005
Posts: 1392
Location: Kirkland WA

Posted: Sat Sep 12, 2009 5:37 pm Post subject:

tom_seddon wrote:

I believe this is a problem because you expand the image to RGBA then convert the image to req_comp.

Let's say the file is RGA, so there are 3 components. Now, if your client code knew what it was doing, you might be happy to just get back a 3-channel image where the image was R, G, A. In that case you could report a comp of 3 and return those three channels, RGA, consecutively. I'm not sure it's a good idea, but you could do it. But the code doesn't let you do that.

The important part is that you decode the image to a temporary RGBA buffer and then report '4' as the number of channels when you call convert_format(). This means nothing in there breaks if you have an explicit req_comp--nothing ever uses the value of *comp you compute.

But there are two failure cases.

One, if the client specifies a value of 0 for req_comp, then *comp should have the number of channels returned, but here the number of channels returned will always be 4 (since that's what you hand to convert_format), but *comp may be something else.

Two, there's no actual way to leverage that value of *comp. If the file is RB, and reports *comp of 2, then if you set req_comp to 2, you'll decode R*B* into a 4-channel buffer, call convert_format with req_comp=2. convert_format interprets '2' as Luminance + Alpha, so it will compute a weighted average of RGB for one channel, and return alpha for the other channel. So you'll end up getting back something approximately like <(R+B)/4,255> for the RB file.

To do it "right" you'd have to change the API from the simple mapping for channel counts of 1,2,3,4, but I don't think it's worth it. So I think the right thing to do is detect alpha or not and only allow the return values of 3 (RGB) and 4 (RGBA), and then do the appropriate juggling in calling convert_format to get the right result.

If that makes sense I'll code it.

sean

Joined: 01 Feb 2005
Posts: 1392
Location: Kirkland WA

	Posted: Sat Sep 12, 2009 5:48 pm Post subject:

http://nothings.org/misc/stb_image_pic_test.c Let me know if it works; I don't have any pic files to test with. If you diff it against your code, you'll see a lot of whitespace/formatting changes, but no real changes other than a couple of lines to fix the thing I said above.

tom_seddon

Joined: 30 Oct 2005
Posts: 23
Location: sheffield, england

Posted: Sat Sep 12, 2009 6:54 pm Post subject:

OK, thanks for looking at this! The changes work fine, and the TGAs saved out by my test program are byte for byte identical to what they were before.

I agree with your suggestion just to support RGB/RGBA. So one minor change for that I think: the top 3 bits of the components bitmask encode presence or absence of R, G and B, so I guess they should be checked. So the right thing would then be to replace this:

Code:

*comp = (act_comp & 0x10 ? 4 : 3); // has alpha channel?

with this (tweak comment to taste of course):

Code:

// Fail if it isn't exactly RGB or RGBA. Arbitrary sets of components will
// load fine, but won't necessarily interact with convert_format properly.
// I'll deal with this when/if I need it.
if (act_comp == (0x80 | 0x40 | 0x20)) *comp = 3;//RGB
else if (act_comp == (0x80 | 0x40 | 0x20 | 0x10)) *comp = 4;//RGBA
else return epuc("bad format", "PIC data is not RGB/RGBA");

Hopefully I got the formatting right this time.

Complete copy uploaded again:

http://www.tomseddon.plus.com/.stb_image/stb_image.c

Also are you interested in any patches to fix warnings when compiling with warning level 4 under VC++2005? I currently have a few #pragma warnings to get rid of the common ones, which is a bit annoying (though I'll live :). Some of them would require some extra casts and so on I should think -- e.g., conversion from stbi_uc to int -- so I could see that you might prefer it the way it is (in which case I'll just leave it).
_________________
--Tom

ryg

Joined: 31 May 2007
Posts: 276
Location: Kirkland, WA

	Posted: Sat Sep 12, 2009 9:02 pm Post subject:

We do use PIC quite a lot where I work (our artists use XSI), and the artists I worked with on demoscene related stuff also happened to prefer XSI, so I've had some exposure to the format. I've never encountered a PIC file that didn't store all of the R, G and B channels. In fact, there are only two combinations I've seen used: 1. 1 packet, RGB 2. 2 packets, 1st RGB, 2nd A Our PIC reader at work only supports these two and I've never seen it barf. Judging by this, it seems pretty likely that XSI has the same restriction ("Channel codes"). Haven't tested it, but I'm willing to bet the same goes for the Photoshop plugin too. In other words, don't worry about it. (Normally I'd give you a few example images for testing, but I don't have any on this machine and I'll be leaving for vacation tomorrow.)

sean

Joined: 01 Feb 2005
Posts: 1392
Location: Kirkland WA

Posted: Sat Sep 12, 2009 10:00 pm Post subject:

tom_seddon wrote:

I agree with your suggestion just to support RGB/RGBA. So one minor change for that I think: the top 3 bits of the components bitmask encode presence or absence of R, G and B, so I guess they should be checked.

I think it makes more sense to return the client bogus RGB values than to fail to load it, although I guess ryg's comment implies that we could assume it's a corrupt/invalid PIC in that case.

Actually we could pretty easily handle alpha-only PICs, which would be worth it if we thought anyone created them. But I think from ryg's comment the version I posted is actually in the sweet spot (in the sense of behaving correctly with the apparent official definition while being reasonably behaved for any file that follows the actual spec but exceeds that). Maybe change the initialization to 0,0,0,255 like you said before.

Quote:

Hopefully I got the formatting right this time.

You don't have to worry about that, I don't mind reformatting the code, it gives me an excuse to read through it and understand it anyway.

Quote:

Also are you interested in any patches to fix warnings when compiling with warning level 4 under VC++2005?

Yes, feel free. I used to hate doing this but in the long run it makes far more sense to treat this like 'const' and centralize the fix rather than make people who want it go through extra work.

jetro

Joined: 02 May 2009
Posts: 3

	Posted: Tue Sep 22, 2009 1:00 pm Post subject:

The PSD support could be even nicer if it would support extracting the layer images (pre-rendered by photoshop when saving with compatibility mode). Naturally this would need some API changes for stb_image though. DrPetter has made a PSDImage class which extracts that stuff, and to test it I made a little tool with to extract layers and some meta info and output each layer to a separate image - available here: http://jet.ro/2009/09/09/tool-psdlayerstga/ direct link to zip: http://jet.ro/files/psdlayerstga.zip Maybe DrPetter's PSDImage class features could be integrated to stb_image?

sean

Joined: 01 Feb 2005
Posts: 1392
Location: Kirkland WA

	Posted: Tue Sep 22, 2009 6:28 pm Post subject:

I'm not sure what the best way to handle something like that would be. Since it would need a custom API that would pretty much only ever be appropriate for PSD, maybe it would make more sense to be a separate PSD-reading library? But in that case, maybe the existing one is good enough.

jetro

Joined: 02 May 2009
Posts: 3

	Posted: Sun Nov 22, 2009 8:53 pm Post subject:

For a moment I thought I'd need the stbi_info_* stuff for a tool, until I realized that the amount of data still probably won't be significant enough to care about loading it all in memory at once or not. But meanwhile I implemented the stbi_info stuff for three formats: png, jpeg, tga. I also noticed that the internal tga_test uses get16 instead of get16le to check the image size which is a bug. The additional code is here and it's ok to put in public domain if you want: http://jet.ro/files/stb_image_ex.c Note that I didn't modify my copy of stb_image (thinking first that my additions might be something which won't get into the lib and wanting to have easy upgrade of stb_image itself by copying over). Which is why the stuff is in a different file, and that's also reason why some of the stuff has copy-paste although it would have been more compact/elegant to modify the stb_image code itself a bit, or something.

sean

Joined: 01 Feb 2005
Posts: 1392
Location: Kirkland WA

	Posted: Sun Nov 22, 2009 9:25 pm Post subject:

Cool! It's kind of lame I never got around to it given how little source code it turns out to be... Thanks for spending the time to work it all out. I will integrate it this week.

memon

Joined: 04 Sep 2006
Posts: 13

Posted: Wed Feb 03, 2010 3:34 pm Post subject:

I just happened to run the stb_image.c through xcode's new static code analysis, as stb_image happens to be part of a project of mine, and I got this results:

Code:

stb_image.c:2821:31 Although the value stored to 'z' is used in the enclosing expression, the value is never actually read from 'z'
stb_image.c:2898:13 Value stored to 'mr' is never read
stb_image.c:2898:18 Although the value stored to 'mg' is used in the enclosing expression, the value is never actually read from 'mg'
stb_image.c:2898:23 Although the value stored to 'mb' is used in the enclosing expression, the value is never actually read from 'mb'
stb_image.c:2905:19 Value stored to 'fake_a' is never read
stb_image.c:3328:40 Although the value stored to 'tga_palette_bits' is used in the enclosing expression, the value is never actually read from 'tga_palette_bits'
stb_image.c:3329:4 Although the value stored to 'tga_x_origin' is used in the enclosing expression, the value is never actually read from 'tga_x_origin'
stb_image.c:3329:19 Although the value stored to 'tga_y_origin' is used in the enclosing expression, the value is never actually read from 'tga_y_origin'
stb_image.c:3328:2 Value stored to 'tga_palette_start' is never read
stb_image.c:3126:6 Value stored to 'tga_x_origin' during its initialization is never read
stb_image.c:3127:6 Value stored to 'tga_y_origin' during its initialization is never read
stb_image.c:3328:22 Although the value stored to 'tga_palette_len' is used in the enclosing expression, the value is never actually read from 'tga_palette_len'
stb_image.c:3529:5 Value stored to 'count' is never read
stb_image.c:3612:8 Value stored to 's' during its initialization is never read

Some of them are actually unused values, take with grain of salt.

sean

Joined: 01 Feb 2005
Posts: 1392
Location: Kirkland WA

Posted: Thu Feb 04, 2010 10:47 am Post subject:

I do llike to fix compiler warnings, but this is kind of problematic. (I guess these aren't normally produced when compiling, fortunately?) Not only are most of these are false alarms, but some of them are necessary to suppress warnings in other compilers. And in fact it shows how easily tools can miss aspects of programs that are in there for human readers not for the compiler.

Consider the first one:

memon wrote:

stb_image.c:2821:31 Although the value stored to 'z' is used in the enclosing expression, the value is never actually read from 'z'

I believe that is this:

Code:

// returns 0..31 for the highest set bit
static int high_bit(unsigned int z)
{
int n=0;
if (z == 0) return -1;
if (z >= 0x10000) n += 16, z >>= 16;
if (z >= 0x00100) n += 8, z >>= 8;
if (z >= 0x00010) n += 4, z >>= 4;
if (z >= 0x00004) n += 2, z >>= 2;
if (z >= 0x00002) n += 1, z >>= 1;
return n;
}

The issue here is the last "z >>= 1". Obviously you could delete it since it has no effect... but I think it's clearer with it included.

Although now that I think about it, since it will leave z as either 0 or 1, I could actually initialize n to negative 1, and then at the very end add z to n and return that, and then remove the initialize explicit test for z of 0. I don't think that code would necessarily be clearer, though.

Also, if I've correctly determined which code it is, it means the "used in the enclosing expression" is actually wrong or at least misleading.

Quote:

stb_image.c:2898:13 Value stored to 'mr' is never read
stb_image.c:2898:18 Although the value stored to 'mg' is used in the enclosing expression, the value is never actually read from 'mg'
stb_image.c:2898:23 Although the value stored to 'mb' is used in the enclosing expression, the value is never actually read from 'mb'

These appear to be because I use a variable like mg in two different branches of an if, but I declare it before the if so I initialize it to 0, but both paths set it explicitly so that 0 is never visible. So this warning wants me to *not* always initialize my variables.

I guess this fits with e.g. Java where you don't need to initialize iff a certain process proves they're always initialized. But this is C, not Java; there are some compilers that will produce warnings if the variables aren't initialized. Thus I'm doubtful that this report is actually useful if you want to have cross-platform code.

Quote:

stb_image.c:2905:19 Value stored to 'fake_a' is never read

Yeah, this is a value that is setup and there's an "@TODO" for it to actually do something with it, but the code isn't written yet.

Quote:

stb_image.c:3328:40 Although the value stored to 'tga_palette_bits' is used in the enclosing expression, the value is never actually read from 'tga_palette_bits'
stb_image.c:3329:4 Although the value stored to 'tga_x_origin' is used in the enclosing expression, the value is never actually read from 'tga_x_origin'
stb_image.c:3329:19 Although the value stored to 'tga_y_origin' is used in the enclosing expression, the value is never actually read from 'tga_y_origin'
stb_image.c:3328:2 Value stored to 'tga_palette_start' is never read
stb_image.c:3328:22 Although the value stored to 'tga_palette_len' is used in the enclosing expression, the value is never actually read from 'tga_palette_len'

This appears to be complaining about code at the end of a function that reassigns some variables that were earlier assigned but never read (and yet not complained about by xcode?) in an attempt to suppress one particular compiler's warnings. (Not actually my code, so I dunno, that's what the comments say.)

Oh wait, here's the earlier assigned but never read:

Quote:

stb_image.c:3126:6 Value stored to 'tga_x_origin' during its initialization is never read
stb_image.c:3127:6 Value stored to 'tga_y_origin' during its initialization is never read

So yeah, I think all the ones up to here are bogus.

Quote:

stb_image.c:3529:5 Value stored to 'count' is never read

If this is the second case of the psd load, that looks like an actual unused (unnecessary) assignment. (But not actually buggy.)

Quote:

stb_image.c:3612:8 Value stored to 's' during its initialization is never read

If this is hdr_gettoken(), looks like an actual unused (unnecessary) variable.

Although that error message is busted--you should get that error if the variable is initialized, the initial value is never read, and the variable does get used later. For a variable that's initialized and never read at all, which is the case here, it should generate some kind of unused-variable message instead.

memon

Joined: 04 Sep 2006
Posts: 13

	Posted: Fri Feb 05, 2010 7:45 am Post subject:

Those warnings or "notes" were coming from static code analysis. I think the latest xcode uses the clang static analyzer. I have noticed the same thing that machines do not catch all the subtleties programs, let it be static code analyzer or really pedantic warnings on your compiler.

Display posts from previous:

	Molly Rocket Forum Index -> Free Libraries and Tools	All times are GMT Goto page Previous 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 Next
Page 8 of 10

You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum