Twinside / Juicy.Pixels

Haskell library to load & save pictures
BSD 3-Clause "New" or "Revised" License
238 stars 57 forks source link

Check image width on writePixel #181

Open Elewyth opened 4 years ago

Elewyth commented 4 years ago

writePixel on a MutableImage seems to be intended as an abstraction of the underlying color (Pixel) implementation, as well as the coordinate mapping from (x, y) to a single integer index.

In my opinion, it seems unintuitive that writePixel does not check whether the supplied x coordinate exceeds the image's width. This usually does not even cause an error, but simply (due to the way mutablePixelBaseIndex is implemented) writes the pixel in a later row. Supplying a too large y always causes an out-of-bounds error (as expected).

This snippet reproduces the "x overflow":

img <- createMutableImage 50 100 (PixelRGB8 255 0 0)
writePixel img 25 25 (PixelRGB8 0 255 0)
writePixel img 75 25 (PixelRGB8 0 255 255)
freezeImage img
    >>= writePng "test.png"

Obviously, checking the boundaries on every single writePixel call could have some serious performance implications, which is why I suggest to add a new function, such as writePixelChecked or safeWritePixel (analogous to unsafeWritePixel) which would check the x and y coordinates separately against the image dimensions and either throw an error, or more nicely return a Maybe () or Either String ().

A suggestion for an implementation:

safeWritePixel :: (PrimMonad m, Pixel a) => MutableImage (PrimState m) a -> Int -> Int -> a -> m (Either String ())
safeWritePixel image@MutableImage{ mutableImageWidth = w, mutableImageHeight = h } x y px
    | x >= w = return $ Left "x exceeds the image's width"
    | y >= h = return $ Left "y exceeds the image's height"
    | otherwise = Right <$> writePixel image x y px

Which could e.g. be used as follows:

safeWritePixel img 50 25 (PixelRGB8 255 0 255)
    >>= \case
        Left err -> putStrLn err
        Right () -> return ()