[FFmpeg-devel] [PATCH] lavf: add SFTP protocol via libssh

[Derek Buitenhuis]

On 9/18/2013 3:27 PM, Lukasz Marek wrote:
> +Example: play a file stored on remote server.

s/play/Play/

> +#define DEBUG 1

This looks quite wrong

> +#include "libavutil/avstring.h"
> +#include "avformat.h"
> +#include "internal.h"
> +#include "url.h"
> +#include "libavutil/opt.h"

nit: Order.

> +#include <libssh/sftp.h>
> +#include <fcntl.h>

fcntl.h is not portable. Use the proper guards provided in config.h

> +#define D AV_OPT_FLAG_DECODING_PARAM
> +#define E AV_OPT_FLAG_ENCODING_PARAM

D and E are far to short and potentially pollute and conflict
with the namespacing of system headers.

> +static const AVOption options[] = {
> +    {"timeout", "set timeout of socket I/O operations", OFFSET(rw_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, -1, INT_MAX, D|E },
> +    {"truncate", "Truncate existing files on write", OFFSET(trunc), AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, E },
> +    {NULL}
> +};
> +
> +static const AVClass libssh_context_class = {
> +    .class_name     = "libssh",
> +    .item_name      = av_default_item_name,
> +    .option         = options,
> +    .version        = LIBAVUTIL_VERSION_INT,
> +};

This stuff is usually at the bottom of the file.

> +static int libssh_close(URLContext *h)
> +{
> +    LIBSSHContext *s = h->priv_data;
> +    if (s->file)
> +        sftp_close(s->file);
> +    if (s->sftp)
> +        sftp_free(s->sftp);
> +    if (s->session) {
> +        ssh_disconnect(s->session);
> +        ssh_free(s->session);
> +    }
> +    return 0;
> +}
> +
> +static int libssh_open(URLContext *h, const char *url, int flags)
> +{
> +    static const int verbosity = SSH_LOG_NOLOG;

Why is this a variable?

> +    if (!(s->session = ssh_new())) {
> +        goto fail;
> +    }

Unneeded braces.

> +    user = av_strtok(credencials, ":", &end);
> +    pass = av_strtok(end, ":", &end);

Any sanity checks needed? Probably not...


> +    if (flags & AVIO_FLAG_WRITE && flags & AVIO_FLAG_READ) {

nit: Paren would help ambiguity here.

> +        access = O_CREAT | O_RDWR;
> +        if (s->trunc)
> +            access |= O_TRUNC;
> +    } else if (flags & AVIO_FLAG_WRITE) {
> +        access = O_CREAT | O_WRONLY;
> +        if (s->trunc)
> +            access |= O_TRUNC;
> +    } else {
> +        access = O_RDONLY;
> +    }

These flags may need special treatment on windows (e.g. _O_CREAT). I'm
not sure we handle it yet.

> +    if (!(stat = sftp_fstat(s->file))) {
> +        av_log(h, AV_LOG_WARNING, "Cannot stat remote file %s.\n", path);
> +        s->filesize = -1;
> +    } else {
> +        s->filesize = stat->size;
> +        sftp_attributes_free(stat);
> +    }

This is not fatal?


> +  fail:
> +    libssh_close(h);
> +    return AVERROR(EIO);

You should pass along a more descriptive error when it fails, depending
on the situation.

if (...) {
    ret = AVERROR(...);
    goto fail;
}


> +    switch(whence) {
> +    case AVSEEK_SIZE:
> +        if (s->filesize != -1) {
> +            return s->filesize;
> +        } else {
> +            return AVERROR(EIO);
> +        }

Extra braces. Also, you forgot a break here.

> +    case SEEK_END:
> +        if (s->filesize != -1) {
> +            newpos = s->filesize + pos;
> +        } else {
> +            return AVERROR(EIO);
> +        }

Ditto.

> +static int libssh_read(URLContext *h, unsigned char *buf, int size)
> +{
> +    LIBSSHContext *s = h->priv_data;
> +    int read;

'read' is an illegal variable name I believe, due to read()'s existence.

- Derek