search

cessen / openexr-rs

This repository has been moved to https://github.com/vfx-rs/openexr-rs
MIT License
23 stars 10 forks source link

FrameBuffer can't read immutable pixel data #12

Closed Ralith closed 7 years ago

Ralith commented 7 years ago

Because FrameBuffer is used by both input and output code, it takes mutable references to pixel storage, but this is excessively strict for many use-cases.

This could be addressed by making FrameBuffer a trait and providing both mutable and immutable versions, with output code taking a generic T: FrameBuffer and input code taking only a concrete FrameBufferMut. Both mutable and immutable versions would be very thin wrappers around an unsafe (and perhaps unexported) common object that contains the C API wrangling.

Alternatively, mutability could be stored per-channel and safety checked at runtime a la RefCell, but this strikes me as less desirable.

cessen commented 7 years ago

Yeah, I'd like to address this as well!

I've been trying to think if there's some more elegant way we can deal with this, but I'm coming up blank. I think your idea of splitting the type and using a trait for output probably makes the most sense. I would definitely rather have this enforced at compile time than panic at runtime.

cessen commented 7 years ago

So, I think I'm having a change of heart. Although it would be nice to statically guarantee correct usage at compile time, as I started creating the new types and traits to make it work it all seemed kind of... overkill, I guess. And it complicates the API's more than I think is necessary. The trade-off doesn't seem worth it to me.

So I've taken a different approach. I've changed the insert_channel and insert_pixels methods to have both const and _mut versions. The const versions increment a const_channel_count field in the FrameBuffer struct. When any code asks for a handle to the C++ handle they have to specify if they want it const or mut. In the case of mut, it checks to see if self.const_channel_count > 0 and panics if so.

There is also a is_const method (for internal use), so we can give nicer error messages at the point of use. I'll make a pull request soon.

Ralith commented 7 years ago

I haven't seen the code, but I feel like having twice the number of accessor methods on one type, and statically-unpredictable panics, represents a considerably greater increase in API complexity than merely introducing an immutable framebuffer type.

cessen commented 7 years ago

Hashed it out on IRC and came up with something even better.

Completed in PR #16.