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

New public API pub fn expand_interlaced_row #495

Merged

Conversation

anforowicz
Copy link
Contributor

Currently users of Reader.next_interlaced_row can't handle interlaced rows without re-implementing significant chunks of the png crate. This PR resolves this by exposing a new public API: pub fn expand_interlaced_row + pub enum InterlaceInfo.

@anforowicz
Copy link
Contributor Author

Example code that depends on the new API can be seen in http://review.skia.org/894576

src/adam7.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/adam7.rs Outdated Show resolved Hide resolved
@anforowicz anforowicz force-pushed the public-expand-interlaced-row-api branch from 40bf4e3 to f6eadd4 Compare August 30, 2024 20:01
This helps with the following things:

* It means that after making `InterlaceInfo` public in a follow-up
  commit, `pass`, `line`, and `width` fields can remain private.
* It means that a follow-up commit that refactors `adam7::expand_pass`
  to take a single `info` parameter doesn't allow passing an
  invalid `InterlaceInfo::Null` value.
* It gives `Adam7Iterator` a nicer, named `Iterator::Item`.
Instead of passing `line_no` and `pass` as separate parameters, we
can pass a single `&Adam7Info` parameter.  This minor refactoring
helps with:

* Exposing a nice public API in a follow-up commit
* Depending on `Adam7Info.width` in a follow-up commit
Instead of passing `line_no` and `pass` as separate parameters, we
can pass a single `&Adam7Info` parameter.  This minor refactoring
helps with:

* Cleaning up `fn expand_pass` by moving some of its complexity closer
  to where its needed (within `fn expand_adam7_bits`)
* Preparing for changing the semantics of the `width` parameter of
  `fn expand_adam7_bits` in a follow-up commit, where we will stop
  depending on `width` for calculating the length of the returned
  iterator (depending on `Adam7Info.width` instead).  See the follow-up
  commit for more details.
This commit refactors implementation of `expand_adam7_bits` so that it
can accept `row_stride_in_bytes` that is different from the expanded
width of the frame or image.

This is helpful for making `expand_pass` work in scenarios where
interlaced row needs to expanded into a bigger image (e.g. if the
currently decoded frame is an animation frame that only takes a
subregion of the whole image - in this case the stride between expanded
rows is bigger than the size of expanded rows).

And the above is helpful for exposing `expand_pass` through a public API
that will hopefully pass the test of time...
This also exposes supporting items through the public API: `Adam7Info`,
`InterlaceInfo`, `InterlacedRow`.
@anforowicz anforowicz force-pushed the public-expand-interlaced-row-api branch from f6eadd4 to ac04cd3 Compare August 30, 2024 20:02
Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's run with this.

@HeroicKatora HeroicKatora merged commit 7dae687 into image-rs:master Sep 9, 2024
19 checks passed
@anforowicz anforowicz deleted the public-expand-interlaced-row-api branch September 9, 2024 18:21
@anforowicz
Copy link
Contributor Author

Can you please hold off with publishing a new version of the png crate until we finish iterating on my other PR at #496?

This PR adds InterlaceInfo as a public API (see the changes in lib.rs). I've only now realized that in the other PR I need to change the shape of the InterlaceInfo::Null enum variant (to include a public, but totally opaque struct - e.g. NullInfo). If we include both PRs in the next release, then there won't be any breaking change (only new public APIs), but if we split both PRs across two separate releases then the 2nd PR will technically be a breaking change.

I'll try to push new commits/changes to the other PR today or tomorrow.

hubot pushed a commit to google/skia that referenced this pull request Sep 12, 2024
This is a clean-up / follow-up for http://review.skia.org/889877 where
we implemented `onIncrementalDecode` but only for non-interlaced images.

Note that this CL depends on new public APIs in the `png` crate (ones
that are not yet present in `png-0.17.13`).  Therefore this CL depends
on having a post-0.17.3 crate version when building `FFI.rs`:

* In the short-term this dependency is fulfilled by patching the crate
  in https://crrev.com/c/5825297
* In the long-term this dependency will be met by a new release of the
  `png` crate (the PR at image-rs/image-png#495
  hasn't been released yet).

Bug: chromium:356923435
Change-Id: I2787c2f5a9b256720e779ea0acc8737c87cee2c3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/894576
Commit-Queue: Łukasz Anforowicz <lukasza@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
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