'image' should be in IO

[fryguybob]

(Imported from http://code.google.com/p/diagrams/issues/detail?id=59. Original issue from felipe.l...@gmail.com on November 6, 2011, 06:06:54 PM UTC)

Currently we have on the user side

image :: Renderable Image b => FilePath -> Double -> Double -> Diagram b R2

and on the backend side

data Image = Image { imgFile :: FilePath
, imgSize :: SizeSpec2D
, imgTransf :: T2
}

However, there are some problems with this approach. First of all, we are forcing every backend that supports Image to be inside IO -- diagrams-cairo is already in IO, but diagrams-svg isn't.

Another problem is that the user can't do anything if the image loading process fails. If 'image' was in IO, the user would be able to catch an exception and do something else (like drawing something instead of showing the image).

Yet another problem is that the diagrams library isn't able to do anything about the image's size, since it doesn't know anything about it.

So, what we need is something like

class SupportsImage b where
image :: SupportsImage b => FilePath -> IO (Diagram b R2)

Each backend may do whatever it wants to load the image. Note that it returns a Diagram, so the backend is free to choose its internal representation of an image. For example, if the user gives an SVG image, 'diagrams-svg' may choose to just inline it on the <defs> of the output.

On the user side, this API also has advantages. First of all, 'image' is now in IO and the user is back in control. Also, the user doesn't need to supply the dimensions: the backend is able to load the image and get the correct dimensions.

[fryguybob]

(Imported. Original comment by felipe.l...@gmail.com on November 6, 2011, 06:11:11 PM UTC)

Of course that should be

class SupportsImage b where
image :: FilePath -> IO (Diagram b R2)

Another more flexible idea would be to do something like

-- | /Minimum complete definition:/ 'imageHandle'
class SupportsImage b where
image :: FilePath -> IO (Diagram b R2)
image fp = withFile fp ReadMode imageHandle

imageHandle :: Handle -> IO (Diagram b R2)

Possible problems with this approach:

• We don't use Renderable. Is this a problem?
• All images have to be in memory before the diagram starts rendering.

Anything else?

[fryguybob]

(Imported. Original comment by byor...@gmail.com on November 6, 2011, 09:10:52 PM UTC)

Well, the IO has to go somewhere of course, and I was trying to keep it out of the diagram-building code. So I don't find the argument about forcing backends to be in IO very compelling. However, I do find compelling the ability to know the size up front, and to allow the user to handle failure themselves. So I think this is a good idea.

However, I think the SupportsImage class will have to be a bit more sophisticated. For one thing there ought to be more explicit ways for users to deal with errors, instead of just forcing them to catch I/O errors themselves. They should also be able to specify a size for the resulting image diagram if they want. So perhaps something like

class ImageBackend b where -- another potential name for the class?
image :: FilePath -> SizeSpec2D -> IO (Maybe (Diagram b R2))
image = ... catch imageRaw and turn exceptions into Maybe ...

imageRaw :: FilePath -> SizeSpec2D -> IO (Diagram b R2)
imageRaw = ... default in terms of imageHandle ...

imageHandle :: ...

and so on.

It's not a problem that we don't use Renderable. Each backend will have to use it internally for whatever type of image primitive it provides.

The fact that all images have to be in memory before the diagram starts rendering could be a problem, although there may be sneaky ways (unsafeInterleaveIO?) to get around it.

[fryguybob]

(Imported. Original comment by felipe.l...@gmail.com on November 6, 2011, 09:26:20 PM UTC)

Nice to see that you agree, even if not for exactly the same reasons =D.

Regarding sizes, isn't it better to have some general

sized :: Backend b => SizeSpec2D -> AnnDiagram m b R2 -> AnnDiagram m b R2

instead of putting sizing code on every backend? (Maybe sized may even get a more general type.)

Regarding exceptions/Maybe, what's the reason of returning a Maybe? Simple code won't handle the Nothing anyways and will always pattern match on the Just. More sophisticated code may be able to log the exception, which will tell you why the image loading failed (file not found, file format unrecognized, corrupted file, etc). I don't see any reasons for having IO (Maybe ...) as our default.

[fryguybob]

(Imported. Original comment by byor...@gmail.com on November 7, 2011, 02:33:12 AM UTC)

Ah, you're right, 'sized' is much better!

How about (Either IOError) instead of Maybe? I just want to make it easy for people to use without having to figure out how to use the exception-handling mechanisms.

[fryguybob]

(Imported. Original comment by felipe.l...@gmail.com on November 7, 2011, 12:41:55 PM UTC)

(Please excuse me if I'm bikeshedding more than I should. Please feel free to ignore me =).)

IMHO, it is easier to use the exception thing. Compare

i <- image "funny-cat.jpg"

to either

Just i <- image "funny-cat.jpg"

or

Right i <- image "funny-cat.jpg"

Of course, I'm not being fair since I'm not handling any possible errors =). My point is that most people won't handle the errors that may appear. For example, in the current API it isn't even possible to handle errors! In this case, the exception thing is better in terms of readability and also in terms of error printing ('Right i <- ' will just give a pattern match failure).

Not using Maybe/Either also has the advantage of being consistent. For example, image saving may also fail, but the Cairo backend just returns 'IO ()'. Either is for errors, exceptions are for exceptions.

Okay, so I'll give some options we have to end this bikeshedding:

a) 'image' with Maybe/Either, 'imageRaw' with exceptions.

