Add VIDEO_FORMAT header when writing to string/file from ALE adpater #462

mikekoetter · 2019-03-20T00:00:13Z

So, the Media Composer seems to be a bit picky about ALE headers and it will complain if we don't set a VIDEO_FORMAT. This MR looks to do that.

The first thing it does is add a `video_format` kwarg to the adapter signature, allowing you to set the format on output.

If we don't set that variable it will try to preserve the existing header column (if set).
This would likely only be set if the time-line was read in from a previous ALE

When the header column is not found, it searches each clips ALE metadata for an "Image Size" setting to infer a project format. 

If none of that is found it defaults to the adapters DEFAULT_VIDEO_FORMAT (1080)

1080p is probably the most popular format being used right now, but I am open to suggestions if anyone has other thoughts on default settings

Adds a video_format kwarg to the adapter signature, allowing you to set the format on output. The fallback is to preserve the existing header column, if set. Finally it searches each clips ALE metadata for an "Image Size" setting to infer a project format. If none of that is found it defaults to the adapters DEFAULT_VIDEO_FORMAT (1080)

codecov-io · 2019-03-20T00:02:09Z

Codecov Report

Merging #462 into master will increase coverage by 0.77%.
The diff coverage is 96.22%.

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
+ Coverage   87.21%   87.99%   +0.77%     
==========================================
  Files          67       68       +1     
  Lines        6819     6878      +59     
==========================================
+ Hits         5947     6052     +105     
+ Misses        872      826      -46

Impacted Files	Coverage Δ
...elineio_contrib/adapters/tests/test_ale_adapter.py	`98.33% <100%> (+0.96%)`	⬆️
opentimelineio_contrib/adapters/ale.py	`87.86% <93.54%> (+4.52%)`	⬆️
opentimelineio_contrib/adapters/extern_rv.py	`0% <0%> (ø)`	⬆️
opentimelineio/console/console_utils.py	`100% <0%> (ø)`
...elineio_contrib/adapters/aaf_adapter/aaf_writer.py	`93.62% <0%> (+0.02%)`	⬆️
...elineio_contrib/adapters/tests/test_aaf_adapter.py	`98.1% <0%> (+0.02%)`	⬆️
...neio_contrib/adapters/advanced_authoring_format.py	`90.62% <0%> (+0.04%)`	⬆️
opentimelineio/console/otiocat.py	`96.29% <0%> (+12.96%)`	⬆️
opentimelineio/console/otioconvert.py	`93.42% <0%> (+40.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 44a06b0...e8488d5. Read the comment docs.

ssteinbach

Looks good, just had a couple small tweaks. Thanks!

opentimelineio_contrib/adapters/ale.py

ssteinbach · 2019-03-25T17:59:42Z

opentimelineio_contrib/adapters/ale.py

@@ -109,6 +111,20 @@ def _parse_data_line(line, columns, fps):
        ))


+def get_video_format(width, height):


Might be a good idea to bump this map up to the top level and/or add a comment about how to use this.

I'm not sure what 'top level' means in an adapter file, but I added a brief comment and made it "private"

Move metadata search and the video_format mapping into private methods outside of `write_to_string`

ssteinbach · 2019-03-27T00:43:36Z

opentimelineio_contrib/adapters/ale.py

+def _video_format(width, height):
+    """Utility function to map a width and hight to an Avid Project Format"""
+
+    format_map = {


I meant that this map could be moved outside the function as a module level constant -- AVID_PROJECT_HEIGHT_TO_WIDTH_MAP = {...} or something. We're trying to call out these kinds of constant definitions to make them easier to find in the future.

ok, I think I understand the request, however I don't think that dict would be very useful unless you are applying the width check for resolutions wider than 1920, which avid (currently) considers CUSTOM

I can move the _video_format(...) function to the top of the file and make give that a "public" syntax if you think that would be helpful

@ssteinbach - I moved and renamed it. please let me know if that is more what you are thinking

Yup, looks great. Thanks!

rename _video_format() to AVID_PROJECT_FORMAT_FROM_WIDTH_HEIGHT()

Rename function to AVID_VIDEO_FORMAT_ from AVID_PROJECT_FORMAT_ to be more accurate to what is being mapped

ssteinbach

Looks great, thanks.

ssteinbach requested changes Mar 25, 2019

View reviewed changes

style(ale_adapter): move format searche into private methods

f2ddcad

Move metadata search and the video_format mapping into private methods outside of `write_to_string`

ssteinbach reviewed Mar 27, 2019

View reviewed changes

mikekoetter added 2 commits March 27, 2019 12:13

style(ale_adapter): rename and make_video_format() more accesible

e8488d5

rename _video_format() to AVID_PROJECT_FORMAT_FROM_WIDTH_HEIGHT()

style(ale_adapter): Rename to AVID_VIDEO_FORMAT

d0845d7

Rename function to AVID_VIDEO_FORMAT_ from AVID_PROJECT_FORMAT_ to be more accurate to what is being mapped

ssteinbach approved these changes Mar 27, 2019

View reviewed changes

ssteinbach merged commit af1a817 into AcademySoftwareFoundation:master Mar 27, 2019

ssteinbach added this to the Public Beta 10 milestone Mar 27, 2019

andrewmoore-nz mentioned this pull request Apr 8, 2019

AAF Reader: File can no longer be parsed after #460 #484

Closed

andrewmoore-nz mentioned this pull request Jun 12, 2019

AAF Reader: Incorrect Clip Source Times #523

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VIDEO_FORMAT header when writing to string/file from ALE adpater #462

Add VIDEO_FORMAT header when writing to string/file from ALE adpater #462

mikekoetter commented Mar 20, 2019

codecov-io commented Mar 20, 2019 •

edited

ssteinbach left a comment

ssteinbach Mar 25, 2019

mikekoetter Mar 26, 2019

ssteinbach Mar 27, 2019

mikekoetter Mar 27, 2019

mikekoetter Mar 27, 2019

ssteinbach Mar 27, 2019

ssteinbach left a comment

		@@ -109,6 +111,20 @@ def _parse_data_line(line, columns, fps):
		))


		def get_video_format(width, height):

Add VIDEO_FORMAT header when writing to string/file from ALE adpater #462

Add VIDEO_FORMAT header when writing to string/file from ALE adpater #462

Conversation

mikekoetter commented Mar 20, 2019

codecov-io commented Mar 20, 2019 • edited

Codecov Report

ssteinbach left a comment

Choose a reason for hiding this comment

ssteinbach Mar 25, 2019

Choose a reason for hiding this comment

mikekoetter Mar 26, 2019

Choose a reason for hiding this comment

ssteinbach Mar 27, 2019

Choose a reason for hiding this comment

mikekoetter Mar 27, 2019

Choose a reason for hiding this comment

mikekoetter Mar 27, 2019

Choose a reason for hiding this comment

ssteinbach Mar 27, 2019

Choose a reason for hiding this comment

ssteinbach left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 20, 2019 •

edited