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

RationalTime value mismatch when serialize/deserialize to JSON #1822

Open
3 of 5 tasks
robin-ynput opened this issue Jan 15, 2025 · 1 comment
Open
3 of 5 tasks

RationalTime value mismatch when serialize/deserialize to JSON #1822

robin-ynput opened this issue Jan 15, 2025 · 1 comment
Labels
bug A problem, flaw, or broken functionality.

Comments

@robin-ynput
Copy link

Required:


  • I believe this isn't a duplicate topic
  • This report is not related to an adapter

For adapter related issues, please go to the appropriate repository - likely in
the OpenTimelineIO github organization.
For general questions and help please use the
Academy Software Foundation slack,
#opentimelineio.

Select One:

  • Build problem
  • Incorrect Functionality or bug
  • New feature or functionality

Description

I'm having RationalTime accuracy issue when serializing/deserializing a Clip as json.
It's very small but enough to produce value comparison mismatch on my end.

Is that expected ? Has anyone spotted that before ?
Is there something else I do not understand ?

(tested with opentimelineio-0.17.0 )

Reproduction Steps

import opentimelineio as otio

rate_23_976 = 23.976024627685547

# Create a random clip
start =  otio.opentime.RationalTime(30, 24.0)
src_range = otio.opentime.TimeRange(
  start.rescaled_to(rate_23_976),
  otio.opentime.RationalTime(0, rate_23_976)
)
clip = otio.schema.Clip(name="test", source_range=src_range)

# Recreate same clip through serialization/deserialization to JSON
clip2 = otio.schema.Clip.from_json_string(clip.to_json_string())

# Comparison
assert clip2.source_range.start_time == clip.source_range.start_time  # I'm expecting this to be True but raises
assert clip.source_range.start_time.rate == clip2.source_range.start_time.rate  # OK
assert clip.source_range.start_time.value == clip2.source_range.start_time.value # raises
@meshula
Copy link
Collaborator

meshula commented Jan 16, 2025

Notes on rapidjson for whoever takes this issue on; from the rapidjson documentation it seems round trip precision is not guaranteed given the "maximum 3 [UL{] error note. Have we looked at round tripping previously, or do we even already have unit tests for it?

## Parsing to Double {#ParsingDouble}

Parsing string into `double` is difficult. The standard library function `strtod()` can do the job but it is slow. By default, the parsers use normal precision setting. This has has maximum 3 [ULP](http://en.wikipedia.org/wiki/Unit_in_the_last_place) error and implemented in `internal::StrtodNormalPrecision()`.

When using `kParseFullPrecisionFlag`, the parsers calls `internal::StrtodFullPrecision()` instead, and this function actually implemented 3 versions of conversion methods.
1. [Fast-Path](http://www.exploringbinary.com/fast-path-decimal-to-floating-point-conversion/).
2. Custom DIY-FP implementation as in [double-conversion](https://github.com/floitsch/double-conversion).
3. Big Integer Method as in (Clinger, William D. How to read floating point numbers accurately. Vol. 25. No. 6. ACM, 1990).

If the first conversion methods fail, it will try the second, and so on.
## Double-to-String conversion {#dtoa}

Originally RapidJSON uses `snprintf(..., ..., "%g")`  to achieve double-to-string conversion. This is not accurate as the default precision is 6. Later we also find that this is slow and there is an alternative.

Google's V8 [double-conversion](https://github.com/floitsch/double-conversion
) implemented a newer, fast algorithm called Grisu3 (Loitsch, Florian. "Printing floating-point numbers quickly and accurately with integers." ACM Sigplan Notices 45.6 (2010): 233-243.).

However, since it is not header-only so that we implemented a header-only version of Grisu2. This algorithm guarantees that the result is always accurate. And in most of cases it produces the shortest (optimal) string representation.

The header-only conversion function has been evaluated in [dtoa-benchmark](https://github.com/miloyip/dtoa-benchmark).

@meshula meshula added the bug A problem, flaw, or broken functionality. label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A problem, flaw, or broken functionality.
Projects
None yet
Development

No branches or pull requests

2 participants