Fix initial album fetch + cleanup

[ShadiestGoat]

This is a fix for #230

The root cause was basically that sometimes some methods would return image names (1234.png), sometimes the full path (/home/name/.cache/euphonica/images/1234.png)

To fix this, I had a bit of a rabbit hole moment and updated a bunch of stuff to all return the same thing - the the image name. Imo its just more robust than the full path. Also, in some instances it'd get the full path, then extract the image name from there so I cleaned that up.

I also extracted some of the image loading parts into a util function, since they all do the same but slightly different. Finally, there was 1 todo in set_image_internal which I resolved in what I hope is a clean manner

One annoyance that I wanted to resolve but just couldn't figure out the borrow/lifetime thing with is load_image is exclusively used with the .external.call wrapper, and despite how much I tried i could not figure out how to make that wrapper be part of the load_image util so ://

As with before, I'm not a rust person & am still flying more or less blind so please lmk if there is something wrong!

[htkhiem]

Could you clarify how calling these individually would be more efficient? I can't recall any instance of the code saving just one resolution and not both at the same time.

P.S. Thanks a lot for optimising away the above read-back-from-disk thing :) I didn't realise the byte slices returned by .as_bytes() are still valid inputs to Texture::from_bytes().

[ShadiestGoat]

The efficient comment is in reference to the avoiding disk read part. Its not less or more efficient to call them separately, but I basically just didn't really want to return the resized images from save_and_register_image. However, looking at it now, I think its not a bad idea to just return them from that call - I think any time we call that method it eventually leads to loading that image from disk. I'll look into this more & see if I can't spread this no-disk-read method to other places

[ShadiestGoat]

Ok I did find that any function that calls save_and_register_image will eventually want the image itself. Refactor, blah blah blah and I have a proposal in the PR. Lmk if its good.

I have 2 doubts about it:

1. I'm very confused by rust's borrowing and it's optimizer: does the current way I have it implemented do a return by value or by pointer? If its by value, how can I make sure to not do that? (i'm specifically talking about the RegisteredImage.img)
2. The read_texture_from_register looks like its copying the bytes twice, but I'm not 100% sure. I followed the example impl given by the image create, however, glib::Bytes::from explicitly says it copies the data from bytes, so I'm wondering if its not possible to like not do that & to write to it's buf directly? it does say its immutable but maybe some fancy init code can avoid that?

[htkhiem]

does the current way I have it implemented do a return by value or by pointer? If its by value, how can I make sure to not do that?

IIRC, if you don't have to call .clone(), it's by pointer (and sometimes even .clone() is lightweight, such as with Rc). Rust is explicit about expensive copy/alloc operations so you shouldn't have to worry about it doing something grossly inefficient behind your back. If you're interested, you can take a look at the Clone and Copy built-in traits and the differences between them.

To enforce memory safety, however, Rust disallows aliasing, i.e. it forbids us from accessing those same image bytes from a "previous" alias. Take for example your save_and_register_image() function:

    let (hires_img, thumb_img) = resize_convert_image(dyn_img);  // <-- The image objects are originally assigned to these variable names
    let hires_k = save_and_register_single_image(&hires_img, key, prefix, false);
    let thumb_k = save_and_register_single_image(&thumb_img, key, prefix, true);
    // After the above calls the objects are still at their original memory locations & 
    // can still be accessed via their original variable names. This is because the above
    // functions did not "take ownership" of the objects. They were merely given pointers
    // to them.

    // originally `return`ing here but rewritten to a var assignment for ex
    let res = RegisteredImageBundle {
        hires: RegisteredImage{ name: hires_k, img: Some(hires_img) },
        thumb: RegisteredImage{ name: thumb_k, img: Some(thumb_img) }
    };
    // The bytes are now "owned" by `res`. While `res` was TECHNICALLY given
    // pointers too (no move/alloc happened here), Rust will no longer allow 
    // us to refer to the image objects via their old `hires_img` and `thumb_img` 
    // names. For example, the next line will fail to compile:
    // foo(&hires_img);
    
    // Accessing the `img` field of those `RegisteredImage`s by reference is trivially lightweight:
    bar(&res.hires.img);  // no alloc, move or anything
    // But passing by value is a bit weird. The below technically compiles and is _also_ lightweight:
    baz(res.hires.img);  // actually no move or alloc either
    // But running it will cause the next to fail to compile:
    // baz2(res.hires.img);
    // Because we've moved res.hires.img into `baz`, which Rust treats as a _partial move_ of `res`
    // itself. After the `baz` call, the whole `res` variable is technically dropped as `baz` doesn't return
    // it to our scope, leaving zero remaining option to access it given Rust's constraints.
}

The read_texture_from_register looks like its copying the bytes twice, but I'm not 100% sure. I followed the example impl given by the image create, however, glib::Bytes::from explicitly says it copies the data from bytes, so I'm wondering if its not possible to like not do that & to write to it's buf directly?

Yep, Bytes::from() will copy. This is to ensure that it owns the bytes & won't run into dangling pointers down the line. We can avoid that by using Bytes::from_owned(). Note how that constructor requires us to pass the object itself and not a reference, which allows Bytes to keep the image bytes in memory for as long as it lives, via "owning" the bytes outright.

I think there's more to it though. Please check out my new review comment regarding read_texture_from_register.

[htkhiem]

I think this should be an associated function (object method) of RegisteredImage. I can think of two options:

1. Convert the RegisteredImage itself into a gdk::MemoryTexture. We can implement it as an Into<gdk::MemoryTexture> trait that transfers ownership of the bytes to the new texture using glib::Bytes::from_owned for ease of use. This destroys the RegisteredImage but is zero-copy.
2. Create a more traditional associated function like fn texture(&self). This will need at least one copy, but otherwise leaves the RegisteredImage struct intact and doesn't require ownership of that RegisteredImage.

Actually since they're both small functions code-wise I think we can just do both :)

[ShadiestGoat]

Good point - I thought of it, but I don't know why I eventually didn't do that haha

I'll implement them and see the diff. I doubt the latter is required, but let's see...

[htkhiem]

does the current way I have it implemented do a return by value or by pointer? If its by value, how can I make sure to not do that?

IIRC, if you don't have to call .clone(), it's by pointer (and sometimes even .clone() is lightweight, such as with Rc). Rust is explicit about expensive copy/alloc operations so you shouldn't have to worry about it doing something grossly inefficient behind your back. If you're interested, you can take a look at the Clone and Copy built-in traits and the differences between them.

To enforce memory safety, however, Rust disallows aliasing, i.e. it forbids us from accessing those same image bytes from a "previous" alias. Take for example your save_and_register_image() function:

    let (hires_img, thumb_img) = resize_convert_image(dyn_img);  // <-- The image objects are originally assigned to these variable names
    let hires_k = save_and_register_single_image(&hires_img, key, prefix, false);
    let thumb_k = save_and_register_single_image(&thumb_img, key, prefix, true);
    // After the above calls the objects are still at their original memory locations & 
    // can still be accessed via their original variable names. This is because the above
    // functions did not "take ownership" of the objects. They were merely given pointers
    // to them.

    // originally `return`ing here but rewritten to a var assignment for ex
    let res = RegisteredImageBundle {
        hires: RegisteredImage{ name: hires_k, img: Some(hires_img) },
        thumb: RegisteredImage{ name: thumb_k, img: Some(thumb_img) }
    };
    // The bytes are now "owned" by `res`. While `res` was TECHNICALLY given
    // pointers too (no move/alloc happened here), Rust will no longer allow 
    // us to refer to the image objects via their old `hires_img` and `thumb_img` 
    // names. For example, the next line will fail to compile:
    // foo(&hires_img);
    
    // Accessing the `img` field of those `RegisteredImage`s by reference is trivially lightweight:
    bar(&res.hires.img);  // no alloc, move or anything
    // But passing by value is a bit weird. The below technically compiles and is _also_ lightweight:
    baz(res.hires.img);  // actually no move or alloc either
    // But running it will cause the next to fail to compile:
    // baz2(res.hires.img);
    // Because we've moved res.hires.img into `baz`, which Rust treats as a _partial move_ of `res`
    // itself. After the `baz` call, the whole `res` variable is technically dropped as `baz` doesn't return
    // it to our scope, leaving zero remaining option to access it given Rust's constraints.
}

The read_texture_from_register looks like its copying the bytes twice, but I'm not 100% sure. I followed the example impl given by the image create, however, glib::Bytes::from explicitly says it copies the data from bytes, so I'm wondering if its not possible to like not do that & to write to it's buf directly?

Yep, Bytes::from() will copy. This is to ensure that it owns the bytes & won't run into dangling pointers down the line. We can avoid that by using Bytes::from_owned(). Note how that constructor requires us to pass the object itself and not a reference, which allows Bytes to keep the image bytes in memory for as long as it lives, via "owning" the bytes outright.

I think there's more to it though. Please check out my new review comment regarding read_texture_from_register.

[ShadiestGoat]

