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

Allow other options to ffmpeg #7

Merged
merged 2 commits into from
Mar 11, 2014
Merged

Conversation

albertsun
Copy link
Contributor

Not sure about the syntax for the formatting of these options, but this was useful for Alastair and I so I figure it may be useful for others.

@sjwilliams
Copy link
Owner

@albertsun do you mind documenting the use in README.md and adding a test? We'll then push it out to npm as 0.1.1 for you and Alastair.

@albertsun
Copy link
Contributor Author

Added. But no rush for pushing this out to npm, you may decide there's a more elegant solution for this.

It's not blocking us right now since we're installing from the forked version.

@sjwilliams
Copy link
Owner

I had some time to look at this today. I think there's a cleaner approach. I've worked up a feature branch that would allow you to, I think, achieve the same goal with this config:

options: {
    sizes: [{
        width: 640,
        filter: 'scale=640:trunc(ow/a/2)*2,crop=360:360:140:0',
        poster: true
    },{
        width: 320,
        poster: true
    }],
    additionalFlags: [
        {'-g': '3'}
    ]
}

Here, two sizes would be made. The 320 is standard. The 640 has your cropping magic.

For the filter, I just simplified the name. Everything is FFMpeg and custom, so just make the property filter.

For the additional flags, that functionality kinda already existed, but was clunky. You could have added the flag as part of a custom encode in the options.

options: {
  encodes: [
    mp4:
    [
        {'-flag', 'value'},
        {'-flag', 'value'},
        ....
        {'-g', '3'},
    ]
}

But you'd have had to look up the default settings in the source for that particular encode. And it's not DRY at all.

So, the additionalFlags property I'm proposing would allow you augment defaults (and any per-encode settings) by additionally adding this array of flags to the options passed into FFMpeg.

Does this interface make sense for what you and Alastair are doing? Either way, I like where this is going and will probably merge them into master after some testing. If you have time, feel free to use this branch and new interface. Holler if you do and I'll keep tweaking for you.

@albertsun
Copy link
Contributor Author

Yep. That makes sense and is a lot cleaner than what I had hacked in.

Our project just wrapped tonight though, so I'll look to use this in a
future one.

On Mon, Mar 10, 2014 at 10:53 PM, Josh Williams [email protected]:

I had some time to look at this today. I think there's a cleaner approach.
I've worked up a feature branchhttps://github.com/sjwilliams/grunt-responsive-videos/tree/customoptionsthat would allow you to, I think, achieve the same goal with this config:

options: {
sizes: [{
width: 640,
filter: 'scale=640:trunc(ow/a/2)*2,crop=360:360:140:0',
poster: true
},{
width: 320,
poster: true
}],
additionalFlags: [
{'-g': '3'}
]}

Here, two sizes would be made. The 320 is standard. The 640 has your
cropping magic.

For the filter, I just simplified the name. Everything is FFMpeg and
custom, so just make the property filter.

For the additional flags, that functionality kinda already existed, but
was clunky. You could have added the flag as part of a custom encode in the
options.

options: {
encodes: [
mp4:
[
{'-flag', 'value'},
{'-flag', 'value'},
....
{'-g', '3'},
]}

But you'd have had to look up the default settings in the source for that
particular encode. And it's not DRY at all.

So, the additionalFlags property I'm proposing would allow you augment
defaults (and any per-encode settings) by additionally adding this array of
flags to the options passed into FFMpeg.

Does this interface make sense for what you and Alastair are doing? Either
way, I like where this is going and will probably merge them into master
after some testing. If you have time, feel free to use this branch and new
interface. Holler if you do and I'll keep tweaking for you.

Reply to this email directly or view it on GitHubhttps://github.com//pull/7#issuecomment-37258612
.

Albert Sun
[email protected]
215.253.8566

@sjwilliams sjwilliams merged commit e41110a into sjwilliams:master Mar 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants