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.
> + if (fd < 0) {
> + av_log(s, AV_LOG_ERROR, "Failed to create temporary file\n");
Indentation is odd.
> + 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.
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.
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.
> + err_free_context:
> + avformat_free_context(yc->fmtctx);
> + goto err_free_media_url;
That looks like spaghetti code.
Regards,
--
Nicolas George
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
