diff mbox series

[FFmpeg-devel] avformat/concatdec: add support for setting input options

Message ID 20210121230013.22751-1-jeebjp@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel] avformat/concatdec: add support for setting input options
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Jan Ekström Jan. 21, 2021, 11 p.m. UTC
This way protocol or format related options can be set for all
of the files opened during concatenation.
---
 libavformat/concatdec.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

Jan Ekström Jan. 25, 2021, 7:54 a.m. UTC | #1
On Fri, Jan 22, 2021 at 1:00 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> This way protocol or format related options can be set for all
> of the files opened during concatenation.

Ping for reviews.

Jan
Nicolas George Jan. 25, 2021, 9:52 a.m. UTC | #2
Jan Ekström (12021-01-22):
> This way protocol or format related options can be set for all
> of the files opened during concatenation.
> ---
>  libavformat/concatdec.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)

I know it is a little more work, but options need to be set segment per
segment, in the script file.

Thanks for working on this. It has been needed for a long time.

Regards,
Jan Ekström Jan. 25, 2021, 11:28 a.m. UTC | #3
On Mon, Jan 25, 2021 at 11:52 AM Nicolas George <george@nsup.org> wrote:
>
> Jan Ekström (12021-01-22):
> > This way protocol or format related options can be set for all
> > of the files opened during concatenation.
> > ---
> >  libavformat/concatdec.c | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
>
> I know it is a little more work, but options need to be set segment per
> segment, in the script file.

I knew that would be useful, but opted out of it since:
1. For now I didn't yet need it.
2. I wanted to keep out of the actual playlist parsing code.
3. Possible security ramifications (since we already have "safe" /
"unsafe" playlist entries).

It is a relatively simple extension yes, if you ignore the points 2/3.
You have an AVDictionary per file and after initializing the
AVDictionary in the opening function and applying global input options
- copy the contents of the per-file options there, too.

Jan
Nicolas George Jan. 25, 2021, 12:50 p.m. UTC | #4
Jan Ekström (12021-01-25):
> I knew that would be useful, but opted out of it since:
> 1. For now I didn't yet need it.

That is not a good reason.

> 2. I wanted to keep out of the actual playlist parsing code.

I know, file parsing code is not nice. But this is what needs doing.

> 3. Possible security ramifications (since we already have "safe" /
> "unsafe" playlist entries).

You are right, we need to treat options as unsave.

> 
> It is a relatively simple extension yes, if you ignore the points 2/3.
> You have an AVDictionary per file and after initializing the
> AVDictionary in the opening function and applying global input options
> - copy the contents of the per-file options there, too.

Please, no half-baked solution.

Regards,
Jan Ekström Feb. 3, 2021, 7:58 a.m. UTC | #5
On Mon, Jan 25, 2021 at 2:50 PM Nicolas George <george@nsup.org> wrote:
>
> Jan Ekström (12021-01-25):
> > I knew that would be useful, but opted out of it since:
> > 1. For now I didn't yet need it.
>
> That is not a good reason.
>
> > 2. I wanted to keep out of the actual playlist parsing code.
>
> I know, file parsing code is not nice. But this is what needs doing.
>
> > 3. Possible security ramifications (since we already have "safe" /
> > "unsafe" playlist entries).
>
> You are right, we need to treat options as unsave.
>
> >
> > It is a relatively simple extension yes, if you ignore the points 2/3.
> > You have an AVDictionary per file and after initializing the
> > AVDictionary in the opening function and applying global input options
> > - copy the contents of the per-file options there, too.
>
> Please, no half-baked solution.
>

I am pretty sure you mis-understood what I meant here. I meant that if
you exclude those bits that I was not interested in (namely, parsing
the playlist file and handling the "safe" flag), the extension would
be simple. Not meaning half-baked solutions.

Pushed a version last night which seems to now include the parsing
changes to a branch:
https://github.com/jeeb/ffmpeg/commits/add_input_options_to_concatdec

If this looks good, I'll just update the documentation and post it on
the mailing list.

Jan
diff mbox series

Patch

diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
index 6d5b9914f9..3c81d9fea6 100644
--- a/libavformat/concatdec.c
+++ b/libavformat/concatdec.c
@@ -66,6 +66,7 @@  typedef struct {
     ConcatMatchMode stream_match_mode;
     unsigned auto_convert;
     int segment_time_metadata;
+    AVDictionary *input_options;
 } ConcatContext;
 
 static int concat_probe(const AVProbeData *probe)
@@ -329,6 +330,7 @@  static int open_file(AVFormatContext *avf, unsigned fileno)
 {
     ConcatContext *cat = avf->priv_data;
     ConcatFile *file = &cat->files[fileno];
+    AVDictionary *options = NULL;
     int ret;
 
     if (cat->avf)
@@ -344,12 +346,31 @@  static int open_file(AVFormatContext *avf, unsigned fileno)
     if ((ret = ff_copy_whiteblacklists(cat->avf, avf)) < 0)
         return ret;
 
-    if ((ret = avformat_open_input(&cat->avf, file->url, NULL, NULL)) < 0 ||
+    if (cat->input_options &&
+        (ret = av_dict_copy(&options, cat->input_options, 0) < 0))
+        return ret;
+
+    if ((ret = avformat_open_input(&cat->avf, file->url, NULL, &options)) < 0 ||
         (ret = avformat_find_stream_info(cat->avf, NULL)) < 0) {
         av_log(avf, AV_LOG_ERROR, "Impossible to open '%s'\n", file->url);
         avformat_close_input(&cat->avf);
+        av_dict_free(&options);
         return ret;
     }
+
+    if (av_dict_count(options)) {
+        AVDictionaryEntry *en = NULL;
+
+        while ((en = av_dict_get(options, "", en, AV_DICT_IGNORE_SUFFIX))) {
+            av_log(avf, AV_LOG_WARNING,
+                   "Option '%s' set to '%s' was ignored when opening %s "
+                   "with the %s reader!\n",
+                   en->key, en->value, file->url, cat->avf->iformat->name);
+        }
+    }
+
+    av_dict_free(&options);
+
     cat->cur_file = file;
     file->start_time = !fileno ? 0 :
                        cat->files[fileno - 1].start_time +
@@ -764,6 +785,9 @@  static const AVOption options[] = {
       OFFSET(auto_convert), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, DEC },
     { "segment_time_metadata", "output file segment start time and duration as packet metadata",
       OFFSET(segment_time_metadata), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, DEC },
+    { "input_options",
+      "set options for all opened inputs using a :-separated list of key=value pairs",
+      OFFSET(input_options), AV_OPT_TYPE_DICT, { .str = NULL }, 0, 0, DEC },
     { NULL }
 };