Remove Reader::scrach_buffer field + resulting breaking API changes.

[anforowicz]

PTAL?

This mostly follows what we've discussed in #417.

If we merge this, then some coordination may be required for releasing a new version of the png crate and then updating the image crate:

• I am happy to volunteer to update the image crate after a new version of the png crate is released. OTOH, I am not sure how to get notified when this happens?
• I am not sure when exactly it would be desirable to release a new version of the png crate. It may be worth waiting until some additional changes get merged:
  • additional performance-related improvements (e.g. PRs that stem from the "copy avoidance series of commits")
  • additional performance-related breaking-changes (e.g. I think that Read => BufRead is on the table - I want to try measuring this after flushing out other changes from the "copy avoidance series of commits")

[anforowicz]

Hmmm... after having already submitted the PR, I now started to wonder:

• Maybe the remaining API should just be called next_row (rather than next_interlaced_row)

• Maybe I should simplify the description in CHANGES.md to something like:

    ```
    * Breaking API changes:
       - Removing the `Row` and `InterlacedRow` structs
       - Removing the `Reader::next_interlaced_row` method
       - Changing the signature of the `Reader::next_row` method
    ```
  

[fintelia]

I would like to eventually make this API change, but for now I wonder if it would be simpler to make next_frame call self.next_pass() and self.next_interlaced_row_impl() directly. The scratch buffer variable would still exist, but it would only be allocated/used if the user specifically requested rows one-by-one.

[anforowicz]

One motivation for the changes in this PR is to improve the performance of image::codecs::png::PngReader which calls into next_row. Changes to next_frame wouldn't help with that.

It seems that sometimes decoding row-by-row (or at least smaller-than-frame-chunk-by-chunk) may be desirable if pixels needs some kind of post-processing (e.g. applying gamma, alpha premultiplication, etc.) and the user of the png crate wants to do such post-processing while the image data is in L1 cache. image::codecs::png::PngReader is compatible with that idea, but next_frame would add a considerably larger L1 cache pressure. ("seems" because I don't have hard data and/or benchmarks to confirm this. But it does seem like a valid idea. I think.)

[anforowicz]

Is the main concern the desire to avoid breaking changes?

One way to address this concern may be to instead add a new API (next_row2?) and the #[deprecated] attribute to the old APIs (structs and methods).

OTOH, IMO some breaking API changes may be required for performance one way or another. So maybe another way to proceed is to wait until those other breaking changes have PRs with measurements that show their impact for the noncompressed benches and then land all the breaking changes around the same time. (For example, I plan to send the Read => BufRead changes after first flushing out 2 less controversial PRs - so, in a week or two maybe, depending on how the other PRs are received :-))

[fintelia]

The goal is to bundle all the breaking changes we'd like to make into a single 0.18 release, rather than have a couple breaking releases in rapid succession. But that means that breaking changes will take longer to land than non-breaking changes. So if there are pieces that can be split out, it likely makes sense to merge those first.

These changes generally look good to me though, so I'm fine with letting them linger until we're ready to do a breaking release

[anforowicz]

The goal is to bundle all the breaking changes we'd like to make into a single 0.18 release, rather than have a couple breaking releases in rapid succession.
...
These changes generally look good to me though, so I'm fine with letting them linger until we're ready to do a breaking release

Ack. That sounds good and is totally reasonable.

Just as an FYI, let me point out that currently I think that I may want to make the following 3 breaking changes:

• This PR (Reader::next_row taking &mut [u8] instead of returning &[u8])
• Requiring that the reader passed to the png crate is already BufRead and not just Read (this is in the spirit of copy avoidance but wasn't explicitly covered in my recent measurements). Like this PR, that other PR will probably also require follow-up changes in the image crate.
• Changing ZlibStream::decompress (and ZlibStream::finish_compressed_chunks) so it takes &mut [u8] instead of taking and appending to &mut Vec<u8>. This has been a part of the copy-avoidance-series-of-commits and has been covered by my recent measurements. This is technically a breaking API change, but this is a fairly low-level API that I think/hope shouldn't see that much usage (or at least should see less usage than the other changes above).

I think that it's probably desirable to first flush out 3-4 other PRs with non-breaking changes (starting with the noncompressed benchmarks). Flushing those out will help to highlight/magnify the relative improvement from the breaking changes (at least from the last 2; the current PR isn't directly covered by the benchmarks in the png crate and I am hesitant to expand the scope/shape of the benchmarks just to justify the PR).

[anforowicz]

Status update:

• I've made the small tweaks that I've listed in Remove Reader::scrach_buffer field + resulting breaking API changes. #421 (comment)
• I've put together a PR for another breaking API change (BufRead): Performance: Require BufRead instead of just Read for inputs. #427
• I am no longer sure if it is desirable to pursue "changing ZlibStream::decompress [...] so it takes &mut [u8] instead of [...] &mut Vec<u8>." (mentioned in Remove Reader::scrach_buffer field + resulting breaking API changes. #421 (comment) above). In particular, Reduce copying and allocations #422 (comment) shows that avoiding copying of prev can be achieved without this breaking API change. There may be additional desirable tweaks of ZlibStream, but hopefully they won't require a breaking API change.