Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Use duration from streams as its more precise #6171

Merged
merged 5 commits into from
Mar 6, 2024

Conversation

tokejepsen
Copy link
Member

@tokejepsen tokejepsen commented Jan 26, 2024

Changelog Description

When dealing with 30 fps mov of 2 frames, the duration was reduce to 3 decimal places (0.067) which meant that the flag for ffmpeg -ss ended up with a time that was not precise enough for ffmpeg to pick a frame; 0.0335. Should be 0.0333.

Using the duration from the streams is more precise; 0.066667.

Testing notes:

  1. Publish render of 2 frames with 30 fps.
  2. Validate a thumbnail is created.

@ynbot
Copy link
Contributor

ynbot commented Jan 26, 2024

@ynbot ynbot added type: bug Something isn't working size/XS Denotes a PR changes 0-99 lines, ignoring general files labels Jan 26, 2024
Copy link
Collaborator

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

I didn't test the code - so not sure if there are any annoying side effects to this.

It might be good to also add a comment above this code that states something like:

# Use duration of the individual streams since it is returned with
# higher decimal precision than 'format.duration'. We need this
# more precise value for calculating the correct amount of frames
# for higher FPS ranges or decimal ranges, e.g. 29.97 FPS 

Just so someone doesn't come along in a year optimizing it back to what it was now.

It'd be extra nice if we can have a "test" implemented for this particular bug so that we can ensure the problem remains resolved over time. It might be quite some work to do so though, but it would be the best case scenario to implement with PRs like these. Unfortunately implementing tests currently is still quite a hefty and involved process than just a few lines of code.

@tokejepsen
Copy link
Member Author

Just so someone doesn't come along in a year optimizing it back to what it was now.

Yeah, will do.

Unfortunately implementing tests currently is still quite a hefty and involved process than just a few lines of code.

Yea, the testing framework needs to be more dev friendly.

@mkolar mkolar added the sponsored Client endorsed or requested label Feb 9, 2024
@mkolar mkolar changed the title OP-8005 - Use duration from streams as its more precise Use duration from streams as its more precise Feb 9, 2024
Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

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

LGTM!

@jakubjezek001 jakubjezek001 merged commit 119320b into develop Mar 6, 2024
3 checks passed
@ynbot ynbot added this to the next-patch milestone Mar 6, 2024
@tokejepsen tokejepsen deleted the bugfix/OP-8005_thumbnail_duration branch March 7, 2024 07:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
port to AYON size/XS Denotes a PR changes 0-99 lines, ignoring general files sponsored Client endorsed or requested target: AYON target: OpenPype type: bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants