Added thumbnail functionality #20

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
6 participants

flatzo commented Mar 14, 2012

No description provided.

Collaborator

dbackeus commented Mar 20, 2012

Will probably not merge this as I'm not sure it's the way to go for screenshot / thumbnail support. If I'll implement screenshots in the future I will definitely look at your code first though. Thanks!

flatzo commented Mar 20, 2012

This is up to you, but this works fine with every video I've tried and seems to be faster than any other way I've come across. Takes only the video stream at the right position and give you the frame. Even IF their is a better way, this works, doesn't have any effect on the rest of the code and is better than nothing ... and it still can be changed if a better solution comes in.

@mistydemeo mistydemeo and 1 other commented on an outdated diff Mar 21, 2012

lib/ffmpeg/thumbnailing_options.rb
@@ -0,0 +1,20 @@
+module FFMPEG
+ class ThumbnailingOptions
+ attr_reader :preceding, :following
+ DEFAULT_POSITION = 4
+ ##
+ # Position is in seconds
+ # output
+ def initialize(options = {})
+ position = options.has_key?(:position) ? options[:position] : DEFAULT_POSITION
+ width = options.has_key?(:width) ? options[:width] : nil
+ height = options.has_key?(:height) ? options[:height] : nil
+
+ @preceding = "-itsoffset -#{position}"
@mistydemeo

mistydemeo Mar 21, 2012

Contributor

I don't think this does what you think it does! itsoffset is a timestamp offset, not a seek time. You're looking for -ss. (Also, it's dangerous to put the seek before the -i flag - this will work for some formats, and not others.)

@flatzo

flatzo Mar 21, 2012

The problem with -ss is that it "[...]decodes but discards input until the timestamps reach position." so you waste a lot of time for decoding useless stuff. -itsoffset may not offer the same precision, but fair enough and with better performances. And from what i understand, -itsoffset is only used on input file, so I doubt their would have any problem with it. Correct me if I'm wrong.

@mistydemeo

mistydemeo Mar 21, 2012

Contributor

I tested with ffmpeg 0.10, and -itsoffset has no effect when choosing input offset in the invocation you're using here - it's always the same frame no matter what number's used. I believe this is because -itsoffset is not a seek option, it's a timestamp offset option.

The time taken while using -ss depends on whether you place it before or after the input video. It's very fast when placed before the input, though that doesn't work with every format.

@flatzo

flatzo Mar 21, 2012

I've never come across this problem, but seems like -ss is faster when used in front of -i than itsoffset. You're right, it's not exactly what I tought it was.

I'm thinking about doing it with -ss in front and if their's a error in the seeking, just having a fallback call to ffmpeg with -ss after -i. Any thought on this ?

@mistydemeo

mistydemeo Mar 21, 2012

Contributor

There won't be an error in the seeking, the output screen will just be garbage. You either need to select based on the video properties, or allow the user to select the safe or unsafe seek. I would argue that safe should be the default. (That's how I implemented it in my screenshot function that I didn't get around to polishing & submitting.)

@flatzo flatzo - Following the recommendations of mistydemeo, using -ss instead of -…
…tisoffset and use a safe way by default.

- Refactoring thumbnail to screenshot
484c5aa

rheaton commented Apr 4, 2012

+1 :)

coreyti commented Apr 4, 2012

+1

Contributor

stakach commented Apr 5, 2012

+1 I merged this code into my pull request too streamio/streamio-ffmpeg#24

Collaborator

dbackeus commented Apr 24, 2012

Due to the popular demand I have implemented a screenshot method on Movie. It shares the same options as the transcode method making it more powerful than the approach in this pull request and keeping code simpler at the same time.

It does however lack the "safe" feature. Which I think is a fair tradeoff to avoid complicating the code for a feature which is by definition not safe (keep in mind this project is aiming to be the clean and simple alternative to the rvideo gem).

Please have a look at the README and see if it makes sense to you guys.

flatzo commented Apr 24, 2012

It looks great, but I don't get what you mean @dbackeus with "a feature which is by definition not safe" as, from my understanding, with params after the input you'll always get the expected result. Would it be possible, to create a combination of parameters which would create any problems ? In other words, isn't it a little risky to allow all video encoding options ?

Anyway, it works pretty well, nice job.

Collaborator

dbackeus commented Apr 25, 2012

What I meant is that there is now only the safe way available. The seek time will always be after the input. And that I think that's the best way to go for keeping the code (and api) clean and simple.

Contributor

mistydemeo commented Apr 25, 2012

I think that including only the safe option is the right choice for this.

flatzo closed this Apr 29, 2012

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