patternpythonMinor

Decoding grayscale PNG and performing Prewitt operator for edge detection

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

performingpngoperatorprewittedgegrayscalefordecodingdetectionand

Problem

I'm a hobbyist programmer.

What I try to do here is to read 8-bit grayscale PNG file. Then unfilter it. (That caused a headache). And after that perform Prewitt operator for "edge detection". I know that there is million and five libs for this but I want to learn why and how these things works.

I try follow OOP-principles, but it is hard.

There are still some features I intend to add, but I'd like a review of the existing code before going further.

So what I want to know:

Do I follow OOP-principles? If not, what to do better? (Link to good book would be amazing.)

Should I use bytearray instead of list? Does it affect performance?

Any comments, pointers etc are more than welcome.

Input and output of program

And code:

PNG.py

```
import sys, binascii, math
from Chunk import *
from Filter import Filter
import time
class PNG:

def __init__(self, f):
self.data = []
self.f = open(f, "rb")
self.mod = sys.modules[__name__]

def readFile(self):
f = self.f
try:
self.head = f.read(8)
while True:
length = f.read(4)
if length == b'':
break
try:
c = getattr(self.mod, f.read(4).decode()) #Class
except Exception as e:
print("Chunk is not implemented yet")
raise
found = [(i, x) for (i,x) in enumerate(self.data) if isinstance(x, c)]
if len(found) > 0:
index, obj = found[0]
obj.append(f.read(int.from_bytes(length, byteorder='big')))
else:
i = c(data=f.read(int.from_bytes(length, byteorder='big'))) #Read data
self.data.append(i)
f.read(4)
finally:
f.close()

def getKernels(self, pixels, width):
kernels = []
pixels = [x for sublist in pixels for x in sublist]
for i, px in e

Solution

Style

Read PEP 8, especially the parts about whitespace and variable naming. You also happen to use super() in various places in Chunk.py (especially super().getData()) but most of these calls can simply use self. instead since you don't override these methods in your class.

There is also various if ...: raise in your code: don't do that, it's a bug. You will run into a RuntimeError: No active exception to reraise. In some sense it works since it generate an exception, but in the end, it's not the one you want. raise alone can only be used in except clauses.

You should raise a specific exception instead. It seems that every time you have this pattern, you print(' not implemented yet') before; thus you should raise NotImplementedError instead of these two lines.

Lastly, you should familiarize yourself with the truthy values of Python's objects: empty containers and numbers representing 0 are False, everything else is True by default. So instead of if len(found) > 0:, write if found:; instead of if x == 0: write if not x:, etc.

OOP

Many of the methods on your objects does not make use of the self parameter. Or only to call other methods. This often indicates that OOP is not required there and you may use regular functions instead.

Moreover, the PNG class seems disturbing. First off, since you can not perform any operation before reading the file, you should merge __init__ and readFile; this will also allow you to use the with statement to deal with the file (kuddos for using finally: f.close(), btw) for you. Second, I trully feel that your class hierarchy is the wrong one. Why is it the responsibility of the PNG class to perform image transformation? The day you’ll want to support JPEG format, will you repeat the same code for edge detection? Is the Chunk class usefull for something else than PNG?

Instead, you should provide classes that knows about file formats: how to read them, how to write to them and how to extract pixels blobs out of them. And provide utility functions to modify the pixels blobs returned by such classes. This will also allow you to remove this weird hack using sys.modules.

Sketching this new layout would turn your code into something like:

PREWITT_X_MASK = [-1, 0, 1, -1, 0, 1, -1, 0, 1]
PREWITT_Y_MASK = [-1, -1, -1, 0, 0, 0, 1, 1, 1]

def get_kernels(pixels, width, radius=1):
    for y in range(len(pixels)):
        y_min = y - radius
        y_max = y + radius + 1
        for x in range(width):
            x_min = x - radius
            x_max = x + radius + 1
            yield [
                pixel for row in pixels[y_min:y_max]
                for pixel in row[x_min:x_max]]

def prewitt_operator(kernel):
    if len(kernel) != 9:
        return 0

    gx = sum(i*j for i, j in zip(kernel, PREWITT_X_MASK))
    gy = sum(i*j for i, j in zip(kernel, PREWITT_Y_MASK))
    return int((gx**2 + gy**2)**(1/2)) % 256

def edge_detect(image):
    pixels = image.pixels
    width = image.width

    detected = [prewitt_operator(kernel) for kernel in get_kernels(pixels, width)]
    new_image = [detected[i*width:(i+1)*width] for i in range(len(detected)//width)]
    image.pixels = new_image

For the image detection part. And you would use this code like:

image = PNG('input.png')
edge_detect(image)
image.write('output.png')

And voilà. The whole logic of reading the file, converting its data into a list of list of pixels and converting back a list of list of pixels should be hidden in the PNG class. And it will be much easier to provide alternate image format support like JPEG.

The PNG class would look a bit like:

class PNG:
    def __init__(self, filename):
        self.name = filename
        with open(filename, 'rb') as f:
            # process f and store into self.data
            # combines your PNG.__init__ and PNG.readFile
            # and helper methods on the class to create the
            # equivalents of your chunks objects directly as
            # attributes:
            #   self.width, self.height, self.bit_depth, self.color_type ...

    @property
    def pixels(self):
        # process self.data and return a list of lists of pixels
        # combines your IDAT.getImage + Filter.unfilter

    @pixels.setter
    def pixels(self, new_image):
        # process new_image to store into self.data

    def write(self, filename=None):
        if filename is None:
            filename = self.name

        with open(filename, 'wb') as f:
            # write back metadata and self.data into f

    # other utility methods

The @propertys allowing for easy, attribute like, retrieval or update of data, as shown in edge_detect.

Miscelaneous

-
You can feed a whole list at once to bytes:

>>> bytes([72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 33])
b'Hello World!'

-
Your use case of math can easily be simplified using ** (exponentiation) or // (integral division) for instanc

Code Snippets

PREWITT_X_MASK = [-1, 0, 1, -1, 0, 1, -1, 0, 1]
PREWITT_Y_MASK = [-1, -1, -1, 0, 0, 0, 1, 1, 1]


def get_kernels(pixels, width, radius=1):
    for y in range(len(pixels)):
        y_min = y - radius
        y_max = y + radius + 1
        for x in range(width):
            x_min = x - radius
            x_max = x + radius + 1
            yield [
                pixel for row in pixels[y_min:y_max]
                for pixel in row[x_min:x_max]]


def prewitt_operator(kernel):
    if len(kernel) != 9:
        return 0

    gx = sum(i*j for i, j in zip(kernel, PREWITT_X_MASK))
    gy = sum(i*j for i, j in zip(kernel, PREWITT_Y_MASK))
    return int((gx**2 + gy**2)**(1/2)) % 256


def edge_detect(image):
    pixels = image.pixels
    width = image.width

    detected = [prewitt_operator(kernel) for kernel in get_kernels(pixels, width)]
    new_image = [detected[i*width:(i+1)*width] for i in range(len(detected)//width)]
    image.pixels = new_image

image = PNG('input.png')
edge_detect(image)
image.write('output.png')

class PNG:
    def __init__(self, filename):
        self.name = filename
        with open(filename, 'rb') as f:
            # process f and store into self.data
            # combines your PNG.__init__ and PNG.readFile
            # and helper methods on the class to create the
            # equivalents of your chunks objects directly as
            # attributes:
            #   self.width, self.height, self.bit_depth, self.color_type ...

    @property
    def pixels(self):
        # process self.data and return a list of lists of pixels
        # combines your IDAT.getImage + Filter.unfilter

    @pixels.setter
    def pixels(self, new_image):
        # process new_image to store into self.data

    def write(self, filename=None):
        if filename is None:
            filename = self.name

        with open(filename, 'wb') as f:
            # write back metadata and self.data into f

    # other utility methods

>>> bytes([72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 33])
b'Hello World!'

def sanitize_along_x(x, y, data):
    return 0 if not x else data[y][x - 1]

def sanitize_along_y(x, y, data):
    return 0 if not y else data[y - 1][x]

def paeth(x, y, data):
    a = sanitize_along_x(x, y, data)
    b = sanitize_along_y(x, y, data)
    c = 0 if not (x and y) else data[y - 1][x - 1]
    # p = a + b - c  ## Was it right and not p = a + b + c ???
    pa = abs(b - c)
    pb = abs(a - c)
    pc = abs(a + b - 2 * c)
    if pc => pa <= pb:
        return a
    if pb <= pc:
        return b
    return c


class PNG:
    UNFILTER_METHOD = [
        lambda *nil: 0,
        sanitize_along_x,
        sanitize_along_y,
        lambda x, y, data: (sanitize_along_x(x, y, data) + sanitize_along_y(x, y, data)) // 2,
        paeth,
    ]

    @property
    def pixel(self):
        # prepare self.data according to IDAT.getImage
        unfiltered = []
        for y, row in enumerate(self.data):
            filter_method, *data = row
            current_row = []
            unfiltered.append(current_row)
            for x, pixel in enumerate(data):
                px = self.UNFILTER_METHOD[filter_method](x, y, unfiltered)
                current_row.append((px + pixel) % 256)
        return unfiltered

    # rest of the class

Context

StackExchange Code Review Q#147747, answer score: 3

Revisions (0)

No revisions yet.