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

Use io::Write::write_all #94

Merged
merged 2 commits into from
Nov 27, 2018
Merged

Use io::Write::write_all #94

merged 2 commits into from
Nov 27, 2018

Conversation

nox
Copy link
Contributor

@nox nox commented Nov 13, 2018

It's incorrect to use io::Write::write and ignore how many bytes were actually written in the underlying writer.

It's incorrect to use io::Write::write and ignore how many bytes were
actually written in the underlying writer.
@marshallpierce
Copy link
Contributor

marshallpierce commented Nov 13, 2018

This snippet demonstrates the issue, extracted from me tinkering in base64 to see what was going on:


#[test]
fn png() {
    let mut rng = rand::thread_rng();
    let mut input_bytes = Vec::<u8>::new();
    let mut png_data = Vec::<u8>::new();

    // A really ugly 200x200 rgba
    for _ in 0..(200 * 200 * 4) {
        input_bytes.push(rng.gen());
    }

    // encode into a Vec
    self::image::png::PNGEncoder::new(&mut png_data)
        .encode(&input_bytes, 200, 200, self::image::ColorType::RGBA(8))
        .unwrap();

    // and b64 the vec
    let simple_b64 = encode_config(&png_data, ::STANDARD);

    // encode into a b64 streaming encoder
    let mut stream_png_b64 = Vec::new();

    {
        let mut b64_encoder = EncoderWriter::new(&mut stream_png_b64, ::STANDARD);

        self::image::png::PNGEncoder::new(&mut b64_encoder)
            .encode(&input_bytes, 200, 200, self::image::ColorType::RGBA(8))
            .unwrap();

        b64_encoder.finish().unwrap();
    }

    assert_eq!(simple_b64, ::std::str::from_utf8(&stream_png_b64).unwrap());
}

With debug logging in base64 added to show when the streaming encoder can't write the full input it's given, I get this:

input = 160236, consumed = 768

I'm working on getting a failing test case and possibly a fix.

marshallpierce added a commit to marshallpierce/image-png that referenced this pull request Nov 13, 2018
@marshallpierce
Copy link
Contributor

I changed the roundtrip test to use all available test images, which worked fine but only for images whose line_size was 32, so the test skips all other images. For the affected images, encoding fails with errors like "wrong data size, expected 65536 got 196608". What's up with that? Just a guess, but perhaps the decode buffer is sized conservatively, but since next_frame doesn't return the number of bytes decoded, there's no way to only encode the relevant portion of the decode buffer?

@nox
Copy link
Contributor Author

nox commented Nov 20, 2018

I added your commit, this is now ready to be reviewed.

@nox
Copy link
Contributor Author

nox commented Nov 20, 2018

@bvssvni?

@nox nox mentioned this pull request Nov 27, 2018
@bvssvni
Copy link
Contributor

bvssvni commented Nov 27, 2018

Merging.

@bvssvni bvssvni merged commit 5f15a33 into image-rs:master Nov 27, 2018
@nox
Copy link
Contributor Author

nox commented Nov 28, 2018

@bvssvni Can you make a new png release, please?

@nox nox deleted the write-all branch November 28, 2018 09:30
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.

3 participants