jpeg image read class

[Marc 'BlackJack' Rintsch]

On Wed, 31 Oct 2007 06:48:18 +0000, devnew at gmail.com 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