On Wed, Apr 08, 2015 at 09:07:06PM +0200, Nicolas George wrote:
> Le nonidi 19 germinal, an CCXXIII, Gilles Chanteperdrix a écrit :
> > Signed-off-by: Gilles Chanteperdrix <gilles.chanteperd...@xenomai.org>
> > ---
> > libavformat/Makefile | 1 +
> > libavformat/allformats.c | 1 +
> > libavformat/youtubedl.c | 221
> > +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 223 insertions(+)
> > create mode 100644 libavformat/youtubedl.c
>
> The basic idea seems not bad, but the design is IMHO all wrong.
>
> > + fd = av_tempfile("youtubedl", &output_filename, 0, s);
>
> Do not use a temp file to read the single output of a command. Use a pipe
> directly: more reliable, less garbage to clean up.
As I explained, I did it this way for portability. system is ANSI
not POSIX, redirections work with windows shell too, and av_tempfile
is implemented by ffmpeg, so I expect it to be portable.
>
> > + if (fd < 0) {
>
> > + av_log(s, AV_LOG_ERROR, "Failed to create temporary file\n");
>
> Indentation is odd.
Probably tabs on my side.
>
> > + command = av_asprintf("%s > %s", command, output_filename);
>
> Never EVER do that. Do not try to quote arguments. You will get it wrong.
> Everybody gets it wrong.
Well, actually, it looks not so hard to me. One argument is an URL
so, we could url encode it before passing it to youtube-dl, and the
other is a format which is expected to be something like best,
bestvideo, bestaudio, or some numbers separated by slashes, so, we
can probably add sanity checks on the string. But I get it, system()
is a bad idea in general.
>
> DO NOT USE A SHELL.
>
> Use fork() + exec() directly to execute exactly the command you want. Even
> better, use posix_spawn(), it takes care of a lot of details that people
> usually get wrong with fork+exec.
Well, actually, vfork() + exec() is better than fork() + exec(),
both performance wise and because it works on machines without an
MMU, and the implementation of posix_spawn() in glibc is too
conservative and uses fork() + exec() even in situations where
vfork() could be used, see:
https://sourceware.org/bugzilla/show_bug.cgi?id=10354
So, better use vfork() + exec() (that is what system does for
instance). I did not use it simply because I thought ffmpeg could
have to be compiled on machines without the POSIX interface.
>
> If you really need a shell (you DO NOT need a shell just to establish a
> pipeline or a redirect), then use a constant command and shell command-line
> arguments ($1, $2...) for the variable parts.
>
> > + snprintf(buffer, sizeof(buffer), "youtube-dl -e '%s'", s->filename);
> > + ret = youtubedl_get_output(buffer, sizeof(buffer), s, buffer);
>
> > + snprintf(buffer, sizeof(buffer), "youtube-dl -f %s -g '%s'",
> > + yc->format, s->filename);
> > + ret = youtubedl_get_output(buffer, sizeof(buffer), s, buffer);
>
> I would say: go propose a patch to youtube-dl to make its output fully
> usable in a single call. Well, actually, it already exists.
>
> Then I suggest you just endeavour to write a pseudo-demuxer for the output
> of "youtube-dl -J". No need of a shell or an external command.
>
> > + err_close_input:
> > + avformat_close_input(&yc->fmtctx);
> > + err_free_media_url:
> > + av_free(media_url);
> > + err_free_pagetitle:
> > + av_free(pagetitle);
> > + return ret;
>
> Remember that all these operations are harmless if the corresponding
> variables are initialized to NULL, so you do not need to have that many exit
> points.
A habit.
>
> > + err_free_context:
> > + avformat_free_context(yc->fmtctx);
> > + goto err_free_media_url;
>
> That looks like spaghetti code.
Yes, this is called in only one place, so better put it there.
--
Gilles.
pgpxXC4mQA957.pgp
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
