Initial feedback for std.experimental.image

[via Digitalmars-d]

On Thursday, 9 July 2015 at 04:09:11 UTC, Rikki Cattermole wrote:
> On 9/07/2015 6:07 a.m., "Gregor =?UTF-8?B?TcO8Y2tsIg==?= 
> <gregormueckl at gmx.de>" wrote:
>> On Monday, 6 July 2015 at 13:48:53 UTC, Rikki Cattermole wrote:
>>>
>>> Please destroy!
>>>
>>
>> You asked for it! :)
>>
>> As a reference to a library that is used to handle images on a
>> professional level (VFX industry), I'd encourage you to look 
>> at the
>> feature set and interfaces of OpenImageIO. Sure, it's a big 
>> library and
>> some of it is definitely out of scope for what you try to 
>> accomplish
>> (image tile caching and texture sampling, obviously).
>>
>> Yet, there are some features I specifically want to mention 
>> here to
>> challenge the scope of your design:
>>
>> - arbitrary channel layouts in images: this is a big one. You 
>> mention 3D
>> engines as a targeted use case in the specification. 3D 
>> rendering is one
>> of the worst offenders when it comes to crazy channel layouts 
>> in
>> textures (which are obviously stored as image files). If you 
>> have a data
>> texture that requires 2 channels (e.g. uv offsets for texture 
>> lookups in
>> shaders or some crazy data tables), its memory layout should 
>> also only
>> ever have two channels. Don't expand it to RGB transparently 
>> or anything
>> else braindead. Don't change the data type of the pixel values 
>> wildly
>> without being asked to do so. The developer most likely has 
>> chosen a 16
>> bit signed integer per channel (or whatever else) for a good 
>> reason.
>> Some high end file formats like OpenEXR even allow users to 
>> store
>> completely arbitrary channels as well, often with a different
>> per-channel data format (leading to layouts like RGBAZ with an
>> additional mask channel on top). But support for that really 
>> bloats
>> image library interfaces. I'd stick with a sane variant of the
>> uncompressed texture formats that the OpenGL specification 
>> lists as the
>> target set of supported in-memory image formats. That mostly 
>> matches
>> current GPU hardware support and probably will for some time 
>> to come.
>
> As long as the color implementation matches isColor from 
> std.experimental.color. Then it's a color. I do not handle that 
> :)
> The rest of how it maps in memory is defined by the image 
> storage types. Any image loader/exporter can use any as long as 
> you specify it via a template argument *currently*.
>

Hm... in that case you introduce transparent mappings between 
user-facing types and the internal mapping which may be lossy in 
various ways. This works, but the internal type should be 
discoverable somehow. This leads down a similar road to OpenGL 
texture formats: they have internal storage formats and there's 
the host formats to/from which the data is converted when passing 
back and forth. This adds a lot of complexity and potential for 
surprises, unfortunately. I'm not entirely sure what to think 
here.

>> - window regions: now this not quite your average image format 
>> feature,
>> but relevant for some use cases. The gist of it is that the 
>> image file
>> may define a coordinate system for a whole image frame but 
>> only contain
>> actual data within certain regions that do not cover the whole 
>> frame.
>> These regions may even extend beyond the defined image frame 
>> (used e.g.
>> for VFX image postprocessing to have properly defined pixel 
>> values to
>> filter into the visible part of the final frame). Again, the 
>> OpenEXR
>> documentation explains this feature nicely. Again, I think 
>> this likely
>> is out of scope for this library.
>
> Ugh based upon what you said, that is out of scope of the image 
> loader/exporters that I'm writing. Also right now it is only 
> using unsigned integers for coordinates. I'm guessing if it is 
> outside the bounds it can go negative then.
> Slightly too specialized for what we need in the general case.
>

Yes, this is a slightly special use case. I can think of quite a 
lot of cases where you would want border regions of some kind for 
what you are doing, but they are all related to rendering and 
image processing.

>> My first point also leads me to this criticism:
>>
>> - I do not see a way to discover the actual data format of a 
>> PNG file
>> through your loader. Is it 8 bit palette-based, 8 bit per 
>> pixel or 16
>> bits per pixel? Especially the latter should not be 
>> transparently
>> converted to 8 bits per pixel if encountered because it is a 
>> lossy
>> transformation. As I see it right now you have to know the 
>> pixel format
>> up front to instantiate the loader. I consider that bad 
>> design. You can
>> only have true knowledge of the file contents after the image 
>> header
>> were parsed. The same is generally true of most actually 
>> useful image
>> formats out there.
>
> The reasoning is because this is what I know I can work with. 
> You specify what you want to use, it'll auto convert after 
> that. It makes user code a bit simpler.
>

I can understand your reasoning and this is why libraries like 
FreeImage make it very simple to get the image data converted to 
the format you want from an arbitrary input. What I'd like to see 
is more of an extension of the current mechanism: make it 
possible to query the data format of the image file. That way, 
the application can make a wiser decision on the format in which 
it wants to receive the data, but it always is able to get the 
data in a format it understands. The format description for the 
file format would have to be quite complex to cover all 
possibilities, though. The best that I can come up with is a list 
of tuples of channel names (as strings) and data type (as enums). 
Processing those isn't fun, though.

>> - Could support for image data alignment be added by defining 
>> a new
>> ImageStorage subclass? The actual in-memory data is not 
>> exposed to
>> direct access, is it? Access to the raw image data would be 
>> preferable
>> for those cases where you know exactly what you are doing. 
>> Going through
>> per-pixel access functions for large image regions is going to 
>> be
>> dreadfully slow in comparison to what can be achieved with 
>> proper
>> processing/filtering code.
>
> I ugh... had this feature once. I removed it as if you already 
> know the implementation why not just directly access it?
> But, if there is genuine need to get access to it as e.g. void* 
> then I can do it again.
>
>> - Also, uploading textures to the GPU requires passing raw 
>> memory blocks
>> and a format description of sorts to the 3D API. Being 
>> required to
>> slowly copy the image data in question into a temporary buffer 
>> for this
>> process is not an adequate solution.
>
> Again for previous answer, was possible. No matter what the 
> image storage type was. But it was hairy and could and would 
> cause bugs in the future. Your probably better off knowing the 
> type and getting access directly to it that way.
>

This is where the abstraction of ImageStorage with several 
possible implementations becomes iffy. The user is at the 
loader's mercy to hopefully hand over the right implementation 
type. I'm not sure I like that idea. This seems inconsistent with 
making the pixel format the user's choice.  Why should the user 
have choice over one thing and not the other?



>
> Some very good points that I believe definitely needs to be 
> touched upon where had.
>
> I've had a read of OpenImageIO documentation and all I can say 
> is irkkk.
> Most of what is in there with e.g. tiles and reflection styles 
> methods are out of scope out right as they are a bit 
> specialized for my liking. If somebody wants to add it, take a 
> look at the offset support. It was written as an 'extension' 
> like ForwardRange is for ranges.
>

I mentioned OpenImageIO as this library is full-featured and very 
complete in a lot of areas. It shows what it takes to be as 
flexible as possible regarding the image data that is processed. 
Take it as a catalog of things to consider, but not as template.

> The purpose of this library is to work more for GUI's and games 
> then anything else. It is meant more for the average programmer 
> then specialized in imagery ones. It's kinda why I wrote the 
> specification document. Just so in the future if somebody comes 
> in saying its awful who does know and use these kinds of 
> libraries. Will have to understand that it was out of scope and 
> was done on purpose.

Having a specification is a good thing and this is why I entered 
the discussion. Although your specification is still a bit vague 
in my opinion, the general direction is good. The limitation of 
the scope looks fine to me and I'm not arguing against that. My 
point is rather that your design can still be improved to match 
that scope better.