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

Intended breakage in 0.17 #213

Closed
9 of 12 tasks
HeroicKatora opened this issue Jun 1, 2020 · 4 comments
Closed
9 of 12 tasks

Intended breakage in 0.17 #213

HeroicKatora opened this issue Jun 1, 2020 · 4 comments
Projects

Comments

@HeroicKatora
Copy link
Member

HeroicKatora commented Jun 1, 2020

A list of all the interface incompatible changes we could clean up in the next major version:

  • Add a feature to enable only no_std (with alloc) compatible interfaces (Probably too complicated)
  • Decoder::read_info returns an overly condensed form of Info.
  • Info has only public members but we will want to add more later (Tweak the Info struct #291).
  • Reader::next_row/Reader::next_interlaced_row should have named types
  • StreamWriter moves lifetime into generic W: Write type parameter ( actually, why? Otherwise we have another Write implementor, &mut W, in the interface )
  • Reader::next_interlaced_row should use wrapper struct instead of tuple
  • DecodingError::Format is far to unspecific, could use a boxed error interface with better than stringly typing of all publicly inspectable error variants. Encapsulated error #251
  • ChunkType could be a wrapper type, not a raw array. This would allow customized debug and display implementations to properly present the usually ascii nature of its contents. Also the free standing functions of chunk could then be methods. Turn ChunkType into a type #248
  • ColorType::samples should take self by value. Issue 213 color samples #249
  • FrameControl::delay_den could be a proper non-zero type. Secondary.
  • Adam7Iterator::Item should be strictly typed with a struct. (Not exposed, so no SemVer problems).
  • Test interlaced APNG
  • Anything needed to prepare customized handling of chunks?
  • The enum variants of ColorType are partially scream-case. They should not be. Fix SCREAM_CASE on ColorType variants #256
  • The name combination of Encoder and Decoder is non-sensical. See the recent rename in gif. Additionally, we should make sure that all Stream* types have the same prefix.. Not: StreamWriter, StreamingDecoder.
  • Default to no transformations. Default Transformations to Identity #270
  • The filter field in Info refers to the IHDR data, instead of the per-line FilterType (Add Adaptive filtering method for encoding #264)
  • Your issue? Please edit and add.
@yanchith
Copy link

yanchith commented Aug 6, 2021

Hey there! Sad to see you drop #![no_std] from the list. Anything I can do to help? What is the major complication there?

@HeroicKatora
Copy link
Member Author

HeroicKatora commented Aug 17, 2021

The problem with #[no_std] is usage of io::Error. It is difficult to have interface that interact with both streams and are compatible with no-std usage, relying only on in memory buffers. It would be slightly easier had the crate been engineered from the ground up for this but retro-fitting it proves difficult. (I think I've found a reasonably good interface in weezl which I can explain a bit further—in a blog post probably—but it's not applicable to the current style of png). There is hopefully more opportunity in the next major version.

@HeroicKatora
Copy link
Member Author

HeroicKatora commented Aug 17, 2021

To expand on the problematic design (if you want to call it that, it's certainly not problematic in blocking code): All decoding methods get handed a borrowed output buffer. The code within is then written in such a way that it assumes it can keep this borrow until either a) the image has been read b) an error occurs. Both async and no-std subvert this expectation because we return to the caller to fetch addition input data, which gives up our current borrow of the buffer but we're not done reading and filtering lines. (Note: technically we could make everything a coroutine and thus avoid the return for async specifically but this is then very unwieldy for all synchronous usage).

To fix this we need to make the layers between the ergonomic interface and the underlying, sans-io StreamDecoder statemachine aware of potentially exiting and storing some progress information for resumption. Removing this assumption also makes it harder to use the output as a temporary buffer which could otherwise be used to avoid a small number of allocations but I don't think it's used that way right now.

@buzmeg
Copy link

buzmeg commented Aug 21, 2021

I'm thumbs downing the change from RGBA to Rgba as confusing for no good reason other than to possibly make some overly pedantic Rust linters shut up at the cost of confusing a bunch of people.

RGBA is a shortening of RedGreenBlueAlpha and not Redgreenbluealpha (which people would flag). You wouldn't decapitalize it when it's written out, why decapitalize it when you delete the internal letters?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
0.17
In Progress
Development

No branches or pull requests

3 participants