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.

Sign up for GitHub

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

Jump to bottom

Write color decision list to ale #1283

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Write color decision list to ale #1283

wants to merge 11 commits into from

Conversation

MichaelPlug
Copy link

Issue #1282

During the adapting from .otio to .ale, I read the color decision list (ASC_SAT, ASC_SOP) from the .otio file and I save it in the .ale file as a column.

In my opinion this changes can be useful to reduce the loss of data during the export of files.

@MichaelPlug MichaelPlug changed the title write color decision list to ale Write color decision list to ale Apr 28, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2022 •
edited
Loading

Codecov Report

Merging #1283 (34b8bb9) into main (ea64d92) will decrease coverage by 0.03%.
The diff coverage is 65.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1283      +/-   ##
==========================================
- Coverage   86.14%   86.11%   -0.04%     
==========================================
  Files         196      196              
  Lines       19810    19839      +29     
  Branches     2314     2322       +8     
==========================================
+ Hits        17065    17084      +19     
- Misses       2181     2187       +6     
- Partials      564      568       +4
Flag Coverage Δ
py-unittests 86.11% <65.51%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
contrib/opentimelineio_contrib/adapters/ale.py 77.15% <65.51%> (-1.67%) ⬇️

Continue to review full report at Codecov.

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

@jminor jminor added the Awaiting Tests Something that looks good, but needs testing to verify label Apr 28, 2022
reinecke
reinecke requested changes Apr 29, 2022
View reviewed changes
Copy link
Collaborator

@reinecke reinecke left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!
I made a few notes in the PR, also we'd like to see unittests that cover the new functionality and make sure it doesn't get broken in the future.
You may also want to check on #1054 - I think it may implement the same functionality.

contrib/opentimelineio_contrib/adapters/ale.py Outdated
@@ -324,7 +324,7 @@ def write_to_string(input_otio, columns=None, fps=None, video_format=None):
columns.append(key)

# Always output these
for c in ["Duration", "End", "Start", "Name", "Source File"]:
for c in ["Duration", "End", "Start", "Name", "Source File", "ASC_SOP", "ASC_SAT"]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we may not want to add ASC_SOP and ASC_SAT here - we only want to include those columns if one of the clips uses them.

contrib/opentimelineio_contrib/adapters/ale.py Outdated Show resolved Hide resolved
@MichaelPlug
Copy link
Author

Thank you for you're help and your suggestions. These two PR are very similar, I miss it, I think that we are a the same point, more or less.
Do you think that this part is ok? The next step is to implement one or more test about this new functionality? In the other PR this wasn't done, right?

@MichaelPlug
Copy link
Author

MichaelPlug commented Apr 29, 2022 •
edited
Loading

Sorry write wrong description of the second-last commit.

  • In the second-last commit:
    I manage possible exception if .otio file is corrupted or ASC_SOP is void.
  • In the last commit:
    I add CDL data (ASC_SOP and ASC_SAT) to sample data and fixed an error in ASC_SOP data ordering. Now the test_ale_adapter.py check if reading ALE and converting again to ALE, also CLD data of the output are consistent.

Let me know if more is needed, thank you.

@MichaelPlug
Copy link
Author

I'm sorry @reinecke , I didn't get what the next steps are to finish the job

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 5, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2023 •
edited
Loading

Codecov Report

Merging #1283 (3757512) into main (eaf8569) will decrease coverage by 0.02%.
The diff coverage is 65.51%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1283      +/-   ##
==========================================
- Coverage   79.91%   79.89%   -0.02%     
==========================================
  Files         197      197              
  Lines       21731    21760      +29     
  Branches     4339     4347       +8     
==========================================
+ Hits        17366    17385      +19     
- Misses       2213     2219       +6     
- Partials     2152     2156       +4
Flag Coverage Δ
py-unittests 79.89% <65.51%> (-0.02%) ⬇️

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

Files Changed Coverage Δ
contrib/opentimelineio_contrib/adapters/ale.py 77.15% <65.51%> (-1.67%) ⬇️

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 eaf8569...3757512. Read the comment docs.

@MichaelPlug
write color decision list to ale
7963864 
Signed-off-by: Michele Spina <[email protected]>
@MichaelPlug
fix to respect lint guidelines
3494585 
Signed-off-by: Michele Spina <[email protected]>
@MichaelPlug
fix to respect lint guidelines
0e044b3 
Signed-off-by: Michele Spina <[email protected]>
@MichaelPlug
if metadata ASC_SOP/ASC_SAT is not in .otio search it in ALE as origi…
83562a1 
…nally

Signed-off-by: Michele Spina <[email protected]>
@MichaelPlug
fix typo
fc4b421 
Signed-off-by: Michele Spina <[email protected]>
@MichaelPlug
add ASC_SOP and ASC_SAT columns only if necessary
416128f 
Signed-off-by: Michele Spina <[email protected]>
@MichaelPlug
lint style
9e4f3bb 
Signed-off-by: Michele Spina <[email protected]>
@MichaelPlug
reset lib using nil
306e282 
Signed-off-by: Michele Spina <[email protected]>
@MichaelPlug
fix ASC_SOP order: slope, offset, power and update sample to check cd…
1e4676b 
…l import/export

Signed-off-by: Michele Spina <[email protected]>
@MichaelPlug
fix sample data (deleted extra column)
04f642d 
Signed-off-by: Michele Spina <[email protected]>
@MichaelPlug MichaelPlug closed this Aug 5, 2023
@MichaelPlug MichaelPlug reopened this Aug 5, 2023
@reinecke reinecke added the Adapters Relating to the adapters that live in the separate repos label Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reinecke reinecke Awaiting requested review from reinecke

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Adapters Relating to the adapters that live in the separate repos Awaiting Tests Something that looks good, but needs testing to verify
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MichaelPlug @codecov-commenter @reinecke @jminor