PNG support #60

Merged
merged 1 commit into from Feb 28, 2016

Conversation

Projects
None yet
3 participants
@vojtechkral
Contributor

vojtechkral commented Feb 15, 2016

This depends on CAIRO_HAS_PNG_FUNCTIONS, is that a problem?

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Feb 15, 2016

Member

Uh, do we have an idiomatic error handling solution in cairo?

Looks like they recommend we bind the _stream functions in an idiomatic way (fn create_from_png<R: Read>(r: R) -> Result<ImageSurface, IoError>). IoError might for now be something like enum { Cairo(Status), Io(io::Error) }

As to CAIRO_HAS_PNG_FUNCTIONS, we have cargo features again so it seems natural to put this behind a feature and investigate if it can be enabled by default.

Member

gkoz commented Feb 15, 2016

Uh, do we have an idiomatic error handling solution in cairo?

Looks like they recommend we bind the _stream functions in an idiomatic way (fn create_from_png<R: Read>(r: R) -> Result<ImageSurface, IoError>). IoError might for now be something like enum { Cairo(Status), Io(io::Error) }

As to CAIRO_HAS_PNG_FUNCTIONS, we have cargo features again so it seems natural to put this behind a feature and investigate if it can be enabled by default.

@vojtechkral

This comment has been minimized.

Show comment
Hide comment
@vojtechkral

vojtechkral Feb 16, 2016

Contributor

Ah, yes, binding the _stream functions is probably a better idea... I'm not sure I'd be able to properly cast the stream object to void* for the closure though.

I've noticed gtk-rs projects now generate bindings using GIR, which I don't have much idea about, but this isn't the case with cairo is it?

Contributor

vojtechkral commented Feb 16, 2016

Ah, yes, binding the _stream functions is probably a better idea... I'm not sure I'd be able to properly cast the stream object to void* for the closure though.

I've noticed gtk-rs projects now generate bindings using GIR, which I don't have much idea about, but this isn't the case with cairo is it?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Feb 16, 2016

Member

No it's not unfortunately.

Member

GuillaumeGomez commented Feb 16, 2016

No it's not unfortunately.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Feb 16, 2016

Member

I'm not sure I'd be able to properly cast the stream object to void* for the closure though.

Full-blown closures are hardly needed here but it's possible to write a generic callback e.g.

struct ReadEnv<R: Read> {
    reader: R,
    error: Option<io::Error>,
}

extern fn read_func<R: Read>(closure: *mut c_void, data: *mut u8, len: c_uint) -> Status {
    CallbackGuard; // "borrow" this from glib or gtk crate
    unsafe {
        let read_env: &mut ReadEnv<R> = &mut *(closure as *mut ReadEnv);
        ...

You might encounter some pushback from the borrow checker but the lifetime of R should not be a practical problem here.

Member

gkoz commented Feb 16, 2016

I'm not sure I'd be able to properly cast the stream object to void* for the closure though.

Full-blown closures are hardly needed here but it's possible to write a generic callback e.g.

struct ReadEnv<R: Read> {
    reader: R,
    error: Option<io::Error>,
}

extern fn read_func<R: Read>(closure: *mut c_void, data: *mut u8, len: c_uint) -> Status {
    CallbackGuard; // "borrow" this from glib or gtk crate
    unsafe {
        let read_env: &mut ReadEnv<R> = &mut *(closure as *mut ReadEnv);
        ...

You might encounter some pushback from the borrow checker but the lifetime of R should not be a practical problem here.

@vojtechkral vojtechkral changed the title from Basic PNG support to PNG support Feb 16, 2016

@vojtechkral

This comment has been minimized.

Show comment
Hide comment
@vojtechkral

vojtechkral Feb 16, 2016

Contributor

Ah, shoot, can't use #![feature(read_exact)] on stable...

Contributor

vojtechkral commented Feb 16, 2016

Ah, shoot, can't use #![feature(read_exact)] on stable...

Cargo.toml
@@ -18,6 +18,7 @@ name = "cairo"
[features]
"1.12" = ["cairo-sys-rs/1.12"]
+"png-functions" = [ "cairo-sys-rs/png-functions" ]

This comment has been minimized.

@gkoz

gkoz Feb 16, 2016

Member

I'd prefer just png.

@gkoz

gkoz Feb 16, 2016

Member

I'd prefer just png.

src/image_surface.rs
Format,
SurfaceType,
};
use surface::{Surface, SurfaceExt, SurfacePriv};
+#[cfg(feature = "png-functions")]
+mod png_functions {

This comment has been minimized.

@gkoz

gkoz Feb 16, 2016

Member

It seems tidier to put all png stuff in a separate image_surface_png module

@gkoz

gkoz Feb 16, 2016

Member

It seems tidier to put all png stuff in a separate image_surface_png module

src/image_surface.rs
+ Status::Success => Ok(surface),
+ Status::ReadError => Err(io::Error::new(io::ErrorKind::InvalidData, "Invalid PNG data.")),
+ Status::NoMemory => Err(io::Error::new(io::ErrorKind::InvalidData, "Could not allocate enough memory.")),
+ _ => Err(io::Error::new(io::ErrorKind::InvalidData, "Unknown error.")),

This comment has been minimized.

@gkoz

gkoz Feb 16, 2016

Member

It doesn't seem right to just hide Status. Clearly io::Error can't reflect some failure modes, notably NoMemory.

Until we have a good error handling story an IoError enum I suggested earlier seems preferable.

@gkoz

gkoz Feb 16, 2016

Member

It doesn't seem right to just hide Status. Clearly io::Error can't reflect some failure modes, notably NoMemory.

Until we have a good error handling story an IoError enum I suggested earlier seems preferable.

This comment has been minimized.

@vojtechkral

vojtechkral Feb 16, 2016

Contributor

Ok, sure, I can do that...

The reason I haven't done this right away is that, at least as of current Cairo implementation, the error return codes seem to be pretty much random when Cairo is fed an invalid PNG. It will either return a ReadError or NoMemory, presumably depending on the kind of corruption of the PNG data, but we can't really tell whether we're actually out of memory or whether the PNG was just corrupted...

@vojtechkral

vojtechkral Feb 16, 2016

Contributor

Ok, sure, I can do that...

The reason I haven't done this right away is that, at least as of current Cairo implementation, the error return codes seem to be pretty much random when Cairo is fed an invalid PNG. It will either return a ReadError or NoMemory, presumably depending on the kind of corruption of the PNG data, but we can't really tell whether we're actually out of memory or whether the PNG was just corrupted...

This comment has been minimized.

@gkoz

gkoz Feb 17, 2016

Member

Eh, they do seem to take some shortucts but then also return a number of statuses that make sense.

@gkoz

gkoz Feb 17, 2016

Member

Eh, they do seem to take some shortucts but then also return a number of statuses that make sense.

src/image_surface.rs
@@ -44,6 +110,31 @@ impl ImageSurface {
}
}
+ #[cfg(feature = "png-functions")]
+ pub fn create_from_png_stream<R: Read>(stream: R) -> io::Result<ImageSurface> {

This comment has been minimized.

@gkoz

gkoz Feb 16, 2016

Member

I believe the best approximation of this guideline would be to avoid the _stream suffix as this is the more generic version. Optionally there could be variants that take P: AsRef<Path>.

@gkoz

gkoz Feb 16, 2016

Member

I believe the best approximation of this guideline would be to avoid the _stream suffix as this is the more generic version. Optionally there could be variants that take P: AsRef<Path>.

This comment has been minimized.

@vojtechkral

vojtechkral Feb 17, 2016

Contributor

Hm, I'm not sure I follow... I can just omit the Path version? Or, if not, what should it be named?

@vojtechkral

vojtechkral Feb 17, 2016

Contributor

Hm, I'm not sure I follow... I can just omit the Path version? Or, if not, what should it be named?

This comment has been minimized.

@gkoz

gkoz Feb 17, 2016

Member

Personally I see little value in providing Path versions that would amount to e.g.

create_from_png(try!(File::open(path)))
@gkoz

gkoz Feb 17, 2016

Member

Personally I see little value in providing Path versions that would amount to e.g.

create_from_png(try!(File::open(path)))
src/image_surface.rs
+ Ok(()) => Status::Success,
+ Err(error) => {
+ write_env.error = Some(error);
+ Status::ReadError

This comment has been minimized.

@gkoz

gkoz Feb 16, 2016

Member

I'd feel safer with WriteError here ;)

@gkoz

gkoz Feb 16, 2016

Member

I'd feel safer with WriteError here ;)

This comment has been minimized.

@vojtechkral

vojtechkral Feb 16, 2016

Contributor

Damn :)

@vojtechkral

vojtechkral Feb 16, 2016

Contributor

Damn :)

src/image_surface.rs
@@ -15,7 +15,7 @@ use ffi::enums::{
use surface::{Surface, SurfaceExt, SurfacePriv};
#[derive(Debug)]
-pub struct ImageSurface(Surface);
+pub struct ImageSurface(pub Surface);

This comment has been minimized.

@gkoz

gkoz Feb 21, 2016

Member

This should not be necessary. If it is there's a bug somewhere.

@gkoz

gkoz Feb 21, 2016

Member

This should not be necessary. If it is there's a bug somewhere.

src/image_surface_png.rs
+}
+
+impl ImageSurfacePng for ImageSurface {
+ fn create_from_png<R: Read>(stream: R) -> Result<ImageSurface, PngError> {

This comment has been minimized.

@gkoz

gkoz Feb 21, 2016

Member

You can just impl ImageSurface without introducing a trait.

@gkoz

gkoz Feb 21, 2016

Member

You can just impl ImageSurface without introducing a trait.

src/image_surface_png.rs
+
+
+#[derive(Debug)]
+pub enum PngError { Cairo(Status), Io(Error) }

This comment has been minimized.

@gkoz

gkoz Feb 21, 2016

Member

There's nothing tying this enum to png. I'd rather it had a more generic name e.g. IoError and live in its own module, say error.

@gkoz

gkoz Feb 21, 2016

Member

There's nothing tying this enum to png. I'd rather it had a more generic name e.g. IoError and live in its own module, say error.

src/image_surface_png.rs
+ }
+ }
+ if !buffer.is_empty() {
+ read_env.error = Some(Error::new(ErrorKind::Other, "failed to fill whole buffer"));

This comment has been minimized.

@gkoz

gkoz Feb 21, 2016

Member

Would ErrorKind::UnexpectedEof be appropriate here?

@gkoz

gkoz Feb 21, 2016

Member

Would ErrorKind::UnexpectedEof be appropriate here?

This comment has been minimized.

@gkoz

gkoz Feb 21, 2016

Member

Hey, read_exact is stable in 1.6. We can use it.

@gkoz

gkoz Feb 21, 2016

Member

Hey, read_exact is stable in 1.6. We can use it.

This comment has been minimized.

@vojtechkral

vojtechkral Feb 23, 2016

Contributor

Hmm. I must've had some outdated nightly rust version accidentally... That's what I get for switching various rustc versions...

@vojtechkral

vojtechkral Feb 23, 2016

Contributor

Hmm. I must've had some outdated nightly rust version accidentally... That's what I get for switching various rustc versions...

src/image_surface_png.rs
+ fn create_from_png<R: Read>(stream: R) -> Result<ImageSurface, PngError> {
+ let mut env = ReadEnv{ reader: stream, error: None };
+ let surface: ImageSurface = unsafe { from_glib_full(ffi::cairo_image_surface_create_from_png_stream(
+ transmute(read_func::<R>), transmute(&mut env))) };

This comment has been minimized.

@gkoz

gkoz Feb 21, 2016

Member

It should not be necessary to transmute read_func, isn't its signature compatible with cairo_read_func_t?
Also in the interest of avoiding transmutes I'd prefer &mut env as *mut ReadEnv as *mut c_void.

@gkoz

gkoz Feb 21, 2016

Member

It should not be necessary to transmute read_func, isn't its signature compatible with cairo_read_func_t?
Also in the interest of avoiding transmutes I'd prefer &mut env as *mut ReadEnv as *mut c_void.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Feb 23, 2016

Member

I'm still a little worried about error handling: cairo_surface_write_to_png_stream seems capable of returning non-zero results, including NoMemory.

Member

gkoz commented Feb 23, 2016

I'm still a little worried about error handling: cairo_surface_write_to_png_stream seems capable of returning non-zero results, including NoMemory.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Feb 23, 2016

Member

@gkoz: As long as they're wrapped, there should be no problem, no?

Member

GuillaumeGomez commented Feb 23, 2016

@gkoz: As long as they're wrapped, there should be no problem, no?

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Feb 23, 2016

Member

Unlike create_from_png, the present version of write_to_png ignores them.

Member

gkoz commented Feb 23, 2016

Unlike create_from_png, the present version of write_to_png ignores them.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Feb 23, 2016

Member

Right, I thought cairo handled it internally.

Member

GuillaumeGomez commented Feb 23, 2016

Right, I thought cairo handled it internally.

@vojtechkral

This comment has been minimized.

Show comment
Hide comment
@vojtechkral

vojtechkral Feb 23, 2016

Contributor

Well, I forgot to commit the file :-/
And yes the status should be checked too. I'll have a look at it tomorrow...

Contributor

vojtechkral commented Feb 23, 2016

Well, I forgot to commit the file :-/
And yes the status should be checked too. I'll have a look at it tomorrow...

@vojtechkral

This comment has been minimized.

Show comment
Hide comment
@vojtechkral

vojtechkral Feb 28, 2016

Contributor

Ok, there it is I hope. Sorry for the delay. Squashing needed?

Contributor

vojtechkral commented Feb 28, 2016

Ok, there it is I hope. Sorry for the delay. Squashing needed?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Feb 28, 2016

Member

Yes please.

Member

GuillaumeGomez commented Feb 28, 2016

Yes please.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Feb 28, 2016

Member

Consider implementing std::error::Error for Error but I won't demand that.
Other than that 👍

Member

gkoz commented Feb 28, 2016

Consider implementing std::error::Error for Error but I won't demand that.
Other than that 👍

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Feb 28, 2016

Member

@gkoz: Open an issue. :p

Member

GuillaumeGomez commented Feb 28, 2016

@gkoz: Open an issue. :p

@vojtechkral

This comment has been minimized.

Show comment
Hide comment
@vojtechkral

vojtechkral Feb 28, 2016

Contributor

I'd rather not elaborate on the error handling, there seems to be a lot of space for design choices...

Contributor

vojtechkral commented Feb 28, 2016

I'd rather not elaborate on the error handling, there seems to be a lot of space for design choices...

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Feb 28, 2016

Member

Fair enough.

Thanks for the PR!

Member

gkoz commented Feb 28, 2016

Fair enough.

Thanks for the PR!

gkoz added a commit that referenced this pull request Feb 28, 2016

@gkoz gkoz merged commit 73a357f into gtk-rs:master Feb 28, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment