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

New API: exe, @ffmpeg_env, ffmpeg_exe, ffprobe_exe #1

Merged
merged 9 commits into from
Jul 10, 2019

Conversation

asinghvi17
Copy link
Member

macro ffmpeg(args)
return quote
withenv(FFMPEG.execenv) do
$(args)
end
end
end

@asinghvi17
Copy link
Member Author

Have tested locally, so I'm pretty sure this branch works. @ianshmean @SimonDanisch any feedback?

@IanButterworth
Copy link
Member

IanButterworth commented Jul 10, 2019

How about a slightly shorter set of macros:

@ffmpeg `-version`
@ffprobe `-version`

I guess it would be good to accept both command strings and strings

@IanButterworth
Copy link
Member

Also, in the past, ffmpeg output has changed to different std's so VideoIO uses:

function collectexecoutput(exec::Cmd)
    out = Pipe(); err = Pipe()
    p = Base.open(pipeline(ignorestatus(exec), stdout=out, stderr=err))
    close(out.in); close(err.in)
    err_s = readlines(err); out_s = readlines(out)
    return (length(out_s) > length(err_s)) ? out_s : err_s
end

https://github.com/JuliaIO/VideoIO.jl/blob/38131d779e191e72ed9638e1f7053b901f57e087/src/util.jl#L31-L37

Maybe that's worth incorporating into the macros?

@asinghvi17
Copy link
Member Author

Could we do exe(args...; command = FFMPEG.ffmpeg) instead, to leave the macro scene more "pure"? We could then alias ffmpeg(args...) to exe(args...; command = FFMPEG.ffmpeg) and ffprobe(args...) to exe(args...; command = FFMPEG.ffprobe) in a similar way to Homebrew - and have special casing in exe for Cmd literals versus strings.

@IanButterworth
Copy link
Member

I like that

@IanButterworth
Copy link
Member

Wait, that would mean the ffmpeg namespace would need to be both a variable and function..

@asinghvi17
Copy link
Member Author

Yeah, just realized that :(. The macro thing will be an annoying API in my opinion, though...

@asinghvi17
Copy link
Member Author

asinghvi17 commented Jul 10, 2019

Could do fmpeg and fprobe instead? Not the cleanest but it could work for now...or we could just use exe.

Edit: mpeg and probe are also options.

@IanButterworth
Copy link
Member

I think dropping an f would be confusing.
How about ffmpeg_exec() etc.

@asinghvi17
Copy link
Member Author

@ianshmean done...

@IanButterworth
Copy link
Member

In this new regime, I find the @ffmpeg run(`$ffmpeg -version`) usage little confusing now.
I'd expect @ffmpeg to be limited to the ffmpeg executable. Seems redundant given exe() and ffmpeg_exe() now. Perhaps we drop the macro?

@asinghvi17
Copy link
Member Author

asinghvi17 commented Jul 10, 2019

Will do!

Edit: Actually, I'll probably rename it to @ffmpegenv, since not everything can be done by run (like pipelines).

@asinghvi17 asinghvi17 changed the title New macro @ffmpeg New API: exe, @ffmpeg_env, ffmpeg_exe, ffprobe_exe Jul 10, 2019
@asinghvi17 asinghvi17 self-assigned this Jul 10, 2019
@asinghvi17 asinghvi17 added documentation Improvements or additions to documentation enhancement New feature or request labels Jul 10, 2019
@asinghvi17 asinghvi17 merged commit 0926c49 into master Jul 10, 2019
@asinghvi17 asinghvi17 deleted the as/pipelines branch July 13, 2019 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants