[FFmpeg-devel] [PATCH 3/3] avformat: add youtube-dl based demuxer

[Nicolas George]

Le nonidi 19 germinal, an CCXXIII, Gilles Chanteperdrix a écrit :
> Signed-off-by: Gilles Chanteperdrix <gilles.chanteperdrix at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150408/d0f8cce6/attachment.asc>