Hey @htkhiem I added the things you asked. Since the image bytes are written into a separate buffer (due to image format requirements I haven't noticed in the initial impl), from_owned can be used anyways. Please take a look when you get the time, cheers!

[htkhiem]

(optional request) I think I've found a way to further optimise the .texture() calls for newly-downloaded images.

[htkhiem]

I've just found out that since GTK4.16 we have a gdk::MemoryTextureBuilder that, contrary to its name, actually builds normal gdk::Texture objects instead of gdk::MemoryTexture. With that we can specify the byte format/layout and then directly ingest the bytes from RgbImage, no copy/alloc required :).

Can you give this a try? I've just run it on my side and it seems to work, but that's for loading already-cached images. Should you agree with this direction, but don't have time to add to your fork, just let me know and I'll push a commit. If you think this is unnecessarily complex, the existing code is fine too.

Notes:

• The RegisteredImage::img field must now be wrapped inside a RefCell to allow us to yank its data out without marking the whole operation mut. This adds a tiny runtime borrow check but it's far lighter than the alloc it helps avoid.
• I decided to rename fn texture() to fn take_texture() to reflect the new behaviour: calling this will leave the struct in an altered state (stealing the RgbImage). The struct will still be valid after the call, with its img field set to None. You might be able to rename it back to just texture to keep changes in other parts of the code minimal, for quick testing, but I'd recommend using this name.
• I have not yet handled the construction side of things (RegisteredImage now MUST contain only R8g8b8 bytes), but given how you've already made it contain RgbImage specifically instead of DynamicImage I think it should be fine?

My apologies for being pedantic, but overall I think this is worth the trouble as this is a rather hot path called when a new user sets up Euphonica (or maybe after a cache clear) & it can help the cache population process be slightly faster.

Suggested change

	pub struct RegisteredImage {
	/// image name (eg. uayhsjdkjasuijad.png)
	pub name: String,
	/// this field is only present if it is returned by a method that created the image
	pub img: Option<RgbImage>
	}
	impl RegisteredImage {
	pub fn texture(&self) -> Result<gdk::Texture, glib::Error> {
	if let Some(rgb_image) = &self.img {
	let mut bytes: Vec<u8> = Vec::new();
	if let Err(e) = rgb_image.write_to(&mut Cursor::new(&mut bytes), image::ImageFormat::Png) {
	panic!("Somehow failed to write image to an internal buffer: {:#?}", e)
	}
	gdk::Texture::from_bytes(&glib::Bytes::from_owned(bytes)).map_err(|e| { dbg!(&e); e })
	} else {
	let mut res = get_image_cache_path();
	res.push(&self.name);
	gdk::Texture::from_filename(res).map_err(|e| { dbg!(&e); e })
	}
	}
	}
	impl TryInto<gdk::Texture> for RegisteredImage {
	type Error = glib::Error;
	fn try_into(self) -> Result<gdk::Texture, Self::Error> {
	self.texture()
	}
	}
	pub struct RegisteredImageBundle {
	pub hires: RegisteredImage,
	pub thumb: RegisteredImage
	}
	impl RegisteredImageBundle {
	pub fn texture(&self, thumb: bool) -> Result<gdk::Texture, glib::Error> {
	if thumb {
	self.thumb.texture()
	} else {
	self.hires.texture()
	}
	}
	}
	pub struct RegisteredImage {
	/// image name (eg. uayhsjdkjasuijad.png)
	pub name: String,
	/// this field is only present if it is returned by a method that created the image
	pub img: RefCell<Option<RgbImage>>,
	}
	impl RegisteredImage {
	pub fn take_texture(&self) -> Result<gdk::Texture, glib::Error> {
	if let Some(rgb_image) = self.img.take() {
	let builder = gdk::MemoryTextureBuilder::new();
	builder.set_width(rgb_image.width() as i32);
	builder.set_height(rgb_image.height() as i32);
	builder.set_format(gdk::MemoryFormat::R8g8b8);
	builder.set_stride((rgb_image.width() * 3) as usize);
	builder.set_bytes(Some(&glib::Bytes::from_owned(rgb_image.into_raw())));
	Ok(builder.build())
	} else {
	let mut res = get_image_cache_path();
	res.push(&self.name);
	gdk::Texture::from_filename(res).map_err(|e| {
	dbg!(&e);
	e
	})
	}
	}
	}
	impl TryInto<gdk::Texture> for RegisteredImage {
	type Error = glib::Error;
	fn try_into(self) -> Result<gdk::Texture, Self::Error> {
	self.take_texture()
	}
	}
	pub struct RegisteredImageBundle {
	pub hires: RegisteredImage,
	pub thumb: RegisteredImage,
	}
	impl RegisteredImageBundle {
	pub fn take_texture(&self, thumb: bool) -> Result<gdk::Texture, glib::Error> {
	if thumb {
	self.thumb.take_texture()
	} else {
	self.hires.take_texture()
	}
	}
	}