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

Honor `use_orig` with videos #153

Merged
merged 4 commits into from Apr 27, 2015

Conversation

2 participants
@jasuarez
Contributor

jasuarez commented Mar 16, 2015

So far use_orig is totally ignored for the case of videos.

But giving that lot of times we have a big amount of videos that we don't want to process because it is expensive, or they are already web-friendly.

This set of commits makes use_orig to be respected in case of videos.

I was doubting between following this, or adding a new video_use_orig (and video_orig_link). But at the end I decided to use the same property for both cases.

outname = os.path.join(outpath, filename)
utils.copy(source, outname, symlink=settings['orig_link'])
return outname

This comment has been minimized.

@saimn

saimn Mar 16, 2015

Owner

This should probably go at the beginning of process_video, as it would avoid to modify the path here and avoid to return a modified filename.

This comment has been minimized.

@jasuarez

jasuarez Mar 17, 2015

Contributor

Correct. I added it in generate_video because the same is done in generate_image. But I'll move it to process_video.

@@ -120,7 +120,7 @@
<div style='display:none'>
<div id="{{ media.filename|replace('.', '')|replace(' ', '') }}">
<video controls>
<source src='{{ media.filename }}' type='video/webm' />
<source src='{{ media.filename }}' {% if not settings.use_orig %}type='video/webm'{% endif %}/>

This comment has been minimized.

@saimn

saimn Mar 16, 2015

Owner

What happens when the type is not specified ?

This comment has been minimized.

@jasuarez

jasuarez Mar 17, 2015

Contributor

Nothing :)

I mean, I've been testing in Firefox and Epiphany, and didn't perceive anything weird: videos were played normally, without any kind of warning.

This comment has been minimized.

@saimn

saimn Apr 22, 2015

Owner

I would be easy to add the type as an attribute of the Video object, based on the file extension, so it is probably better to do it just in case.

@@ -114,6 +114,7 @@ def test_image(settings, tmpdir):
def test_video(settings, tmpdir):
settings['destination'] = str(tmpdir)
settings['use_orig'] = False

This comment has been minimized.

@saimn

saimn Mar 16, 2015

Owner

This surprised me a bit, use_orig should be False by default, but it seems that the settings dict is modified by test_media_orig, which is bad. I will try to fix this (unless you want to take a look :-)).

This comment has been minimized.

@jasuarez

jasuarez Mar 17, 2015

Contributor

Yes, I was quite puzzled about this issue.

This comment has been minimized.

@saimn

saimn Apr 22, 2015

Owner

Fixed with 94c9858

self.dst_path = join(settings['destination'], path, base + '.webm')
if not settings['use_orig']:
self.filename = self.url = base + '.webm'
self.dst_path = join(settings['destination'], path, base + '.webm')

This comment has been minimized.

@saimn

saimn Mar 16, 2015

Owner

Maybe add a test on the extension to make sure that it is supported by the video tag (webm, mp4, ogv) ?

This comment has been minimized.

@jasuarez

jasuarez Mar 17, 2015

Contributor

Good point.

Actually, I wonder how it should behave when it finds out a video not supported, like an avi file. So far it just, it just copies the file in the output. But when opening with a browser, the video can't be played.

I see here two options:

  • Copy the file anyway, but provides a warning message in the output telling the file is not supported.
  • Adding a new option, like convert_unsupported or similar, that performs a conversion on these cases. So the avi file would be converted in webm.

This comment has been minimized.

@saimn

saimn Apr 22, 2015

Owner

Sorry I forgot to answer. I think that unsupported files should be converted without taking care of the use_orig setting, and without adding a new setting. Maybe just make it clear in the doc that use_orig applies only to supported files.

@saimn

This comment has been minimized.

Owner

saimn commented Mar 16, 2015

Thanks for this PR, it is indeed something that was missing (as other feature like mp4/multiple format support).

@saimn

This comment has been minimized.

Owner

saimn commented Apr 22, 2015

Hi @jasuarez ,
I'm thinking about releasing a new version, any chance to get this in it ?

@jasuarez

This comment has been minimized.

Contributor

jasuarez commented Apr 22, 2015

i'm bit busy these weeks, but I'll try to address your comments this weekend

@jasuarez jasuarez force-pushed the jasuarez:honor-use_orig-with-videos branch from e2aeac9 to 2be7878 Apr 26, 2015

@jasuarez

This comment has been minimized.

Contributor

jasuarez commented Apr 26, 2015

@saimn Re-created the PR addressing your comments, plus rebasing against latest master.

@jasuarez jasuarez force-pushed the jasuarez:honor-use_orig-with-videos branch 2 times, most recently from 2be7878 to 8d70776 Apr 26, 2015

@saimn

This comment has been minimized.

Owner

saimn commented Apr 27, 2015

Thanks @jasuarez ! Could you just revert the change in test_video as it should no more be necessary ?

@saimn

This comment has been minimized.

Owner

saimn commented Apr 27, 2015

Also please add yourself to AUTHORS ;)

jasuarez added some commits Apr 26, 2015

Honor 'use_orig' for videos
When 'use_orig' is True, copy the original video files (or symlink them,
depending if 'orig_link' is True) to the destination, and use them as they are,
without processing.

This setting is ignored for all files not supported in HTML5; those are
converted anyway, because otherwise we could not reproduce them.
Set HTML5 video type correctly
As we can use the videos as they are, without conversion, we need to set
the video type correctly.

@jasuarez jasuarez force-pushed the jasuarez:honor-use_orig-with-videos branch from 8d70776 to 0a3198b Apr 27, 2015

@jasuarez

This comment has been minimized.

Contributor

jasuarez commented Apr 27, 2015

@saimn I need to push the change in test_video because otherwise I'm breaking the tests.

@saimn saimn merged commit ca61eed into saimn:master Apr 27, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@saimn

This comment has been minimized.

Owner

saimn commented Apr 27, 2015

Thanks, merged in 2f90da5

I need to push the change in test_video because otherwise I'm breaking the tests.

No, before all tests shared the settings dict, but since a recent change on master there is a settings doct for each test.

@saimn saimn modified the milestone: 0.10 Aug 31, 2015

@jasuarez jasuarez deleted the jasuarez:honor-use_orig-with-videos branch Jan 25, 2017

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