On 24.01.2016 03:42, Michael Niedermayer wrote:
> From: Michael Niedermayer <mich...@niedermayer.cc>
>
> TODO: Docs
> TODO: version bump
>
> Note to maintainers: update tools
>
> Note, testing and checking for missing changes is needed
>
> Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc>
> ---
> ffmpeg_opt.c | 4 ++--
> libavdevice/lavfi.c | 2 +-
> libavformat/async.c | 2 +-
> libavformat/avformat.h | 7 ++++++
> libavformat/avio.c | 44
> ++++++++++++++++++++++++++++++++++---
> libavformat/avio.h | 4 ++++
> libavformat/aviobuf.c | 16 ++++++++++----
> libavformat/cache.c | 3 ++-
> libavformat/concat.c | 5 +++--
> libavformat/crypto.c | 5 +++--
> libavformat/dashenc.c | 9 ++++----
> libavformat/ftp.c | 10 +++++----
> libavformat/gopher.c | 4 ++--
> libavformat/hdsenc.c | 12 +++++-----
> libavformat/hls.c | 8 ++++---
> libavformat/hlsenc.c | 28 +++++++++++------------
> libavformat/hlsproto.c | 10 +++++----
> libavformat/http.c | 16 +++++++++-----
> libavformat/icecast.c | 3 ++-
> libavformat/md5proto.c | 5 +++--
> libavformat/mmst.c | 5 +++--
> libavformat/movenc.c | 2 +-
> libavformat/options_table.h | 1 +
> libavformat/rtmpcrypt.c | 5 +++--
> libavformat/rtmpproto.c | 10 +++++----
> libavformat/rtpproto.c | 10 ++++++---
> libavformat/rtsp.c | 20 ++++++++---------
> libavformat/rtspdec.c | 10 +++++----
> libavformat/sapdec.c | 5 +++--
> libavformat/sapenc.c | 9 +++++---
> libavformat/segment.c | 16 +++++++-------
> libavformat/smoothstreamingenc.c | 18 ++++++++-------
> libavformat/srtpproto.c | 3 ++-
> libavformat/subfile.c | 3 ++-
> libavformat/tee.c | 4 +++-
> libavformat/tls.c | 5 +++--
> libavformat/tls_securetransport.c | 5 +++--
> libavformat/url.h | 5 +++++
> libavformat/utils.c | 13 +++++++----
> libavformat/webm_chunk.c | 7 +++---
> 40 files changed, 230 insertions(+), 123 deletions(-)
>
To make reviewing easier it would be nice to split this into a patch
that adds the protocol_whitelist mechanism and a subsequent one,
which forwards the protocol_whitelist where necessary.
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 4964263..2fb9130 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -1815,6 +1815,13 @@ typedef struct AVFormatContext {
> * Demuxing: Set by user.
> */
> int (*open_cb)(struct AVFormatContext *s, AVIOContext **p, const char
> *url, int flags, const AVIOInterruptCB *int_cb, AVDictionary **options);
> +
> + /**
> + * ',' separated list of allowed protocols.
> + * - encoding: unused
> + * - decoding: set by user through AVOptions (NO direct access)
> + */
> + char *protocol_whitelist;
> } AVFormatContext;
I'm not sure if adding protocol_whitelist to the AVFormatContext is good.
Conceptually it would fit better in the AVIOContext. I think that would also
make it easier to set a default for it.
> diff --git a/libavformat/avio.c b/libavformat/avio.c
> index 05d0557..ad9712d 100644
> --- a/libavformat/avio.c
> +++ b/libavformat/avio.c
[...]
> @@ -296,18 +302,43 @@ int ffurl_alloc(URLContext **puc, const char *filename,
> int flags,
> return AVERROR_PROTOCOL_NOT_FOUND;
> }
>
> -int ffurl_open(URLContext **puc, const char *filename, int flags,
> - const AVIOInterruptCB *int_cb, AVDictionary **options)
> +int ffurl_open_whitelist(URLContext **puc, const char *filename, int flags,
> + const AVIOInterruptCB *int_cb, AVDictionary
> **options, const char *whitelist)
> {
> + AVDictionary *tmp_opts = NULL;
> + AVDictionaryEntry *e;
> int ret = ffurl_alloc(puc, filename, flags, int_cb);
> if (ret < 0)
> return ret;
> if (options && (*puc)->prot->priv_data_class &&
> (ret = av_opt_set_dict((*puc)->priv_data, options)) < 0)
> goto fail;
> +
> + if (!options)
> + options = &tmp_opts;
> +
> + av_assert0(!whitelist ||
> + !(e=av_dict_get(*options, "protocol_whitelist", NULL, 0)) ||
> + !strcmp(whitelist, e->value));
> +
> + if ((ret = av_dict_set(options, "protocol_whitelist", whitelist, 0)) < 0)
> + goto fail;
> +
Wouldn't it be much simpler to pass the protocol_whitelist directly in options
to ffurl_open, instead of adding a new function with a new parameter, just
to set the option here.
That way one could avoid the need for a new public avio_open_whitelist function.
Granted, it would be less explicit then, which might cause people to forget
to set it in the future.
I'm not sure which way would be better overall.
> if ((ret = av_opt_set_dict(*puc, options)) < 0)
> goto fail;
> +
> + whitelist = (*puc)->protocol_whitelist;
> + if (whitelist && av_match_list((*puc)->prot->name, whitelist, ',') <= 0)
> {
> + av_log(*puc, AV_LOG_ERROR, "Protocol not on whitelist!\n");
It would be nice to mention the protocol and the whitelist in the error message.
> + ret = AVERROR(EINVAL);
> + goto fail;
> + }
I think it would be better to check the whitelist in ffurl_connect, as that's
where it actually opens the new protocol and it is not only called by
ffurl_open.
Also, this patch doesn't include a mechanism setting a good default for the
protocol_whitelist, which would be needed to fix the problem of unintentionally
mixing local and remote data.
If the protocol_whitelist would be saved in the AVIOContext, such a mechanism
could be implemented in ffurl_open/ffurl_connect.
Best regards,
Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
