Replace mupdf with hayro#156
Conversation
there's a lot of FIXME:s, commented out/`cfg`:ed out a lot of functionality and finally, the rendered page seems to be in some weird darkmode where white turns black? But its a PoC!
|
Are you interested in reviews right now or would you prefer to be able to work on this for a bit longer before I take a look? |
| Opening(std::io::Error), | ||
| Doc(hayro::hayro_syntax::LoadPdfError), | ||
| UnknownPageNum(usize), |
There was a problem hiding this comment.
are these reasonable errors or too-much-information?
There was a problem hiding this comment.
These seem reasonable to me 👍 I rarely think errors contain too much information
sermuns
left a comment
sermuns
left a comment
There was a problem hiding this comment.
It is very much unfinished, especially I have #[cfg(false)]'d out a lot of logic related to searching, getting results.
But there are already questions I'd love to get your feedback on!
| // And although the font size could theoretically change, we aren't accounting for that right | ||
| // now, so we just use the values passed in. | ||
|
|
||
| let mut stored_doc = None; |
There was a problem hiding this comment.
Actually, I think stored_doc is good to have, since something needs to hold on to the PDF so the hayro cache in line 317 can hold a reference to it.
I was too lazy to fully understand the lifetime implications, so that's why I cheated and am completely dropping the hayro cache and recreating it every reload loop.
There was a problem hiding this comment.
Mm, yeah - give me a shout if you can't figure it out, 'cause we definitely want to avoid recreating that cache if possible.
If hayro keeps its own cache of rendering, we may want to also look into how that works and if we can leverage it for an easy version of some sort of LRU cache (like was discussed in #127) - but that can come later.
| // i'm not a pdf_file | ||
| // TODO: remove the comment above |
There was a problem hiding this comment.
| // i'm not a pdf_file | |
| // TODO: remove the comment above |
that's a joke..... sorry
| // if stored_doc.is_some() { | ||
| // sender.send(Ok(RenderInfo::Reloaded))?; | ||
| // } | ||
| d |
There was a problem hiding this comment.
this is wrong, I was just being lazy. related to lifetime issues described in other comments
| let hayro_cache = hayro::RenderCache::default(); | ||
| let hayro_interpreter_settings = hayro_interpret::InterpreterSettings::default(); | ||
| let hayro_render_settings = hayro::RenderSettings::default(); // FIXME: should be configurable! |
There was a problem hiding this comment.
all of these should be placed outside of loop, but related to lifetime issues.
| // FIXME: can we avoid taking the | ||
| // slice and constructing Vec, just | ||
| // steal the existing bufer instead? | ||
| pixels: pixmap.data_as_u8_slice().to_vec(), |
| height_px: h | ||
| }, | ||
| page_num, | ||
| result_rects: Vec::new() // FIXME: do this! |
There was a problem hiding this comment.
i haven't looked at any of the search logic yet. TODO!
| # epub = ["mupdf/epub"] | ||
| # cbz = ["mupdf/cbz"] |
There was a problem hiding this comment.
unfortunately I don't think hayro supports these..
itsjunetime
left a comment
itsjunetime
left a comment
There was a problem hiding this comment.
Cool, yeah - loving this so far, thanks for the work you've put in.
I would really love to add in hayro support (when this code is at a slightly cleaner place, ofc), but would probably feel most comfortable putting it behind a feature flag (probably both a cargo flag and a commandline switch to configure which backend to use). I wouldn't want to completely remove the ability to view epubs or do searches or support some obscure pdf feature that only mupdf works with.
But for the vast majority of people, hayro probably will do the job better, so it would be nice to get it in.
Feel free to keep developing this however feels easiest to you and we can figure out the annoying job of feature-gating and allowing people to use both mupdf and hayro whenever we get to that point. It shouldn't be bad, just a bit annoying.
| let mut dyn_img = DynamicImage::from_decoder(decoder) | ||
| .map_err(|e| RenderError::Converting(format!("Can't load image: {e}")))? | ||
| .into_rgb8(); | ||
| let mut dyn_img = DynamicImage::ImageRgba8(rgba_image).into_rgb8(); |
There was a problem hiding this comment.
I think we can essentially just remove this line, right? And then down below, we modify the let dyn_img = DynamicImage::ImageRgb8(dyn_img) into let dyn_img = DynamicImage::ImageRgba8(rgba_image)?
We just have this line because before we were using a decoder, but now we construct an image buffer directly.
| } | ||
| }; | ||
| let n_pages = | ||
| NonZeroUsize::new(pdf.pages().len()).expect("PDF always has non-zero amount of pages"); |
There was a problem hiding this comment.
I would agree that this is probably true, but I think maybe we could just like short-circuit to the bottom of the loop here instead of panicking. If there are no pages, we can just not render anything.
| // FIXME: can we avoid taking the | ||
| // slice and constructing Vec, just | ||
| // steal the existing bufer instead? | ||
| pixels: pixmap.data_as_u8_slice().to_vec(), |
There was a problem hiding this comment.
I think we can do bytemuck::cast(pixmap.take())? Since that's what data_as_u8_slice uses under the hood?
| // slice and constructing Vec, just | ||
| // steal the existing bufer instead? | ||
| pixels: pixmap.data_as_u8_slice().to_vec(), | ||
| // FIXME: do we really need these floating point ops? |
There was a problem hiding this comment.
Mmmm yeah maybe not - if we're just casting it back into a u16 in the end, it's probably basically the same result (or close enough) to just do u16 operations. Feel free to remove them and we can see how it turns out.
| height_px: h | ||
| }, | ||
| page_num, | ||
| result_rects: Vec::new() // FIXME: do this! |
There was a problem hiding this comment.
mmm does hayro support this? or will we need to pull in another crate (probably one of the pure-rust crates that support reading pdf files) to do this?
2 participants
Stupid and incomplete code, but working proof-of-concept.
Closes #107.