Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update image to 0.24 #494

Merged
merged 2 commits into from
Feb 7, 2022
Merged

Update image to 0.24 #494

merged 2 commits into from
Feb 7, 2022

Conversation

avitex
Copy link
Contributor

@avitex avitex commented Feb 6, 2022

An allocation has been removed for writing a PNG data-url in piet-svg.

Rather than going through DynamicImage::write_to, we just take the bytes and write directly with PngEncoder. This is due to write_to now requiring an io::Seek constraint which EncoderStringWriter does not implement.

Note PngEncoder::encode is now deprecated, with a note pointing to ImageEncoder::write_image instead. Note also that ImageEncoder::write_image expects image buffers with color types with 16-bit per channel or larger to be native endian, which differs to the semantics of PngEncoder::encode.

@avitex avitex changed the title Update image to 0.24. Update image to 0.24 Feb 6, 2022
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I haven't tested this out but it looks reasonable, so I'm happy to merge and clean up any potential issues as they arise. :)

Comment on lines +464 to +468
let mut writer = base64::write::EncoderStringWriter::from(
String::from("data:image/png;base64,"),
base64::STANDARD,
);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how feasible this is, but in theory if we could estimate the image's final b64 length then we could preallocate the buffer, as in:

Suggested change
let mut writer = base64::write::EncoderStringWriter::from(
String::from("data:image/png;base64,"),
base64::STANDARD,
);
static PRELUDE: &str = "data:image/png;base64,"'
let encoded_len = guess_encoded_len_somehow() + PRELUDE.len();
let mut buf = String::with_capacity(encoded_len);
buf.push_str(PRELUDE);
let mut writer = base64::write::EncoderStringWriter::from(
&mut buf,
base64::STANDARD,
);

This is totally unnecessary but I thought I'd point it out for fun. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be fun actually. I'm starting to have a look at druid for a slow burn project with a lot of yak shaving. Part of that will be starting to dive a little into the stack and helping out where I can.

I gave this a test with picture_2 and the pictures render the same to current state. I will note the PngEncoder (with either write_image/encode) has a slightly larger output. I didn't look too deep into it, was more looking for correct output.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make such changes in a separate PR

@cmyr
Copy link
Member

cmyr commented Feb 7, 2022

@avitex you may need to rebase this on master to pass CI, I've fixed an issue that was causing it to fail.

- An allocation has been removed for writing a PNG data-url in `piet-svg`.
@avitex
Copy link
Contributor Author

avitex commented Feb 7, 2022

@cmyr done :)

@avitex
Copy link
Contributor Author

avitex commented Feb 7, 2022

@cmyr Pushed up a change to fix the previous run

@cmyr cmyr merged commit f19a261 into linebender:master Feb 7, 2022
@avitex avitex deleted the update-image branch February 7, 2022 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants