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

Add MP4 engine to support producing webp/gif directly from mp4 #899

Open
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@winguse
Copy link

winguse commented Mar 23, 2017

In the PR, I implemented a new engine for producing webp / gif from mp4. In my case, I want to serve my users animated images from videos. (webp will be returned only when AUTO_WEBP=True)

I have an original idea that transcode video into gifs, but the gifs are too large and bad in quality. webp is very good, but some devices do not support it.

I also considered saving the webp as the source file to be converted to gif when needed, but ffmpeg do not support decoding the animated webp currently. So in this PR, I am using a pretty tricky hack to save the source file: return the original mp4 buffer when read(extension='.mp4', quality=None).

I started learning Python only a few days ago and had never wrote so much Python, please help to correct me if I made any stupid mistakes.

Thank you!

@andreaugusto

This comment has been minimized.

Copy link
Contributor

andreaugusto commented May 22, 2017

Heya @winguse! Thanks for this PR!

First of all, gratz for you first Python code :) Way to go!

We've been talking about a better way of adding new Engines to Thumbor, but the actual code has some issues that needs to be addressed first. Right now, through configuration, we are only able to change an Engine for all images or change an Engine for gifs, separately. The only way to add a brand new Engine for another mimetype is, as this PR did, adding another if to the _fetch method. IMO we should fix it and add the possibility to select an Engine based on the resource's mimetype. We'd have to change this so that it is not needed anymore to add more ifs to the code. We'd also have to change Thumbor's configuration to support different Engines based on a resource's mimetype.

e.g.:

Config.define(
    'ENGINES', [
        ('video/mp4', 'thumbor.engines.mp4'),
        ('image/gif', 'thumbor.engines.gif'),
        ('default', 'thumbor.engines.pil'),
    ]

With that being said, I'd like to propose that:

1- This PR would be only about adding a new mp4 Engine.
2- Change some things in the actual implementation of the PR'ed Engine (long methods, code legibility and stuff). We'd help you with the actual changes.
3- Open a new issue to add support to new Engines based on configuration.
4- Wait for 3 to be finished so that this Engine could use the correct way to add a new Engine to Thumbor.

I'll take care of 3 and will link it in here ASAP.

Then again, thank you for the PR and keep up the good work!

@winguse

This comment has been minimized.

Copy link
Author

winguse commented May 23, 2017

Thanks @andreaugusto , good to know we can avoid ifs, that should be a good idea. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.