On Wed, 31 Oct 2007 06:48:18 +0000, [EMAIL PROTECTED] wrote:
> i am new to python and PIL and was trying to write a class to read and
> write RGB color model jpeg images..
PIL already offers such a class.
> i came up with this class below..i want to know if this is the way to
> read/write the RGB color model jpeg..if anyone can give feedback (esp
> on the methods writeImage1( ) and writeImage2( ) it wd help my
> studies ..
Those are bad names. What does 1 and 2 mean in them!?
> class JPGFile:
> def __init__(self,filename):
> self._filename=filename
> self._height=0
> self._width=0
> self._mode=""
This will always be 'RGB' so why store it?
> self._rgblist=[]
> self._pixellist=[]
> self._pixarray=None
In the end you store the image *three times* in different representations.
Is that really necessary?
> self.readImage()
Is `readImage()` supposed to be called again later? If not you might add
an underscore in front and give the file name as argument. Then you could
drop `self._filename`.
> def getheight(self):
> return self._height
> def getwidth(self):
> return self._width
>
> def getpixellist(self):
> return self._pixellist
>
> def getrgblist(self):
> return self._rgblist
Maybe get rid of those trivial getters and the underscore in front of the
names. That's a little "unpythonic". Do a web search for "Python is not
Java".
> def makepixval(self,(r,g,b)):
> alpha=255
> pixval=(alpha<<24)|(r<<16 )|( g<<8) | b
> return pixval
Looks like an ordinary function to me. `self` is not used.
> def readImage(self):
> im=Image.open(self._filename)
> self._mode=im.mode
> self._width,self._height=im.size
> for y in range(self._height):
> for x in range(self._width):
> r,g,b=im.getpixel((x,y))
> self._rgblist.append((r,g,b))
> pval=self.makepixval((r,g,b))
> self._pixellist.append(pval)
> self._pixarray=array(self._pixellist,float)
Iterating over `im.getdata()` should be simpler and faster than the nested
loops.
> def makergb(self,pixval):
> alpha=pixval>>24
> red=(pixval>>16 )&255
> green=(pixval>>8 )&255
> blue=(pixval)& 255
> return red,green,blue
Again a simple function.
> def writeImage1(self,fname,data,width,height):
> imnew=Image.new(self._mode,(width,height))
> newrgblist=[]
> for i in range(len(data)):
> r,g,b=self.makergb(data[i])
> newrgblist.append((r,g,b))
If you have ``for i in range(len(something)):`` you almost always really
wanted to iterate over `something` directly. That loop can be written as:
for pixval in data:
rgb = pixval2rgb(pixval)
rgbs.append(rgb)
Or even shorter in one line:
rgbs = map(pixval2rgb, data)
If the `numpy` array is what you want/need it might be more efficient to do
the RGB to "pixval" conversion with `numpy`. It should be faster to shift
and "or" a whole array instead of every pixel one by one in pure Python.
Ciao,
Marc 'BlackJack' Rintsch
--
http://mail.python.org/mailman/listinfo/python-list