b) 'image' with exceptions, 'tryImage' with Maybe/Either.

c) 'image' with exceptions, good docs.

Option c) would be to have only 'image' and tell the user in the documentation how to catch errors.

My opinion is that I don't like a), but you don't like c). However, I think that b) is a good compromise. Note that

tryImage :: FilePath -> IO (Either SomeException (Diagram b R2))
tryImage = try . image

with the bonus that we have restricted the type to 'SomeException'. What do you think?

[fryguybob]

(Imported. Original comment by fryguy...@gmail.com on November 7, 2011, 02:28:38 PM UTC)

Sorry, I'm coming into this discussion late, but I'm not sure why it is wrong for a reference to an image (FilePath) to need to be something in IO. If you have a backend that simply uses the reference as a reference there is no need to do any IO. If you actually want to read the bits of the reference or simply check to see if it is there then at render time you might do some IO.

[fryguybob]

(Imported. Original comment by felipe.l...@gmail.com on November 7, 2011, 02:50:04 PM UTC)

Even if the backend only puts a reference, it needs to know the image size when rendering (even on today's API). See also the first paragraph of Comment 3.

[fryguybob]

(Imported. Original comment by fryguy...@gmail.com on November 7, 2011, 03:01:03 PM UTC)

The size can be declared or "sized to fit" in several ways. Certainly in some situations you want to size to match resolution and use that size elsewhere, but I don't want to see pure image references go away.

[fryguybob]

(Imported. Original comment by felipe.l...@gmail.com on November 7, 2011, 04:10:21 PM UTC)

Although I regard these "pure image references" a misfeature, would something like unsafeImage suit your needs?

unsafeImage :: ImageBackend b => FilePath -> Diagram b R2
unsafeImage = unsafePerformIO . image

[byorgey]

Maybe part of the problem in the above discussion is the conflation of image loading and the image function for constructing a diagram. Perhaps what we really want is a new Image type, which can either be a reference to an external file, or an array of pixel values, along with a size. Then we can provide several functions for constructing Image values (some in IO, some not), and a (pure) function image :: Image -> Diagram. Then backends are not forced to be in IO just to support embedded images (though perhaps the Image type will need a type index indicating whether it contains pixel data or an external file reference).

[byorgey]

See https://trello.com/c/FTkpevHs .

[byorgey]

Reposting here because it's difficult to read on the narrow trello card. Here's a proposed new API for images, feedback welcome:

data ImageDataType = Embedded | External

data ImageData :: ImageDataType -> * where
  ImageRaster :: DynamicImage -> ImageData Embedded
  -- ^ DynamicImage is from JuicyPixels
  ImageRef :: FilePath -> ImageData External

data Image :: ImageDataType -> * where
  Image :: ImageData t -> Int -> Int -> T2 -> Image t
  -- ^ width, height, applied transformation

-- | Use JuicyPixels to read an image in any format.
loadImage :: FilePath -> IO (Either String (Image Embedded))

-- | Check that a file exists, and use JuicyPixels to figure out
--   the right size, but save a reference to the image instead
--   of the raster data
imageRef :: FilePath -> IO (Either String (Image External))

-- | Make an "unchecked" image reference; have to specify a
--   width and height.
uncheckedImageRef :: FilePath -> Int -> Int -> Image External

-- | Create an image "from scratch" by specifying the pixel data
raster :: (Int -> Int -> AlphaColour Double) -> Int -> Int -> Image Embedded

Backends can have a Renderable (Image External) instance if they are able to load images themselves, or embed links to external images (e.g. SVG). They can also have a Renderable (Image Embedded) instance and use JuicyPixel to help convert the raster data into whatever format they need. For example, SVG can convert it to PNG and then embed it with a base64-encoded data URI.

[bergey]

A few questions to make sure I'm reading this correctly:

1. Embedded and External are about the source of the image. It's up to the Backend whether to embed or link in the output file?
2. The Int arguments above are in pixels?
3. Unlike the current implementation, the aspect ratio is always known when the Image is constructed?

And two more serious questions:

1. What is the size of the resulting Diagram? Is it always 1 wide by aspect-ratio tall? Or do the pixel dimensions become Diagrams abstract dimensions? I think those are the obvious choices, and that either is fine.
2. Say I'm printing my 1500-pixel-wide photo at 1" wide. It sounds like it's up to the Backend to handle down-sampling (or not to). Is it worth providing some way for the user to hint that 300 pixels per final Diagrams-abstract-unit would be plenty? If so, does this belong in the Backend options? I think that's right; I think it's rare that I would specify different final resolutions for different images in the same Diagram.

[byorgey]

Embedded and External are about the source of the image. It's up to the Backend whether to embed or link > in the output file?

Right, because the important distinction to make semantically is whether we have some image data or merely a reference to some image data. That is what determines whether a backend can handle it or not (e.g. you can imagine a backend that could handle image data but not file references, or vice versa). What the output looks like is up to the backend, and it doesn't make sense for the type of an image to determine what sort of output it should result in.

There might be better names than Embedded and External, I'm open to suggestions.

The Int arguments above are in pixels?

Yes. Perhaps we should make a Pixel newtype or type synonym.

Unlike the current implementation, the aspect ratio is always known when the Image is constructed?

I imagine that uncheckedImageRef works like the current implementation, where the given size may not match the actual image aspect ratio, in which case padding will be added. With the other three, the aspect ratio is known.

Re: the size of the resulting Diagram, I am not sure. One argument for using pixel dimensions as abstract diagrams dimensions is that you might have a collection of images of various sizes, and you want them to retain their relative sizes. If all images are automatically 1 wide, you would have to do some manual resizing. And conversely, if pixel dimensions are used but you want an image which is 1 unit wide, you can easily apply sized (Width 1) to it. However, in the end perhaps this should just be an optional argument, so you can specify which you want at the time the Image is created?

Re: downsampling and such, that's an interesting and important question. You might also wish to be able to specify various sorts of hinting options to ensure an image is aligned to pixel boundaries in the resulting image (for raster outputs). I am not sure what the best/right thing is to do here. This seems related to our recent discussion about units (to which I plan to respond soon).

[jeffreyrosenbluth]

I'm just wondering if we want to tie the backends down to supporting a Juicy Pixels, Dynamic Image.
Some backends can probably handle the raw data and may even be able to handle formats that Juicy Pixels can't. I think I would change.

ImageRaster :: DynamicImage -> ImageData Embedded

to

ImageRaster :: ByteString -> ImageData Embedded

The backends can handle the conversion to images themselves. Of course we can provide functions to help. I just finished an experimental version using ByteString in the Rasterfic backend (units branch) but that code will eventually be scrapped once we decide on an Image API.

[byorgey]

Well, I agree that tying backends down to one particular library may not be a good idea. The problem with ImageRaster :: ByteString -> ImageData Embedded, however, is that a raw ByteString, by itself, is rather meaningless; this would make the decision of how to interpret the ByteString rather arbitrary and possibly different across different backends, and makes it easy to accidentally get the encoding wrong, resulting in garbage. I proposed using JuicyPixels because it sits at a good middle ground of giving access to the low-level pixel data while still encoding relevant information about the semantics, so it's impossible to accidentally treat RGB data as grayscale or whatever.

Note we are not necessarily tying backends down to supporting every type of Dynamic Image. Maybe each backend has only a few types that it actually supports.

[jeffreyrosenbluth]

I agree with what are saying about ByteString being arbitrary. What I'm worried about is not supporting all of the dynamic image types, but the case where a backend wants to support a format that is not included in Dynamic Image.

Sent from my iPad

On Apr 6, 2014, at 12:54 PM, Brent Yorgey notifications@github.com wrote:

Well, I agree that tying backends down to one particular library may not be a good idea. The problem with ImageRaster :: ByteString -> ImageData Embedded is that a raw ByteString, by itself, is rather meaningless; this would make the decision of how to interpret the ByteString rather arbitrary and possibly different across different backends, and makes it easy to accidentally get the encoding wrong, resulting in garbage. I proposed using JuicyPixels because it sits at a good middle ground of giving access to the low-level pixel data while still encoding relevant information about the semantics, so it's impossible to accidentally treat RBG data as grayscale or whatever.

Note we are not necessarily tying backends down to supporting every type of Dynamic Image. Maybe each backend has only a few types that it actually supports.

—
Reply to this email directly or view it on GitHub.

[fryguybob]

Individual backends can always sidestep the process with their own
attribute.

On Sun, Apr 6, 2014 at 1:12 PM, Jeffrey Rosenbluth <notifications@github.com

wrote:

I agree with what are saying about ByteString being arbitrary. What I'm
worried about is not supporting all of the dynamic image types, but the
case where a backend wants to support a format that is not included in
Dynamic Image.

Sent from my iPad

On Apr 6, 2014, at 12:54 PM, Brent Yorgey notifications@github.com
wrote:

Well, I agree that tying backends down to one particular library may not
be a good idea. The problem with ImageRaster :: ByteString -> ImageData
Embedded is that a raw ByteString, by itself, is rather meaningless; this
would make the decision of how to interpret the ByteString rather arbitrary
and possibly different across different backends, and makes it easy to
accidentally get the encoding wrong, resulting in garbage. I proposed using
JuicyPixels because it sits at a good middle ground of giving access to the
low-level pixel data while still encoding relevant information about the
semantics, so it's impossible to accidentally treat RBG data as grayscale
or whatever.

Note we are not necessarily tying backends down to supporting every type
of Dynamic Image. Maybe each backend has only a few types that it actually
supports.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHubhttps://github.com//issues/29#issuecomment-39673808
.

[jeffreyrosenbluth]

Yes that's true. The other issue I have is that the user now has to understand some of the JP API. Instead of just reading in a png file for example.

Sent from my iPad

On Apr 6, 2014, at 1:13 PM, Ryan Yates notifications@github.com wrote:

Individual backends can always sidestep the process with their own
attribute.

On Sun, Apr 6, 2014 at 1:12 PM, Jeffrey Rosenbluth <notifications@github.com

wrote:

I agree with what are saying about ByteString being arbitrary. What I'm
worried about is not supporting all of the dynamic image types, but the
case where a backend wants to support a format that is not included in
Dynamic Image.

Sent from my iPad

On Apr 6, 2014, at 12:54 PM, Brent Yorgey notifications@github.com
wrote:

Well, I agree that tying backends down to one particular library may not
be a good idea. The problem with ImageRaster :: ByteString -> ImageData
Embedded is that a raw ByteString, by itself, is rather meaningless; this
would make the decision of how to interpret the ByteString rather arbitrary
and possibly different across different backends, and makes it easy to
accidentally get the encoding wrong, resulting in garbage. I proposed using
JuicyPixels because it sits at a good middle ground of giving access to the
low-level pixel data while still encoding relevant information about the
semantics, so it's impossible to accidentally treat RBG data as grayscale
or whatever.

Note we are not necessarily tying backends down to supporting every type
of Dynamic Image. Maybe each backend has only a few types that it actually
supports.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHubhttps://github.com//issues/29#issuecomment-39673808
.

—
Reply to this email directly or view it on GitHub.

[byorgey]

I don't think this requires users to understand the JP API. They can just call loadImage which handles calling the JP API appropriately. This will require backend implementors to understand a bit of the JP API (at least backend implementors who want to support embedded images), but that seems fine to me.

[jeffreyrosenbluth]

fair enough :)