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

add rendering notes #1679

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Conversation

meshula
Copy link
Collaborator

@meshula meshula commented Nov 16, 2023

This PR adds notes on rendering since rendering behavior is unclear from the specification itself.

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2023

Codecov Report

Merging #1679 (86b0074) into main (cbef407) will decrease coverage by 6.16%.
Report is 81 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1679      +/-   ##
==========================================
- Coverage   85.99%   79.84%   -6.16%     
==========================================
  Files         200      197       -3     
  Lines       20934    21792     +858     
  Branches     2459     4358    +1899     
==========================================
- Hits        18002    17399     -603     
+ Misses       2331     2232      -99     
- Partials      601     2161    +1560     
Flag Coverage Δ
py-unittests 79.84% <ø> (-6.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 132 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbef407...86b0074. Read the comment docs.

@darbyjohnston
Copy link
Contributor

What would also be really useful is the equivalent of the "project settings" in Final Cut Pro for establishing the output resolution, audio format, etc:
https://support.apple.com/guide/final-cut-pro/final-cut-pro-project-settings-ver1b946a4ff/10.7/mac/13.5

Currently in tlRender I make a guess at the settings by using the video and audio from the first clip in the timeline. This works mostly OK but fails in some situations and also requires opening the media.

Properties_01
Properties_04

@reinecke
Copy link
Collaborator

reinecke commented Dec 7, 2023

I spun the project settings suggestion @darbyjohnston made into #1683 - I know it'd been talked about before but we'd failed to capture it as an issue. Thanks for highlighting it again!

@meshula
Copy link
Collaborator Author

meshula commented Dec 7, 2023

@reinecke @darbyjohnston Does my edit on compositing ring true?

@darbyjohnston
Copy link
Contributor

Looks good to me, thanks. For transparent clips that allow the background to show through, we assume the background color is black?

@meshula
Copy link
Collaborator Author

meshula commented Dec 8, 2023

@darbyjohnston yes, I added a bit about zero values, and 100% values for alpha. Note that I specified zero, not black, because if we say "black" then it invites discussion about black levels and what we expect to happen for non-zero blacks. I also said 100% so that no one wonders if we mean "1", "1.0", or "255" or ...

@darbyjohnston
Copy link
Contributor

Thanks, I think the note about "zero" is good so we don't get into color issues as you mentioned. Why the 100% alpha? For viewers it is nice to preserve the alpha channels for checking renders, though that could also be achieved by viewing the individual elements instead of the final composite.

@reinecke
Copy link
Collaborator

reinecke commented Dec 8, 2023

This update looks really good to me. On the background color, I think 100% opaque zero color values is a good baseline to match most NLEs. That said, if an application wants to allow people to override that behavior, there’s nothing stopping them.

@meshula
Copy link
Collaborator Author

meshula commented Dec 12, 2023

If someone wants to go ahead and approve, we can merge.

@darbyjohnston
Copy link
Contributor

It looks like the DCO check is failing...

@meshula
Copy link
Collaborator Author

meshula commented Dec 13, 2023

right, if we accept changes in the online form, as we I did, the dco is not attached. heavy sigh.

@meshula
Copy link
Collaborator Author

meshula commented Dec 13, 2023

Does anyone know a trick for repairing that? If not, I'm inclined to just merge, as there are not enough hours in the day to recreate a PR and go through the whole rigamarole again

@reinecke reinecke merged commit d98f657 into AcademySoftwareFoundation:main Dec 13, 2023
39 checks passed
@reinecke
Copy link
Collaborator

I forced DCO pass and merged - thanks for updating the docs @meshula

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.

4 participants