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 header() method to support HTTP/S Headers #890

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FullstackJack
Copy link

Headers are sometimes important for fetching source files from secure locations (i.e. Google Cloud Storage, AWS S3). This PR provides a clearcut method for adding HTTP headers with assurance that headers are applied in the right order on the ffmpeg and ffprobe commands.

  • New headers() method added.
  • README updated.
  • Tests written.

Examples

ffmpeg

ffmpeg()
  .input('https://www.googleapis.com/storage/v1/b/my-private-bucket/o/my-file.mp4?alt=media')
  .headers('Authorization: Bearer xyz123...')
  .headers('X-My-Custom-Header: 1234')
  .output('path/to/saved/file.mp4');

ffprobe

ffmpeg()
  .input('https://www.googleapis.com/storage/v1/b/my-private-bucket/o/my-file.mp4?alt=media')
  .headers(['Authorization: Bearer xyz123...', 'X-My-Custom-Header: 1234'])
  .ffprobe((err, res) => {
    ...
  });

@FullstackJack
Copy link
Author

Fixes #882

@FullstackJack
Copy link
Author

FullstackJack commented Mar 18, 2019

The TravisCI check seems to be choking on something unrelated to this PR. I ran my tests locally and they worked fine.

That TravisCI check is failing due to missing remote file @ https://www.johnvansickle.com/ffmpeg/builds/ffmpeg-git-64bit-static.tar.xz

@njoyard
Copy link
Member

njoyard commented Mar 19, 2019

That TravisCI check is failing due to missing remote file @ https://www.johnvansickle.com/ffmpeg/builds/ffmpeg-git-64bit-static.tar.xz

Yes, John changed his URLs, the new one is https://johnvansickle.com/ffmpeg/builds/ffmpeg-git-amd64-static.tar.xz

Copy link
Member

@njoyard njoyard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I'm not a fan of this solution. There is the obvious problem that header options will be added to all inputs (which is incoherent with how fluent-ffmpeg normally handles input options). Also I feel like allowing multiple header calls to add multiple headers adds a LOT of complexity.

Given that using the headers option currently works with inputOptions, maybe we could just add an alias helper that you can call as:

command.headers({header: value, header: value ...})

and whose implementation would simply be:

headers(headers) {
   this.inputOption('-headers', Object.keys(headers).map(k => k + ': ' + headers[k]).join('\r\n'))
}

@@ -236,6 +237,7 @@ module.exports = function(proto) {

// For each input, add input options, then '-i <source>'
return args.concat(
headers,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. Headers options will be added to all inputs.

@FullstackJack
Copy link
Author

I'll take a look at implementing these suggestions.

@njoyard
Copy link
Member

njoyard commented Mar 19, 2019

I'll make a separate PR to fix the test suite

@njoyard
Copy link
Member

njoyard commented Mar 19, 2019

Tests are fixed, you can rebase on master

@jashanbhullar
Copy link

Any updates on this?

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.

3 participants