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

[Gilles Chanteperdrix]

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.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.

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150408/86bbcf84/attachment.asc>