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

Adding support for videos #18

Merged
merged 14 commits into from Jul 21, 2013

Conversation

2 participants
@chmduquesne
Contributor

chmduquesne commented Jul 17, 2013

It took some work, but, at last, here's a version with video support! You're welcome to add any test that comes to your mind, and to let me know what you think of the whole patch.

Cheers!

@chmduquesne

This comment has been minimized.

Contributor

chmduquesne commented Jul 17, 2013

A few comments about the code.

  • I chose to go for adding another field 'vid' to entries in the db. 'vid' and 'img' could probably be merged together in an abstract field 'media', but I thought it was preferable in order to simplify differentiating between videos and images in the templates.
  • Thumbnail generation for videos is ugly. I would have prefered to find the right options in ffmpeg for directly generating the right thumbnail, but the manual eventually drove me crazy. So the approach I ended up going for is much more reliable, because it guarantees that the video thumbnails have exactly the same properties as the image thumbnails.
  • The video in the galleria theme is an ugly hack: I used the "data-layer" attribute (see http://galleria.io/docs/references/data/#adding-a-layer-above-the-content) in order to display the video over the image, and I pointed the source image to a transparent png, such that the video appears over something transparent. Also, I believe the video should appear bigger, but its size seems to be bounded by the parent div. I am much more satisfied with what I did with the colorbox theme.
@@ -5,3 +5,4 @@ Markdown
Pillow
pilkit
pytest
ffmpeg

This comment has been minimized.

@saimn

saimn Jul 18, 2013

Owner

You should not add ffmpeg here, the requirements.txt file is only for python modules.

This comment has been minimized.

@chmduquesne

chmduquesne Jul 18, 2013

Contributor

Ok, I was not sure. Removed.

saimn added a commit that referenced this pull request Jul 21, 2013

Merge pull request #18 from chmduquesne/master
Adding support for videos

@saimn saimn merged commit e886172 into saimn:master Jul 21, 2013

1 check passed

default The Travis CI build passed
Details
@saimn

This comment has been minimized.

Owner

saimn commented Jul 21, 2013

Merged, thanks @chmduquesne !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment