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

Burnin adapter (second attempt) #167

Merged
merged 7 commits into from Oct 5, 2017
Merged

Conversation

@repsac
Copy link
Collaborator

repsac commented Oct 5, 2017

Removed enums.
Constants are now global (as opposed to a class namespace).
Updating the MIT license with preamble.

@repsac
Copy link
Collaborator Author

repsac commented Oct 5, 2017

Test server doesn't have PIL. That is going to make this difficult.

ImportError: No module named PIL

@@ -0,0 +1,391 @@
# OpenTimelineIO bundles ffmpeg_burnins, which is available under the MIT
# License. For details contrib/adapters.

This comment has been minimized.

@jminor

jminor Oct 5, 2017 Collaborator

Can you move the text "OpenTimelineIO bundles ffmpeg_burnins, which is available under the MIT License. For details contrib/adapters." to the bottom of the top-level LICENSE.txt file here:
https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/master/LICENSE.txt

This comment has been minimized.

@jminor

jminor Oct 5, 2017 Collaborator

and mention the exact filename of the ffmpeg_burnins.py file, to clarify which file(s) the MIT license applies to.

This comment has been minimized.

@jminor

jminor Oct 5, 2017 Collaborator

The PIL issue can be resolved by editing the .travis.yml file. Look for pip install and add PIL to the list.

This comment has been minimized.

@repsac

repsac Oct 5, 2017 Author Collaborator

I may have found a smarter way to do this. All examples of ffmpeg I looked at before were using drawbox, but just now found one that doesn't use it. Doing some tests now (if this works it will make the code cleaner too)

This comment has been minimized.

@repsac

repsac Oct 5, 2017 Author Collaborator

You can make fun of me later for missing this (so much easier)

ffmpeg -loglevel panic -i BUSH0005.MOV -vf "drawtext=text='Bottom Right+50':x=1437.5:y=h-text_h-5:fontcolor=white@1.0:fontsize=48:fontfile=/System/Library/Fonts/Menlo.ttc:box=1:boxborderw=5:boxcolor=black" test.mov

This comment has been minimized.

@jminor

jminor Oct 5, 2017 Collaborator

Nice!

This comment has been minimized.

@repsac

repsac Oct 5, 2017 Author Collaborator

From a syntax perspective this is cleaner, however PIL getting the dimensions of the text is really useful and kind of needed. If you're using either of the right alignment parameters it helps to know how to offset the text from the right side (offsets appear to be calculated from the left side of the text).

I think PIL will be needed, sorry for the complication.

This comment has been minimized.

@repsac

repsac Oct 5, 2017 Author Collaborator

The alignment parameters are a convenience so that people don't have to bang their head against a keyboard getting the right x value (which can change on resolution). If you you the right alignments and give it an offset the tool can figure out what the actual X position of the text is.

repsac added 2 commits Oct 5, 2017
Moved bundling comment to LICENSE.txt
Improved box drawing for background
Added second unittest for burins without backgrounds
Adding Pillow (PIP) to travis' pip install
@repsac
Copy link
Collaborator Author

repsac commented Oct 5, 2017

The system font thing is going to be a pain. On my mac it uses Menlo, and even if I could figure out the system font to use on travis it is going to throw the box dimensions (for the x position) off when calculating the right alignments.

Yeah at this point I am not sure how to get this through.

@jminor
Copy link
Collaborator

jminor commented Oct 5, 2017

You can either just drop the test (honestly that would be fine),
or maybe bundle a specific font so it is consistent?
Something like this: https://fonts.google.com/specimen/Open+Sans maybe?

…ing RIGHT alignments

this should get around the test case issues
@repsac
Copy link
Collaborator Author

repsac commented Oct 5, 2017

I have an update that only parses the system font when using 'right' alignments. As long as we don't use those in the tests this may work. Still poking at it.

adjusting the ffmpeg_burnins lib to not parse a system font
@jminor
jminor approved these changes Oct 5, 2017
@jminor
Copy link
Collaborator

jminor commented Oct 5, 2017

Ed, this looks great, the tests all pass now, and we have your CLA on file. Is there anything else you want to change before we accept it?

@repsac
Copy link
Collaborator Author

repsac commented Oct 5, 2017

Nope, that is all I can think of for now.

@ssteinbach ssteinbach added this to the Public Beta 6 milestone Oct 5, 2017
@jminor jminor merged commit 9e7eeda into PixarAnimationStudios:master Oct 5, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jminor
Copy link
Collaborator

jminor commented Oct 5, 2017

Accepted. Thanks Ed! This will be super handy.

@repsac
Copy link
Collaborator Author

repsac commented Oct 5, 2017

Awesome. Will be interesting to see how it appeals to users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.