openzim / python-scraperlib

Collection of Python code to re-use across Python-based scrapers
GNU General Public License v3.0
19 stars 16 forks source link

Video module #17

Closed satyamtg closed 4 years ago

satyamtg commented 4 years ago

As discussed in #16 , this implements the newer API for fixing #14

Some small changes are needed. I've added a 2 presets one for video and other for audio. Also, recompress is renamed to reencode as it sounded better to me. For the test videos, they are now clips from "Tears of Steel" which is an open video from the Blender foundation.

codecov[bot] commented 4 years ago

Codecov Report

Merging #17 into master will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master       #17    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            9        13     +4     
  Lines          259       387   +128     
==========================================
+ Hits           259       387   +128     
Impacted Files Coverage Δ
src/zimscraperlib/video/config.py 100.00% <100.00%> (ø)
src/zimscraperlib/video/encoding.py 100.00% <100.00%> (ø)
src/zimscraperlib/video/presets.py 100.00% <100.00%> (ø)
src/zimscraperlib/video/probing.py 100.00% <100.00%> (ø)

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 7e2bd20...6ee6d1d. Read the comment docs.

satyamtg commented 4 years ago
  • video.video module makes no sense. Use something like
video/
    presets.py
    encoding.py [reencode]
    probing.py [get_media_info]
    config.py [Config, ConfigBuilder]

That's why I imported video in __init__. However, what you suggested seems much cooler. Did that. Also created the Config class as you suggested and did other suggested changes.

In this example I chose remove the default values. My point being, there's too much options and setting them all might make it difficult to use. If I want a webm video with it, I need to remove the extra_flags so it's extra unexpected steps to make it work.

I think having a blank state with clearly documented values might be better. Those wanting a one-stop solution can use the presets I think.

With regards to this I am also in favour of having no defaults at all. (except max_muxing_queue_size)

Also, I added a list called extras which would be appended at plant and would give flexibility in adding flags. Used it in VoiceMp3Low preset for passing -vn. threads is now moved to Config. Also added a handy update_config() method to update using those shortcut params as used by build_from(). Tests are refactored.

Regarding capturing ffmpeg output, I've added that too. Strangely, ffmpeg throws all output in stderr, so there's an option to return it. Also, instead of failing on ffmpeg failure, we return the return code from the ffmpeg run in reencode().

I think this one is quite generalized and easier to use. @rgaudin please let me know your thoughts.