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

should load(::Stream, ...) methods skip magic bytes? #163

Open
ssfrr opened this issue Nov 25, 2017 · 1 comment · May be fixed by #177
Open

should load(::Stream, ...) methods skip magic bytes? #163

ssfrr opened this issue Nov 25, 2017 · 1 comment · May be fixed by #177

Comments

@ssfrr
Copy link
Contributor

ssfrr commented Nov 25, 2017

In the README, the example for loading from a stream is:

function load(f::File{format"PNG"})
  open(f) do s
  skipmagic(s) # skip over the magic bytes
  # You can just call the method below...
  ret = load(s)
  # ...or implement everything here instead
  end
end

# You can support streams and add keywords:
function load(s::Stream{format"PNG"}; keywords...)
  # s is already positioned after the magic bytes
  # Do the stuff to read a PNG file
  chunklength = read(s, UInt32)
  ...
end

This works if the user calls load("somefile.png"), which will end up calling the load(::File, ...) method, which handles skipping the magic bytes before calling load(::Stream). What I'm not super clear on is how this is intended to be used directly. e.g. I couldn't just do:

open("somefile.png") do io
    load(io)
end

In ImageMagick.jl it looks like load is called on the IO stream directly without skipping the magic(k) bytes.

So if it seems like the way ImageMagick does it (leaving the stream at the very beginning of the file when calling load(::Stream)) I can put in a documentation PR that clarifies that.

@timholy
Copy link
Member

timholy commented Nov 27, 2017

I can't remember why it was done this way. I think I've encountered formats where it makes more sense to keep the magic bytes in the stream. So I'm fine with this change.

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.

2 participants