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

Writer needs an .into_inner() function #298

Closed
surferjeff opened this issue Jul 17, 2021 · 10 comments
Closed

Writer needs an .into_inner() function #298

surferjeff opened this issue Jul 17, 2021 · 10 comments

Comments

@surferjeff
Copy link

I want to write a PNG to memory. I wrote code like this:

    let mut encoder = png::Encoder::new(Cursor::new(Vec::new()), info.width, info.height);
    encoder.set_color(png::ColorType::RGB);
    encoder.set_depth(info.bit_depth);
    let mut writer = encoder.write_header()?;
    writer.write_image_data(&frame)?;
    let cursor = writer.into_inner();
    let bytes = cursor.into_inner();

I can't get the bytes back out without an into_inner() function. The line let cursor = writer.into_inner(); fails to compile because there's no .into_inner() function.

surferjeff added a commit to surferjeff/image-png that referenced this issue Jul 17, 2021
@fintelia
Copy link
Contributor

Couldn't you do:

let mut bytes = Vec::new();
{
    let mut encoder = png::Encoder::new(Cursor::new(&mut bytes), info.width, info.height);
    encoder.set_color(png::ColorType::RGB);
    encoder.set_depth(info.bit_depth);
    let mut writer = encoder.write_header()?;
    writer.write_image_data(&frame)?;
}

@surferjeff
Copy link
Author

Yes, that worked. Thanks!

@ravenexp
Copy link

ravenexp commented Nov 5, 2021

Couldn't you do:

let mut bytes = Vec::new();
{
    let mut encoder = png::Encoder::new(Cursor::new(&mut bytes), info.width, info.height);
    encoder.set_color(png::ColorType::RGB);
    encoder.set_depth(info.bit_depth);
    let mut writer = encoder.write_header()?;
    writer.write_image_data(&frame)?;
}

This workaround no longer works in 0.17:

error[E0597]: `pngbuf` does not live long enough
  --> src/png.rs:31:52
   |
31 |         let mut encoder = Encoder::new(Cursor::new(&mut pngbuf), self.width, self.height);
   |                                        ------------^^^^^^^^^^^-
   |                                        |           |
   |                                        |           borrowed value does not live long enough
   |                                        argument requires that `pngbuf` is borrowed for `'static`
...
50 |     }
   |     - `pngbuf` dropped here while still borrowed

I believe this is because Encoder::new() now enforces a 'static lifetime parameter for some reason:

image-png/src/encoder.rs

Lines 173 to 174 in c24fc93

impl<'a, W: Write> Encoder<'a, W> {
pub fn new(w: W, width: u32, height: u32) -> Encoder<'static, W> {

I think this issue should be reopened for version 0.17 and #299 may be revived as well.

@fintelia
Copy link
Contributor

fintelia commented Nov 5, 2021

Could you check why the static bound was added? Might be that we could remove that instead

@ravenexp
Copy link

ravenexp commented Nov 5, 2021

It was added in 7ed5169

I don't understand the motivation behind it well though...

@fintelia
Copy link
Contributor

fintelia commented Nov 5, 2021

I don't quite understand that either, but it looks like the static bound isn't meant to apply to the writer at all!

@HeroicKatora
Copy link
Member

It still works: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=769e84f77aae22624b26ec51df1a74c1

The return value does not force W: 'static. Can you link to full code?

@ravenexp
Copy link

ravenexp commented Nov 5, 2021

I believe Rust Playground imports png version 0.16. The API I'm using in 0.17 is not there:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=037ccad055f3d5f8862707dacfbdd41a

@HeroicKatora
Copy link
Member

HeroicKatora commented Nov 5, 2021

Okay, that's a separate problem for StreamWriter then. As a workaround you can use stream_writer instead of into_stream_writer. This requires you to keep the underlying Writer alive and borrow from it like so:

            let mut writer = encoder
                .write_header()?;
            let mut writer = writer
                .stream_writer()?;

So it only works as long as you do not want to store the StreamWriter into a 'static struct itself but then the lifetimes work out. It seems that the requirement W: 'static is actually introduced implicitly as part of the StreamWriter type:

Specifically in this enum:

image-png/src/encoder.rs

Lines 981 to 984 in c24fc93

enum ChunkOutput<'a, W: Write> {
Borrowed(&'a mut Writer<W>),
Owned(Writer<W>),
}

This links the lifetime parameter to the Writer because the reference in the type definition implies Writer<W>: 'a which in turn requires W: 'a. Further the output type of into_stream_writer() is StreamWriter<'static, W> implying W: 'static due to the above portion. This does not apply to the Writer::new constructor because the type Encoder has no such link, its lifetime parameter is only used for an &'a Info structure and later lost when converting to Writer in writer_header.

@ravenexp
Copy link

ravenexp commented Nov 5, 2021

Thanks!

Okay, that's a separate problem for StreamWriter then. As a workaround you can use stream_writer instead of into_stream_writer. This requires you to keep the underlying Writer alive and borrow from it like so:

            let mut writer = encoder
                .write_header()?;
            let mut writer = writer
                .stream_writer()?;

worked for me.

I'd happily avoid using StreamWriter if there was an easy way to pass 16-bit samples directly to Writer.
I also had to google to find out what sample data endianness PNG files use internally.
Ideally, such file format details should not leak through the high level library interface...

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 a pull request may close this issue.

4 participants