added DecodingResult struct, changed decoding and predictor API to allow in-place
#103
Conversation
|
oof it seems putting the change at the decoder step introduces some Python headaches. this pyo3 issue seems relevant. Mainly that |
|
Made it a draft, because currently the Python side would need changes as well and I'm as of yet inexperienced with Rust-Python things. Can look into it further after wednesday. For the moment, any thoughts @weiji14? especially if changing to typed array at the unpredicting step is the way to go? (would be slightly sad because of the extra copy, but that's ok and a good compromise I think - if the python stuff turns out to be a lot/difficult or too breaking) alternatively, this PR could be split into:
|
Yes please, might be good to split this into two (I'm generally in favour of reviewing smaller PRs). I'm wondering if there's an opportunity ot use |
will do! I'll keep it in one step for now, because most changes are coherent and basically I won't know what is needed before I see things working together. So the plan:
Is I've given the whole Python part a bit more thought. passing a impl Decoder for PyDecoder {
fn decode_tile(
&self,
compressed_buffer: Bytes,
result_buffer: &mut[u8],
_photometric_interpretation: PhotometricInterpretation,
_jpeg_tables: Option<&[u8]>,
) -> AsyncTiffResult<()> {
let decoded_buffer = Python::with_gil(|py| self.call(py, buffer))
.map_err(|err| AsyncTiffError::General(err.to_string()))?;
result_buffer.copy_from_slice(&decoded_buffer.into_inner()); //only add this extra copy
Ok(())
}
}then there is a bit of sadness (needless copy) in the case of a user-implemented decoder and floating point predictor, but solving that would also be unnecessarily ugly for saving only a copy. |
No, that |
| macro_rules! integral_slice_as_bytes{($int:ty, $const:ident $(,$mut:ident)*) => { | ||
| pub(crate) fn $const(slice: &[$int]) -> &[u8] { | ||
| assert!(mem::align_of::<$int>() <= mem::size_of::<$int>()); | ||
| unsafe { slice::from_raw_parts(slice.as_ptr() as *const u8, mem::size_of_val(slice)) } | ||
| } | ||
| $(pub(crate) fn $mut(slice: &mut [$int]) -> &mut [u8] { | ||
| assert!(mem::align_of::<$int>() <= mem::size_of::<$int>()); | ||
| unsafe { slice::from_raw_parts_mut(slice.as_mut_ptr() as *mut u8, mem::size_of_val(slice)) } | ||
| })* | ||
| }} |
There was a problem hiding this comment.
I'm a pretty strong 👎 on having any unsafe in our code, especially for a transmute like this.
| compressed_buffer: Bytes, | ||
| result_buffer: &mut [u8], |
There was a problem hiding this comment.
It's not clear to me that passing in a result buffer is a meaningful improvement. I'd much rather use bytes::Bytes (i.e. an Arc<Vec<u8>>) that provides cheap cloning. (I.e. keep the existing API)
This is another reason why we should use |
So my final snippet did not change anything Python-API side and solved the issue I raised here. To keep everything Bytes, one would: impl Decoder for PyDecoder {
fn decode_tile(
&self,
compressed_buffer: Bytes,
result_buffer: &mut[u8],
_photometric_interpretation: PhotometricInterpretation,
_jpeg_tables: Option<&[u8]>,
) -> AsyncTiffResult<()> {
let decoded_buffer = Python::with_gil(|py| self.call(py, buffer))
.map_err(|err| AsyncTiffError::General(err.to_string()))?;
result_buffer.copy_from_slice(&decoded_buffer.into_inner()); //only add this extra copy
Ok(())
}
}...I guess it could indeed have some more thought |
closes #87 also #86 is somewhat relevant.
makes the output of a decoding operation a typed array.
The goals:
the problem:
lzw, jpeg) not. uncompressed copies because of 1The typed array can be filled in different places:
this PR does it during the decoding step, which:
decode_into.It changes quite some public api, so open to discussion. more comments coming on the changes
sidenote: there is a redundant copy with no compression and floating point prediction, but which sensible person would ever have that configuration?