DynamicImage->ImageBuffer .. supportable types or streamlining

[dobkeratops]

back doing rust graphics programming after a long time away.

http://www.piston.rs/image/image/struct.ImageBuffer.html

http://www.piston.rs/image/image/enum.DynamicImage.html

ok writing a texture loader from this.. at first , 24bit was fine. then generalising it to handle 24bit/32bit seemed awkward (i might be a bit fussy about eliminating some cut-paste that happens because you need to match to get the buffer.

now on reflection the 2 lines of cut-paste for my use case is ok, it's just bugging me r.e scalability/elegance).

The effort would be worthwhile however, if this wrapping (which seems to allow a type-parameter for the pixel type) allows safe handling of a multitude of formats:-

• 24,32bit - done
• 16bit: 1555, 565, 4444 - these do have some use-cases
• DXT compression ..
• HDR formats - 16 and 32bits per channel
• palettized - retro projects, but maybe some use cases e.g 32bit-palette allows 8bpp to use decent colours and alpha adaptively.

I was thinking an easier way to make image buffers safe would be to use a 3D array as an intermediate: and moving images to & from AI libraries more frequently involves multi-dimensional array operations (especially consider 4D arrays for the feature detectors, and synergy between that and 'array texture' code, i.e. "16 slices of 4-channel RGBA 512x512 images" = u8[512][512][4][16]

.. but if this enum/match approach can handle different bit-level encodings, that would be great

I guess I could try to contribute that if it is compatible with the architecture (i don't quite know the rest of the details re. ImageBuffer etc..)

[dobkeratops]

It's literally one line of cut-paste that i'm seemingly complaining about, but it's also the need to have written that function backwards with a lambda, or have created yet another helper function externally (writing out more intermediate types, more names to sift through) .. and if there were more cases you'd have to extend the 'cut-paste' part.

one little suggestion : what if you tupled the 'raw u8 buffer' with width/channels as follows:

   fn get_buffer(&self)->( (vec[u8],[usize;3])
   // second param is [channels,height,width], i.e the dimensions of a 3d array 
   // alternatively just give 2 of those numbers and the user computes channels from vec.len / w*h or whatever

Is there a way to streamline this? in a C or C++ loader i'd just get back some bytes and 3 numbers, width/height/num_channels, and i'm sorted.

EDIT I found a way to rewrite it half fixing my 'fussy issues' , at least i didn't need the lambda. i guess there's just the suggestion to only need one call to 'img.whatever' to extract stuff by tupling the dimensions with the buffer, aproximating the usability of multi-dim array objects.

fn create_texture_from(dimg:image::DynamicImage)->TextureHandle{

	if let Some(((us,vs),bfr,fmt))=
		(match dimg {
			image::DynamicImage::ImageRgb8(img)=> Some((img.dimensions(),img.into_vec(),GL_RGB)),
			image::DynamicImage::ImageRgba8(img)=> Some((img.dimensions(),img.into_vec(),GL_RGBA)),
			_=> None
		})
	{unsafe {
		let mut texid:TextureHandle=0;
		glGenTextures(1,&mut texid);
		glBindTexture(GL_TEXTURE_2D,texid);
		glTexEnvi( GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, GL_MODULATE as GLint);
		glTexParameteri( GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR as GLint);
		glTexParameteri( GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR as GLint);
		glTexParameteri( GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_REPEAT as GLint);
		glTexParameteri( GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_REPEAT as GLint);

		glTexImage2D(GL_TEXTURE_2D, 0, fmt as GLint, us as GLint,vs as GLint, 0, fmt, GL_UNSIGNED_BYTE, (&bfr[0]));
		dbgt!("created texture {}",texid);
		return texid;
	}} else {0}

}

[fintelia]

Would using the ImageDecoder trait directly work better for your use case?

[dobkeratops]

@fintelia thanks for the info.. it's hard to tell, it seems to solve a different problem i.e. providing more interface to step through an image, whereas I just want to pass it to GL.
it also seems to decode then tell you a format via a variant meaning the usage code will look similar; perhaps it's an interface for an intermediate in image conversions..

I suspect in the other cases I have in mind, want my code to ask for the image in a specific element format (i.e. "i'm going to do some image processing in HDR range fp32 ; give me the image as a 3D array of fp32 values" ). I was surprisingly impressed with how useable numpy was just throwing arrays around for these kind of use cases, and ditto later if i do move into AI-related image processing stuff i think thats what code would look like. I'm talking about 2 different things here.. just loading into an engine, and more advanced image processing

[dobkeratops]

to clarify the 'problem' here, not so bad but it will get worse scaling for more texture formats:-

fn create_texture_from(dimg:image::DynamicImage)->TextureHandle{

	if let Some(((us,vs),bfr,fmt))=
		(match dimg {
			image::DynamicImage::ImageRgb8(img)=> Some((img.dimensions(),img.into_vec(),GL_RGB)),
			image::DynamicImage::ImageRgba8(img)=> Some((img.dimensions(),img.into_vec(),GL_RGBA)),
                               //               It's this repetition,   ^^^^^^^^^^^^^^^^^
                                // cut-pasting code extracting the same information per variant
                                // especially repeat invocations of specifying the 'img'
                                // parameter to get 'buffer, width, height separately
                             // I suggest tupling to fake a
                          // '3d array type' to bind that info together.
			_=> None
		})
	{unsafe {

[dobkeratops]

another way to do it with less repetition and 'ceremony' is to get a buffer with a set of numeric values: num bits per channel, num channels, width, height. total buffer size, and an enum for channel format = (bitschannelswidth*height)/8. 'bits per channel' will be 8. or you could even say "bits per channel0-3; ChannelType; width; height; total size=((sum(0..3 bits ))wh)/8", and that will handle all the packed formats. you still get to leverage Rusts' match statement in passing to GL but cleaner...

      if let Some(bits_per_channel,buffer,width,height) = myDynamicImage.get_buffer(){
       let format = match (bits[0],bits[1],bits[2],bits[3],channel_desc) {
              (8,8,8,0,ImageRGB) => (GL_RGB),
              (8,8,8,8,ImageRGBA) => (GL_RGBA),
              (8,8,0,0,ImageLA) => ...,
            // etc
       }
       glGenTextures...
       glTextureImage(.... format... &buffer[0]..)
       id
     } else {0}

the code to use it is more straightforward.. less nesting ,less intermediate named types to dig through documentation for.

whats nice there is you could even get rid of more temporary variables, after time in haskell I've begun to find the need to make so many variables to do things irritating :).. I like just passing values through sequences of expressions etc
(EDIT you can certainly add 'luma' to that list, the RGB's wil just match with a zero, and vica versa)

[fintelia]

I'm not sure I follow. Couldn't you do something like:

fn create_texture_from<D: ImageDecoder>(decoder: D)->Result<TextureHandle>{
    let (width, height) = decoder.dimensions()?;
    let format = match decoder.colortype()? {
        ColorType::RGB(8) => GL_RGB,
        ColorType::RGBA(8) => GL_RGBA,
        ColorType::Gray(8) => GL_LUMINANCE,
        ColorType::Palette(_) => unreachable!(),
        _ => unimplemented!(),
    };
    let contents = match decoder.read_image()? {
        DecodingResult::U8(v) => v,
        DecodingResult::U16(_) => unimplemented!(), // A bit sad to have this special case
    };

    // Actually create texture
}

[dobkeratops]

can't it just have something that returns a buffer and a numerical description of the bits as I indicated, then there's less matching ceremony to go through . you've got 2 matches there. My existing solution has just 1, and my proposed API extention simplifies the boilerplate - by describing the image as bits directly. It would handle all the cases like 1555 , 565, 4444, 8888 888 via a few simple numbers ; and they will sum to a word size packed pixel because that's how those formats were designed.

I think the issue here is this library is designed for a broader range of use cases (i.e. actually writing conversions from one format to another) - fair enough. I'm just suggesting adding some peices that will streamline the most common tasks people are likely to encounter (grab an image in a desired format, or hand whats there to openGL). Personally most of the time if i'm actually going to do something to an image i'd ask for it in 32bpp 8888 , or [fp32;4], but I do know there's a range of other formats in the middle like 10:10:10 and so on, if people want to throw screenshots around and so on..

[dobkeratops]

the nice thing about reporting it as numbers of bits is that some of the alternative code paths in other situations will be unified by just using those bit values to extract data with shifts and masks directly: less matching/wrapping, more direct computing, smaller code size, less documents to dig through. I think this is a win all round.

[dobkeratops]

anyway i'll give it a rest. i've got just 1 "offending" line, its ok. it does what I need right now.

I suppose i'm trying to think ahead , is there anything I could add to the library based on things i'm about to do (and things i could change that would make it better for future users)

One more thing to talk about is normal map formats

and ok the "bits" idea does break down with compressed textures (so it would still need a match... "Uncompressesd() vs Compressed()" with the bits idea being in the 'uncompressed' arm.

[fintelia]

Yeah, the issue with just numbers of bits is that there are a bunch of other formats that can't be described that way, including compresesd images, images that have palettes, BGR images and so forth. Arguably, grayscale images don't fit cleanly into that description either. Even OpenGL uses an enum to describe the color format of a texture...

As for the unfortunate issue of having to match over Vec vs Vec, I completely agree.

[dobkeratops]

(what i've got in mind for further image processing is a multi-dim array with the ability to fold dimensions , e.g. doing something 'per scanline' is a fold of the outermost axis [row][column][channel], whereas changing pixel packing would be a fold of the last axis; tile interleaving would be like adding more dimensions e.g. turning 256x256 into 16x16 x16x16 - but i don't need that right now.
in my current sourcebase i've got some 2d & 3d array helper classes for landscape manipulation - it might be nice to clean that up and see if it could be applied to image manipulation code

One thing I might be doing is automatically generating guesses for 'specular channels' by appliing a filter (clamped difference of gaussians perhaps to pick out ridges), i have an itch to write an auto-tiling utility although preparing textures in GIMP probably better and i could look into script-fu there rather than rust )

[dobkeratops]

Yeah, the issue with just numbers of bits is that there are a bunch of other formats that can't be described that way,

right; and the compressed texture formats are really important to have. so indeed I concede supporting that is essential; there will be a match somewhere. I just wonder if one of the arms could be the 'uncompressed, direct-color' cases . but maybe that itself would be imposing a match-nesting heh.

[HeroicKatora]

What seemed to be the result of a discussion on gitter, these two following methods could be useful for ImageBuffer to communicate how it uses the underlying container:

pub fn dimensions(&self) -> (usize, usize, usize) { 
    (image.width(), image.height(), P::channel_count()) 
} 

pub fn strides(&self) -> (usize, usize, usize) { 
    (image.width()*P::channel_count(), P::channel_count(), 1) 
}

Open questions:

• Should there be another buffer wrapper that takes T: AsRef<[P::Subpixel]> and three arbitrary strides, to accomodate cases where the samples are layed out differently in the input container?
• In case that yes, should it maybe even be merged with the current ImageBuffer?

[fintelia]

This functionality seems completely reasonable to add. ImageBuffer::dimensions already exists with a different return type, so it may be better to pick another name. I also don't love using usize in the return types, but u32 or u64 would probably be worse.

[HeroicKatora]

The main reason for usize is to express that this result is to be used in index calculation, while that is not the main concern of the existing dimensions. As a different name, maybe array_extents would be better?

[dobkeratops]

ok calming down a bit I realise you dont want feature creep (potential future alternate processing library)
but let me update the suggestion to this API - it does mean making a type but named fields explain whats going on

     struct InterleavedChannelArray<C:ChannelType> { // or 'ChannelArray', ?
         pub buffer:Vec<C>,
         pub layers:usize,        // optional, would be 1 mostly
         pub height:usize,
         pub width:usize,
         pub channels:usize,   // would be 3 or 4 mostly
                                 //maybe include strides here aswell, rename ChannelArray
     }

    impl DynamicImage {  // can't remember specifically.. the type you get from loading
           fn into_interleaved_channel_array<C:ChannelType>()->InterleavedChannelArray<C>;
    }

so for my (common) use case i would say

    let ar=image::Load("foo.jpg").into_interleaved_channel_array::<u8>() �

    glBufferData(... ar.buffer[0].as_void_ptr(), ar.width,  ar.height,  if ar.channels==3{GL_RGB}else{GL_RGBA} )

this allows getting (what I consider to be the most common image representation) in 1LOC, without needing to create & multiply reference variables/match/make multiple calls just to get the data & width out.

this is what i had to do instead (as far as I know), the irritation is multiple places where you have to reference the image variable and more nesting levels, more looking forwards and backwards to see what it's actually doing

fn create_texture_from(dimg:image::DynamicImage)->TextureHandle{


	if let Some(((us,vs),bfr,fmt))=
		(match dimg {
			image::DynamicImage::ImageRgb8(img)=> Some((img.dimensions(),img.into_vec(),GL_RGB)),
			image::DynamicImage::ImageRgba8(img)=> Some((img.dimensions(),img.into_vec(),GL_RGBA)),
			_=> None
		})

it does have a bit of polymorphism i.e. this call could support u8 or f32. That would be a bit more involved to implement i.e some internal polymorphism for conversions for details of u8 or f32 conversion ... alternatively you could just support u8 and leave it to the community to extend if someone wants HDR done this way

the name of the buffer object is open to question .. whats the best balance between explicitness/concision..

you could also embed the strides (and rename the struct ChannelArray<T>) allowing planar formats to be held however I would still suggest the underlying call remains 'get_interleaved_channel_array' returning the specific layout where the channel stride is 1 (and if there's demand, support get_planar..). The word 'interleaved' means R0 G0 B0 A0 R1 G1 B1 A1 ..etc versus R0 R1 R2.. G0 G1 G2... etc (planar).

if supported, layers could make it a 4D array, but will be set to 1 most of the time.. it's just it would open the door to represent texture arrays , volume textures/voxels,(and possibly cube-map faces?) in the same object, also feature maps for machinelearning

Image needn't implement the details of array access or processing - anyone else could make a 3D,4D array interface , and impl it for InterleavedChannelArray

it could be stretched with channels=1, T=??? to hold 'packed' formats such as 565 in this struct however my request is really for a call that always converts the contained image to a format where the channels are expanded out (i.e. if the image was 565, it would have to do all the shift/mask/scaling extraction.. the work could explode which is why i suggest marking it as experimental and just supporting the simple-to-write 24/32bpp bases,panic on the rest, and leave it to the community to flesh out the cases that actually get used

This will just pave the way for interfacing to processing libraries, and also streamline the simple use case of getting a simple heightxwidthx(3 or 4 u8) into GL. the use of an array replaces polymorphism with indexing - a lot of GPU programming is array based, and elsewhere I've been impressed with how you get a lot of intuitive functionality for simple APIs when leveraging multi-dim array functionality (which has many uses beside images).

i suppose you could even consider having a vector of channel names (Either<StandardChannel,String>) ?? enum StandardChannel{Red,Green,Blue,Alpha,Luma,Y,U,V, ..} .. and you could consider getting GIMP channels out this way..* although they might start to get really inefficient without per-channel compression , and I definitely wouldn't suggest going that far here

[HeroicKatora]

It's a good point to note that converting a DynamicImage into an interleaved array with no associated static color type is potentially less computational work. However, under that point of view the argument for automatically converting sample representation falls a bit flat. It would cause implicit O(w·h) processing which would otherwise be avoided. Should we consider immediately consider that DynamicImage needs to represent images with u16 or other sample types?

The same holds if the conversion would be done as
fn into_flat_samples<T>(&self) -> Option<InterleavedChannesl<T>>
due to the implicit copy but an alternative is
fn into_flat_samples<T>(self) -> Result<InterleavedChannels<T>, Self>
which would currently only work for T == u8 anyways, as all underlying representations are of that type. The conversion could also be a trait, so that the container of InterleavedChannels can be a &[P::Subpixel] as well and both DynamicImage and ImageBuffer can be converted using the same interfaces.

As a generic output interface that may do arbitrary conversion on samples, the benefit compared to just providing the proposed two additional index-based methods above is much less clear to me. (Add: An explicit convert_samples::<f32>().into_flat_samples() maybe).

[dobkeratops]

It would cause implicit O(w·h) processing

this API is for when you know you want 8bits per channel - if you loaded something else that processing is unavoidable ; in my scenarios i'm loading formats that i know have that;

.. I do accept that eventually i will want to support other formats, but they're not yet supporter here either i.e. DDS compressed textures (and sure the simple image array idea breaks down there, it's a block format

, the benefit compared to just providing the proposed two additional index-based methods above is much less clear to me.

Its just an annoying interface. like many other things in rust. The bloat is coming from having to use the match construct to extract what could just be index information. The last time I was doing this in C/C++ that's how it worked, jpg::load_from_file(file, &width,&height,&channels), and it was more pleasant than this (i need one extra line to declare the vars to get the return values but it's not as annoying as the extra nesting levels here)

Perhaps I just made a mistake here thinking I can use someone elses library to load an image.

everything in rust is unecaserily bloated.. and the community is full of people who glorify that
please, prove my perceptions here wrong..

[HeroicKatora]

Some additional thought: InterleavedChannels, or any stride-based matrix representation, has an implicit invariant. Namely that the strides will only generate valid indices if multiplied and index with valid indices. Checking this invariant would require the member to be non-public so we need to be explicit about this not being the case, and provide a check method.

[dobkeratops]

Namely that the strides will only generate valid indices if multiplied and index with valid indices. Checking this invariant would

oh jesus christ...

it' s just as fucking image array.

[HeroicKatora]

this API is for when you know you want 8bits per channel - if you loaded something else that processing is unavoidable ; in my scenarios i'm loading formats that i know have that;

As I noted below, I don't think it shouldn't be possible with this library. The main point of the argument was that there should be two different interfaces, one which does the costly conversion, and one which has a guarantee of not doing it. In a generalized setting, many users may want such stability guarantees.

[HeroicKatora]

oh jesus christ...

Please read the full argument and stay on-topic. Again, Rust is more oriented towards stability.

C/C++ that's how it worked, jpg::load_from_file(file, &width,&height,&channels)

If you want to play example, compared the pnm loader from image::pnm::decoder and imagemagick::pnm and you tell me which is actually maintainable or analyzable by anyone but the original author.

So I must ask you to stay professional. This is the third time and many of your argument have already been considered and there was a consensus that some requests are reasonable. If you can not properly handle an technical evaluation of pros and cons, I see no way that this continues in a productive manner (Meaning I will close this issue and we continue adding the agreed upon improvements fully internally.).

[dobkeratops]

that there should be two different interfaces, one which does the costly conversion, and one which has a guarantee of not doing it.

thats fine; my suggestions are orthogonal to the other side of things where I understand you may wrap many different types of image with underlying representations, and you want to query exactly what was loaded.

I think whats gone wrong here for me is..

... i had some nice simple, working code in C++ 5 years ago, which loaded TGAs and JPGs, into an array. then I came to Rust, used FFI to grab them. Then I thought "hey, lets get into the spirit of the rust community and use an existing crate and rust code" ... and here I am jumping through hoops with layers of wrappers that bloat and confuse my code to do the same thing, and I pretty much have to make another wrapper function to make the library pleasant to use.

sorry to get frustrated and lash out.. but it must be possible to streamline. an image in an array is a simple concept i've been dealing with for 25 years.. it must be possible to write a simple interface to load such a thing with minimum fuss

[HeroicKatora]

then I came to Rust, used FFI to grab them.

I see how that would easily work in your single own project. But designing an interface that scales to a more general setting is not as easy as exposing an ffi function pointer, especially when you have to consider the Rust stance on undefined behaviour and memory safety.

[dobkeratops]

putting the data in a Vec<> achieves memory safety.

thats no excuse to bloat the rest of it

[dobkeratops]

ok. so if i just want to load an array of pixels i have to write my own library rather than re-use community libraries, got it.

although in C++ i could use an existing library,

hmmm.

[HeroicKatora]

If you feel like that is your sole choice, there is nothing stopping you. That's why this project is open source. @bvssvni Can I close this? (And reopen individual tracking issues for the agreed upon improvements)?

[dobkeratops]

If you want to play example, compared the pnm loader from image::pnm::decoder and imagemagick::pnm and you tell me which is actually maintainable or analyzable by anyone but the original author.

nice straw man, comparing Rust to C instead of C++ . I see people in this community doing that sort of thing on and off . you're kind of confirming some negative preconceptions i've learned in the past 5 years in this community.

The irony is, without exaggerating the safety aspect there's plenty of other reasons to want to use rust

[HeroicKatora]

So let's cool this down and continue tomorrow, I feel myself getting non-technical as well. The comparison could have been fairer had you actually provided any details on that library which seems to offer this magically sounding interface. Maybe you can prepare to engage in the arguments individually themselves without repeating your assertion that you don't want to write code. I can only tell you that I have used the lib in a Vulkan project myself, without the feeling major issues you seem to have.

[bvssvni]

@HeroicKatora there was this heavy debate some years ago when @dobkeratops , me and others wanted more gamedev ergonomic features in Rust, since we felt that the common language designer weren't thinking about those use cases that costed programmers hours and hours in long term projects. Gamedevs were really hoping Rust would move more in their direction, but they saw focus on safety as an overarching conflicting goal with their needs.

The lasting impression from that discussion was that some people did not really listened, or even made fun of these concerns (however, taken seriously by some very good programmers), but in the discussion a lot of interesting ideas came up. Jonathan Blow started working on Jai, I got interested in language design and made Dyon to experiment with some ideas, and @dobkeratops worked on his own language, written in C++.

I think that's the underlying issue here, which got very little to do with how you work with pixels.

@dobkeratops I have been thinking of some ideas of language design lately, starting a new thread here: PistonDevelopers/piston#1253

[HeroicKatora]

I had a growing suspicion that this problem was rooted much deeper, although I did not know of the lengthy prior legacy. However, there are alternatives to approaching these concerns, alternatives which are more healthy to the state of this library than approach its design from the perspective of a different language. Apparantly ffi has already worked successfully in parts.

And there are more pressing issues that actually limit functionality instead of revolving around mostly sparing 5 lines of wrapper code. I tried my best to extract those suggestions from the stylistic questions, hoping that this would at least partially solve those as well by embedding some documentation assumptions in runtime values.

[bvssvni]

@HeroicKatora I agree that we should focus on stability.

[dobkeratops]

ok i'll apologise for outbursts recently, I was lashing out. I have some broad frustrations and you might have just got me on the wrong day. I have found Image:: useful.

I might think about making an 'ImageArray' crate (somewhere between simple image handling, voxel grids, and interface to machinelearning stuff i do intend to dip back into later), but i'll probably run into the issue that its hard to get anyone to notice. (so many small crates, and only a few get community traction)

it would be nice to give back to the community. but any time i've tried I find ideas clash very quickly .. it's nearly impossible to do. so i'll probably just roll an ad-hoc thing. It's still nice to compare ideas.

i see that Image/dynamic image does have some processing interfaces which is interesting - and I know there's many , many ways to deal with internal storage. I already touched on the issue that multi-channel images (e.g. from GIMp with all the layers and channels) would really need some tiling to handle efficiently because there's so much empty space in the channels.. and thats where a more complex interface with iterators etc would become necassery. (something for parsing GIMP layers/channels woudl be really useful..)