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

Use duration from streams as its more precise#6171

Merged
jakubjezek001 merged 5 commits intodevelopfrom
bugfix/OP-8005_thumbnail_duration
Mar 6, 2024
Merged

Use duration from streams as its more precise#6171
jakubjezek001 merged 5 commits intodevelopfrom
bugfix/OP-8005_thumbnail_duration

Conversation

@tokejepsen
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
@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