[PATCH] New image loader (Softimage PIC)

[Aaron J. Seigo]

On Thursday 30 August 2007, Ruben Lopez wrote:
> Ingo Klöcker wrote:
> > On Tuesday 28 August 2007 09:42, Bongani Hlope wrote:
> >> The code bellow should be called convertABGRtoRGA, becaus that is
> >> what it does. In RGBA, red is the highest nibble so to get red you
> >> need  (pixel >> 24) & 0xFF
> >>
> >> inline unsigned convertRGBAtoABGR(unsigned pixel)
> >> {
> >> 	unsigned char r = pixel & 0xFF;
> >> 	unsigned char g = (pixel >> 8) & 0xFF;
> >> 	unsigned char b = (pixel >> 16) & 0xFF;
> >> 	unsigned char a = (pixel >> 24) & 0xFF;
> >> 	return a | (b << 8) | (g << 16) | (r << 24);
> >> }
> >
> > Don't be fooled by the completely irrelevant naming of the temporary
> > variables. This method converts RGBA to ABGR and vice versa. So it's
> > pretty much irrelevant how the method is called. Both names would be
> > equally correct and equally wrong.
> >
> > I am much more concerned about the usage of the arithmetic shift
> > operator on variables of type unsigned char. A char is 8 bit wide so
> > for example b << 8 might be 0 for some compilers/on some architectures.
> > Or does char always expand to a 32-bit integer when its bits are
> > shifted? I guess the answer is "It _should_ on 32+-bit architectures."
> >
> > I have observed a weird problem with a few signed chars being casted to
> > float and back which yielded different results in debug and release
> > builds with vc7, so I wouldn't be surprised if the above would still
> > break for some compiler.
>
> I have changed these unsigned char to unsigned. This should fix the
> problem on most architectures. Anyway, when integrating this code with
> kdelibs or kdegraphics configure (CMake?) system, maybe I could use more
> portable types (like uint32 or something like that).

for Qt code you can use quint32 and friends, yes =)

> avoid warnings in VC. Well, it is also readed from the QIODevice, but I
> read the whole PICHeader structure as a block. Can this yield to endian
> problems?

you mean in picReadHeader? that looks ok to me, as you use ntoh* to pull the 
values out of the file and into the in-memory variable..

> As soon as I can test the new code (I miss scons on this machine) I will
> send the new version. I have also cleaned it up for dead code and
> spanish comments...
>
> Btw, is there any doc online explaining the procedure for submitting a
> patch for kdelibs or kdegraphics?

you're doing the right thing right here =)

> If submitted to kdegraphics, what is 
> the right folder for the KImgIO plugins? I have seen a folder for KFile
> plugins, but I miss the other one.

the kfile plugins have been moved to "strigi-analyzer" but the idea is the 
same, i suppose. 

so, i looked at the source tarball a bit more just now (only after having sent 
the last email.. meh, bad aaron, bad!) and i see it's very small and indeed a 
user-invisible plugin. so upon further examination, i'm fine with having it 
in kdegraphics itself, at trunk/KDE/kdegraphics/imageio/pic/. the contents of 
the share/ and src/ directories should both go in there.

it also occurred to me while reading through the tarball that it's kde3 code 
=) kde3 is no longer accepting new features / code, but you could certainly 
offer this version in branches/extragear/kde3/graphics.

going forward, we need to transition it to kde4, which includes:

- the mimetype .desktop file needs to be transitioned to the shared mimetype 
xml[1]; it would be great to move this upstream[2] as well. i could help here 
if needed.

- moving the build to cmake. that's a 5 minute job and i could also do that 
for you if you'd like.

- port and make sure it compiles. this is the part that entails some work, and 
entails:

  - porting kfile_pic.* to a strigi-analyzer. this is pretty easy, and the 
results should go into kdegraphics/strigi-analyzer/pic/ ... it can share the 
pic_read.* files with the imageio plugin
  - rather than kimigio_* factories, this needs to be a QImageIOPlugin and a 
QImageIOHandler needs to be written around pic_read and pic_write. 
essentially, canRead, read and write need to be implemented; there are a 
number of other virtuals that may or may not be interesting. read and write 
should be pretty easy, as they essentially can be ported verbatin from the 
current kimgio_pic_read and kimgio_pic_write.

so ... do you have a kde4 install around? are you interested in / able to do 
the porting (either with assistance or on your own)? it would be cool to get 
this into kde4, though there is a bit of work to do. the majority of the 
work, indeed the hard bits (the format support itself), is already done so 
it's just the final bits to tie up.

[1] 
http://standards.freedesktop.org/shared-mime-info-spec/shared-mime-info-spec-latest.html
[2] http://www.freedesktop.org/wiki/Software/shared-mime-info

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070830/f64e759a/attachment.sig>