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

Additional video options #420

Merged
merged 3 commits into from Feb 8, 2021
Merged

Additional video options #420

merged 3 commits into from Feb 8, 2021

Conversation

ksfeldman
Copy link
Contributor

Adding in the ability to do 2-pass encoding
Allowing user to disable video resizing
Allowing user to force a video encode even within the same codec

Adding in the ability to do 2-pass encoding
Allowing user to disable video resizing
Allowing user to force a video encode even within the same codec
@codecov
Copy link

codecov bot commented Jan 30, 2021

Codecov Report

Merging #420 (1e65fc5) into master (79789e3) will increase coverage by 0.14%.
The diff coverage is 97.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
+ Coverage   86.49%   86.63%   +0.14%     
==========================================
  Files          22       22              
  Lines        1799     1818      +19     
==========================================
+ Hits         1556     1575      +19     
  Misses        243      243              
Impacted Files Coverage Δ
sigal/settings.py 97.87% <ø> (ø)
sigal/video.py 90.75% <97.36%> (+1.75%) ⬆️

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 79789e3...1e65fc5. Read the comment docs.


# Set this to false if no resizing is desired on the video. This overrides
# the video_size option.
# video_resize = True
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe allow video_size = None to disable resizing, which would remove one configuration option ?

sigal/video.py Outdated
logger.debug('Video size: %i, %i -> %i, %i', w_src, h_src, w_dst, h_dst)
return {'src': (w_src, h_src), 'dst': (w_dst, h_dst)}
Copy link
Owner

Choose a reason for hiding this comment

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

Why not putting this directly in get_resize_options to avoid the dict packing/unpacking ? I think it's the only place where it is used.

final_second_pass_options = _get_empty_if_none_else_variable(
second_pass_options) + resize_opt
generate_video_pass(converter, source,
final_second_pass_options, outname)
Copy link
Owner

Choose a reason for hiding this comment

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

So when doing 2-pass encoding the idea is that ffmpeg will create a temp file in the first pass and find it when doing the second pass right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct, yes. I don't believe there is a way to change the filename from the default "ffmpeg2pass-0.log"

Using video_size = None to prevent resizing instead of another option
Eliminating pointless method for just getting resize_dimensions
@ksfeldman
Copy link
Contributor Author

Thanks for the recommendations, both have been pushed. Would you like me to squash this commit as well?

@saimn
Copy link
Owner

saimn commented Feb 7, 2021

Oups sorry I forgot to merge your PR before mine (#421), so now you get some conflicts :(
It should be easy to fix, just 2 lines in process_video, but I can do it if you prefer.
Also thanks for the change, looks good.

@ksfeldman
Copy link
Contributor Author

No problem, conflict should be resolved. Merge whenever! Thanks

@saimn saimn merged commit b265929 into saimn:master Feb 8, 2021
@saimn
Copy link
Owner

saimn commented Feb 8, 2021

Thanks!

@ksfeldman ksfeldman deleted the additional_video_options branch February 8, 2021 03:47
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.

None yet

2 participants