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

Encapsulate the delay ratio #1128

Merged
merged 4 commits into from
Feb 7, 2020
Merged

Conversation

HeroicKatora
Copy link
Member

Two major reasons:

  • This avoids the public dependency on num_ratio.
  • The internal representation can be changed. This already proved
    necessary once (from u16 rational) and in this manner we are free to
    do it again by adding an inner enum representation.

Note that we already have rounding semantics for the meaning of Delay as
gif rounds to the nearest 10ms when writing frames. Any more or less
precise representation can be made to round to the closest u32 ratio.

Two major reasons:

* This avoids the public dependency on `num_ratio`.
* The internal representation can be changed. This already proved
  necessary once (from u16 rational) and in this manner we are free to
  do it again by adding an inner enum representation.

Note that we already have rounding semantics for the meaning of Delay as
gif rounds to the nearest 10ms when writing frames. Any more or less
precise representation can be made to round to the closest u32 ratio.
@HeroicKatora HeroicKatora requested a review from fintelia February 2, 2020 20:44
@HeroicKatora
Copy link
Member Author

HeroicKatora commented Feb 2, 2020

After this PR the only remaining public dependency will be num-traits for which the risk of a breaking change seems moderate due to the widespread use and this issue specifically noting that the API is de facto stable. This is tested for with travis.

@fintelia
Copy link
Contributor

fintelia commented Feb 4, 2020

Could we avoid adding a new type here, and either use a f32 / u32 with an associated base unit or just a Duration? GIF images can only have delays in multiples of 10ms so it won't impact accuracy for any current uses, and we are probably going to want to redesign this area of the API again anyway

@HeroicKatora
Copy link
Member Author

HeroicKatora commented Feb 4, 2020

Good point, an interaction with time::Duration seems trivial and very useful. But the new type can't be easily avoided. The representation varies between image formats, and an exact representation will be impossible with f32. Additionally, I'll argue that a newtype is to more ergonomic choice not only due to its documentation and constructors describing the precise units.

  • It allows us to define associated constants, independent of time::Duration such as FPS_30 etc.
  • We don't need to choose now and can provide both inexact conversion (the current) and fallible, exact conversions.
  • We might be abled to interact with the known image formats by, e.g., providing a gif_delay constructor, although the exact benefits should be discussed sometimes else.

I'll add the rounding, and best effort conversion from time::Duration though as it does seem quite useful regardless.

With the original available as a millisecond ratio this can easily be
done to the accuracy of nanoseconds, which is the finest available.
Given the prior usage, it does not make much sense to provide an exact
conversion. However, it should be clear that precision is not finite and
the representation is not exactly that of a Duration.

The implementation turned out to be fairly involved as the num-fraction
crate does not provide approximation algorithms for fractions. No fear,
we code them ourselves with extensive testing to cover much more than
the required accuracy tests.
@HeroicKatora
Copy link
Member Author

HeroicKatora commented Feb 5, 2020

@fintelia With conversion from time::Duration added it should be much more useful than the previous representations. However, it was quite involved as we have a lot of edge cases to consider when converting the (essentially) fixed point digit representation to u32 ratios. It should be alright though, and all the complexity hidden which only increases its usefulness.

@HeroicKatora
Copy link
Member Author

Note: we can reuse the implementation of closest_bounded_fraction if we want to add a multiplication operation and addition.

@fintelia
Copy link
Contributor

fintelia commented Feb 6, 2020

I'd feel much better about this if we had multiple different image formats that used the animation API, rather than just extrapolating from GIF. Especially since a bunch of the bounded fraction stuff is really only relevant to formats that don't encode frame delays as an integer number of milliseconds. Because of that, I'd lead towards a minimum viable option of just exposing the GIF frame delay, and planning to make a minor breaking change (to add the rest of this proposal) if/when more formats are added

@HeroicKatora
Copy link
Member Author

HeroicKatora commented Feb 6, 2020

Especially since a bunch of the bounded fraction stuff is really only relevant to formats that don't encode frame delays as an integer number of milliseconds.

  • Right, gif only has a integer delay of 10ms.
  • Animated WebP uses a 'u24' integer delay in milliseconds.
  • It is APNG which encodes the delay as a ratio of seconds.
  • WebM has a TimecodeScale that scales the Timecode etc. of all clusters based on nanosecond resolution but according to the documentation it ' SHOULD be set to a default of 1.000.000 nanoseconds'...

That is already a sufficiently complex amount of conventions to warrant an opaque representation in my opinion, although we might as well switch to an inner enum representation right now instead of later. But let's just get the interface correct and not worry about implementation too much.

and planning to make a minor breaking change (to add the rest of this proposal)

A change can't be both minor and breaking, and lib.rs shows the version churn results, many outdated dependencies that are stuck without security and stability updates.

@HeroicKatora
Copy link
Member Author

I'll take it 👍 means that this is fine? Then I'd merge this and prepare release notes for 0.23.

@fintelia
Copy link
Contributor

fintelia commented Feb 7, 2020

Sounds good. If you're confident that the design will work for all those other formats as well then I think we should be fine

@fintelia fintelia merged commit 9dd1c44 into image-rs:next Feb 7, 2020
@HeroicKatora HeroicKatora deleted the hide-num-ratio branch February 7, 2020 04:24
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.

2 participants