| Message ID | 20210728121517.533173-5-george@nsup.org |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] libavformat/concatdec: remove support for unsafe=-1. | expand |
| 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 |
On 2021-07-28 17:45, Nicolas George wrote: > It was only still supported for subfile and only used by dvd2concat. Examples at http://www.ffmpeg.org/ffmpeg-protocols.html#subfile should be updated. Regards, Gyan > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavformat/avio.c | 34 +--------------------------------- > libavformat/dashdec.c | 2 +- > libavformat/hls.c | 2 +- > 3 files changed, 3 insertions(+), 35 deletions(-) > > diff --git a/libavformat/avio.c b/libavformat/avio.c > index 4846bbd8c6..1ce290737a 100644 > --- a/libavformat/avio.c > +++ b/libavformat/avio.c > @@ -111,39 +111,8 @@ static int url_alloc_for_protocol(URLContext **puc, const URLProtocol *up, > goto fail; > } > if (up->priv_data_class) { > - char *start; > *(const AVClass **)uc->priv_data = up->priv_data_class; > av_opt_set_defaults(uc->priv_data); > - if (av_strstart(uc->filename, up->name, (const char**)&start) && *start == ',') { > - int ret= 0; > - char *p= start; > - char sep= *++p; > - char *key, *val; > - p++; > - > - if (strcmp(up->name, "subfile")) > - ret = AVERROR(EINVAL); > - > - while(ret >= 0 && (key= strchr(p, sep)) && p<key && (val = strchr(key+1, sep))){ > - *val= *key= 0; > - if (strcmp(p, "start") && strcmp(p, "end")) { > - ret = AVERROR_OPTION_NOT_FOUND; > - } else > - ret= av_opt_set(uc->priv_data, p, key+1, 0); > - if (ret == AVERROR_OPTION_NOT_FOUND) > - av_log(uc, AV_LOG_ERROR, "Key '%s' not found.\n", p); > - *val= *key= sep; > - p= val+1; > - } > - if(ret<0 || p!=key){ > - av_log(uc, AV_LOG_ERROR, "Error parsing options string %s\n", start); > - av_freep(&uc->priv_data); > - av_freep(&uc); > - err = AVERROR(EINVAL); > - goto fail; > - } > - memmove(start, key+1, strlen(key)); > - } > } > } > if (int_cb) > @@ -255,8 +224,7 @@ static const struct URLProtocol *url_find_protocol(const char *filename) > size_t proto_len = strspn(filename, URL_SCHEME_CHARS); > int i; > > - if (filename[proto_len] != ':' && > - (strncmp(filename, "subfile,", 8) || !strchr(filename + proto_len + 1, ':')) || > + if (filename[proto_len] != ':' || > is_dos_path(filename)) > strcpy(proto_str, "file"); > else > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c > index 11966f905c..1d186802b3 100644 > --- a/libavformat/dashdec.c > +++ b/libavformat/dashdec.c > @@ -435,7 +435,7 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url, > ; > else if (av_strstart(url, "crypto", NULL) && !strncmp(proto_name, url + 7, strlen(proto_name)) && url[7 + strlen(proto_name)] == ':') > ; > - else if (strcmp(proto_name, "file") || !strncmp(url, "file,", 5)) > + else if (strcmp(proto_name, "file")) > return AVERROR_INVALIDDATA; > > av_freep(pb); > diff --git a/libavformat/hls.c b/libavformat/hls.c > index 3c1b80f60c..e4d24b6fe0 100644 > --- a/libavformat/hls.c > +++ b/libavformat/hls.c > @@ -667,7 +667,7 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url, > ; > else if (av_strstart(url, "data", NULL) && !strncmp(proto_name, url + 5, strlen(proto_name)) && url[5 + strlen(proto_name)] == ':') > ; > - else if (strcmp(proto_name, "file") || !strncmp(url, "file,", 5)) > + else if (strcmp(proto_name, "file")) > return AVERROR_INVALIDDATA; > > av_dict_copy(&tmp, *opts, 0);
Gyan Doshi (12021-07-28): > > > On 2021-07-28 17:45, Nicolas George wrote: > > It was only still supported for subfile and only used by dvd2concat. > > Examples at http://www.ffmpeg.org/ffmpeg-protocols.html#subfile should be > updated. Good catch, thanks. Locally committed this chunk: diff --git a/doc/protocols.texi b/doc/protocols.texi index 726e5f1c44..227614b0b3 100644 --- a/doc/protocols.texi +++ b/doc/protocols.texi @@ -1701,17 +1701,17 @@ Examples: Extract a chapter from a DVD VOB file (start and end sectors obtained externally and multiplied by 2048): @example -subfile,,start,153391104,end,268142592,,:/media/dvd/VIDEO_TS/VTS_08_1.VOB +-start 153391104 -end 268142592 subfile:/media/dvd/VIDEO_TS/VTS_08_1.VOB @end example Play an AVI file directly from a TAR archive: @example -subfile,,start,183241728,end,366490624,,:archive.tar +-start 183241728 -end 366490624 subfile:archive.tar @end example Play a MPEG-TS file from start offset till end: @example -subfile,,start,32815239,end,0,,:video.ts +-start 32815239 -end 0 subfile:video.ts @end example @section tee Regards,
Nicolas George: > It was only still supported for subfile and only used by dvd2concat. > The latter statement is not true: This is public API; anyone can have used it for any purpose. Your 2/5 adds a replacement for using it with dvd2concat, but there are other usages, too; e.g. concatenating several subfile files (each with its own start and end) with the concat protocol won't be possible any more with this patch. > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavformat/avio.c | 34 +--------------------------------- > libavformat/dashdec.c | 2 +- > libavformat/hls.c | 2 +- > 3 files changed, 3 insertions(+), 35 deletions(-) > > diff --git a/libavformat/avio.c b/libavformat/avio.c > index 4846bbd8c6..1ce290737a 100644 > --- a/libavformat/avio.c > +++ b/libavformat/avio.c > @@ -111,39 +111,8 @@ static int url_alloc_for_protocol(URLContext **puc, const URLProtocol *up, > goto fail; > } > if (up->priv_data_class) { > - char *start; > *(const AVClass **)uc->priv_data = up->priv_data_class; > av_opt_set_defaults(uc->priv_data); > - if (av_strstart(uc->filename, up->name, (const char**)&start) && *start == ',') { > - int ret= 0; > - char *p= start; > - char sep= *++p; > - char *key, *val; > - p++; > - > - if (strcmp(up->name, "subfile")) > - ret = AVERROR(EINVAL); > - > - while(ret >= 0 && (key= strchr(p, sep)) && p<key && (val = strchr(key+1, sep))){ > - *val= *key= 0; > - if (strcmp(p, "start") && strcmp(p, "end")) { > - ret = AVERROR_OPTION_NOT_FOUND; > - } else > - ret= av_opt_set(uc->priv_data, p, key+1, 0); > - if (ret == AVERROR_OPTION_NOT_FOUND) > - av_log(uc, AV_LOG_ERROR, "Key '%s' not found.\n", p); > - *val= *key= sep; > - p= val+1; > - } > - if(ret<0 || p!=key){ > - av_log(uc, AV_LOG_ERROR, "Error parsing options string %s\n", start); > - av_freep(&uc->priv_data); > - av_freep(&uc); > - err = AVERROR(EINVAL); > - goto fail; > - } > - memmove(start, key+1, strlen(key)); > - } > } > } > if (int_cb) > @@ -255,8 +224,7 @@ static const struct URLProtocol *url_find_protocol(const char *filename) > size_t proto_len = strspn(filename, URL_SCHEME_CHARS); > int i; > > - if (filename[proto_len] != ':' && > - (strncmp(filename, "subfile,", 8) || !strchr(filename + proto_len + 1, ':')) || > + if (filename[proto_len] != ':' || > is_dos_path(filename)) > strcpy(proto_str, "file"); > else > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c > index 11966f905c..1d186802b3 100644 > --- a/libavformat/dashdec.c > +++ b/libavformat/dashdec.c > @@ -435,7 +435,7 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url, > ; > else if (av_strstart(url, "crypto", NULL) && !strncmp(proto_name, url + 7, strlen(proto_name)) && url[7 + strlen(proto_name)] == ':') > ; > - else if (strcmp(proto_name, "file") || !strncmp(url, "file,", 5)) > + else if (strcmp(proto_name, "file")) > return AVERROR_INVALIDDATA; > > av_freep(pb); > diff --git a/libavformat/hls.c b/libavformat/hls.c > index 3c1b80f60c..e4d24b6fe0 100644 > --- a/libavformat/hls.c > +++ b/libavformat/hls.c > @@ -667,7 +667,7 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url, > ; > else if (av_strstart(url, "data", NULL) && !strncmp(proto_name, url + 5, strlen(proto_name)) && url[5 + strlen(proto_name)] == ':') > ; > - else if (strcmp(proto_name, "file") || !strncmp(url, "file,", 5)) > + else if (strcmp(proto_name, "file")) > return AVERROR_INVALIDDATA; > > av_dict_copy(&tmp, *opts, 0); >
Andreas Rheinhardt (12021-07-29): > The latter statement is not true: This is public API; anyone can have > used it for any purpose. Your 2/5 adds a replacement for using it with > dvd2concat, but there are other usages, too; e.g. concatenating several > subfile files (each with its own start and end) with the concat protocol > won't be possible any more with this patch. This feature was never documented, and only incidentally mentioned in the examples for subfile, so I consider it is more akin to avpriv than to a public API. Do you have practical examples where concatenating subfiles is useful? Regards,
Nicolas George (12021-07-30): > Andreas Rheinhardt (12021-07-29): > > The latter statement is not true: This is public API; anyone can have > > used it for any purpose. Your 2/5 adds a replacement for using it with > > dvd2concat, but there are other usages, too; e.g. concatenating several > > subfile files (each with its own start and end) with the concat protocol > > won't be possible any more with this patch. > > This feature was never documented, and only incidentally mentioned in > the examples for subfile, so I consider it is more akin to avpriv than > to a public API. > > Do you have practical examples where concatenating subfiles is useful? This feature makes several parts of the code harder to understand, see hls.c and dashdec.c. I have also asked on the users mailing-lists: https://ffmpeg.org/pipermail/ffmpeg-user/2021-August/date.html http://ffmpeg.org/pipermail/libav-user/2021-August/012836.html Unless somebody uses it in the real world, I would favor just getting rid of it. Regards,
On Sat, 14 Aug 2021, Nicolas George wrote: > Nicolas George (12021-07-30): >> Andreas Rheinhardt (12021-07-29): >>> The latter statement is not true: This is public API; anyone can have >>> used it for any purpose. Your 2/5 adds a replacement for using it with >>> dvd2concat, but there are other usages, too; e.g. concatenating several >>> subfile files (each with its own start and end) with the concat protocol >>> won't be possible any more with this patch. >> >> This feature was never documented, and only incidentally mentioned in >> the examples for subfile, so I consider it is more akin to avpriv than >> to a public API. >> >> Do you have practical examples where concatenating subfiles is useful? > > This feature makes several parts of the code harder to understand, see > hls.c and dashdec.c. > > I have also asked on the users mailing-lists: > > https://ffmpeg.org/pipermail/ffmpeg-user/2021-August/date.html > http://ffmpeg.org/pipermail/libav-user/2021-August/012836.html > > Unless somebody uses it in the real world, I would favor just getting > rid of it. I think this is one of those cases when we know that strictly speaking it breaks existing API / behaviour but we go ahead with it anyway because it is minor enough. And the feature to specify options in protocols was ugly in the first place. So I am OK with removing it. Regards, Marton
Nicolas George: > Nicolas George (12021-07-30): >> Andreas Rheinhardt (12021-07-29): >>> The latter statement is not true: This is public API; anyone can have >>> used it for any purpose. Your 2/5 adds a replacement for using it with >>> dvd2concat, but there are other usages, too; e.g. concatenating several >>> subfile files (each with its own start and end) with the concat protocol >>> won't be possible any more with this patch. >> >> This feature was never documented, and only incidentally mentioned in >> the examples for subfile, so I consider it is more akin to avpriv than >> to a public API. >> >> Do you have practical examples where concatenating subfiles is useful? > > This feature makes several parts of the code harder to understand, see > hls.c and dashdec.c. > I do not really get that: The option passing syntax is restricted to the subfile protocol, so I don't know why we allow it for the file protocol in dashdec and hls. Unless I am missing something, this could (and should) be removed at once. > I have also asked on the users mailing-lists: > > https://ffmpeg.org/pipermail/ffmpeg-user/2021-August/date.html > http://ffmpeg.org/pipermail/libav-user/2021-August/012836.html > > Unless somebody uses it in the real world, I would favor just getting > rid of it. > I use it to concatenate parts of files: When a DVR of mine has to split files due to the 4GB FAT-32 boundary, it does not do so cleanly; for some reason the first 96256B of the second file are duplicated, i.e. they coincide with bytes 96256-192511. For the third file, about a MB starting from offset 96256 is duplicated. - Andreas
Andreas Rheinhardt (12021-08-14): > I do not really get that: The option passing syntax is restricted to the > subfile protocol, so I don't know why we allow it for the file protocol > in dashdec and hls. Unless I am missing something, this could (and > should) be removed at once. It must be leftover code from when it was supported for all protocols. Not requiring this kind of convoluted tests in new code is also a motivation for just removing the last trace of this feature. > I use it to concatenate parts of files: When a DVR of mine has to split > files due to the 4GB FAT-32 boundary, it does not do so cleanly; for > some reason the first 96256B of the second file are duplicated, i.e. > they coincide with bytes 96256-192511. For the third file, about a MB > starting from offset 96256 is duplicated. Makes sense. I think we should decide that protocols like concat and subfile, protocols where part(s) of the pseudo-URI are themselves pseudo-URI with other protocols (let us call them metaprotocols) should always involve a solution to pass options to the sub-protocols. Possibly, let us make this a common helper API. concat:file01.bin|[start=96256]subfile:file02.bin Anyway, I think I can push the series except for the last patch now. Any objection? Regards,
Nicolas George: > Andreas Rheinhardt (12021-08-14): >> I do not really get that: The option passing syntax is restricted to the >> subfile protocol, so I don't know why we allow it for the file protocol >> in dashdec and hls. Unless I am missing something, this could (and >> should) be removed at once. > > It must be leftover code from when it was supported for all protocols. > Not requiring this kind of convoluted tests in new code is also a > motivation for just removing the last trace of this feature. > >> I use it to concatenate parts of files: When a DVR of mine has to split >> files due to the 4GB FAT-32 boundary, it does not do so cleanly; for >> some reason the first 96256B of the second file are duplicated, i.e. >> they coincide with bytes 96256-192511. For the third file, about a MB >> starting from offset 96256 is duplicated. > > Makes sense. > > I think we should decide that protocols like concat and subfile, > protocols where part(s) of the pseudo-URI are themselves pseudo-URI with > other protocols (let us call them metaprotocols) should always involve a > solution to pass options to the sub-protocols. > > Possibly, let us make this a common helper API. > Yes. If you have a better system for this than the current one, then I am all ears. > concat:file01.bin|[start=96256]subfile:file02.bin > > > Anyway, I think I can push the series except for the last patch now. > Any objection? > Not from me. - Andreas
diff --git a/libavformat/avio.c b/libavformat/avio.c index 4846bbd8c6..1ce290737a 100644 --- a/libavformat/avio.c +++ b/libavformat/avio.c @@ -111,39 +111,8 @@ static int url_alloc_for_protocol(URLContext **puc, const URLProtocol *up, goto fail; } if (up->priv_data_class) { - char *start; *(const AVClass **)uc->priv_data = up->priv_data_class; av_opt_set_defaults(uc->priv_data); - if (av_strstart(uc->filename, up->name, (const char**)&start) && *start == ',') { - int ret= 0; - char *p= start; - char sep= *++p; - char *key, *val; - p++; - - if (strcmp(up->name, "subfile")) - ret = AVERROR(EINVAL); - - while(ret >= 0 && (key= strchr(p, sep)) && p<key && (val = strchr(key+1, sep))){ - *val= *key= 0; - if (strcmp(p, "start") && strcmp(p, "end")) { - ret = AVERROR_OPTION_NOT_FOUND; - } else - ret= av_opt_set(uc->priv_data, p, key+1, 0); - if (ret == AVERROR_OPTION_NOT_FOUND) - av_log(uc, AV_LOG_ERROR, "Key '%s' not found.\n", p); - *val= *key= sep; - p= val+1; - } - if(ret<0 || p!=key){ - av_log(uc, AV_LOG_ERROR, "Error parsing options string %s\n", start); - av_freep(&uc->priv_data); - av_freep(&uc); - err = AVERROR(EINVAL); - goto fail; - } - memmove(start, key+1, strlen(key)); - } } } if (int_cb) @@ -255,8 +224,7 @@ static const struct URLProtocol *url_find_protocol(const char *filename) size_t proto_len = strspn(filename, URL_SCHEME_CHARS); int i; - if (filename[proto_len] != ':' && - (strncmp(filename, "subfile,", 8) || !strchr(filename + proto_len + 1, ':')) || + if (filename[proto_len] != ':' || is_dos_path(filename)) strcpy(proto_str, "file"); else diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c index 11966f905c..1d186802b3 100644 --- a/libavformat/dashdec.c +++ b/libavformat/dashdec.c @@ -435,7 +435,7 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url, ; else if (av_strstart(url, "crypto", NULL) && !strncmp(proto_name, url + 7, strlen(proto_name)) && url[7 + strlen(proto_name)] == ':') ; - else if (strcmp(proto_name, "file") || !strncmp(url, "file,", 5)) + else if (strcmp(proto_name, "file")) return AVERROR_INVALIDDATA; av_freep(pb); diff --git a/libavformat/hls.c b/libavformat/hls.c index 3c1b80f60c..e4d24b6fe0 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -667,7 +667,7 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url, ; else if (av_strstart(url, "data", NULL) && !strncmp(proto_name, url + 5, strlen(proto_name)) && url[5 + strlen(proto_name)] == ':') ; - else if (strcmp(proto_name, "file") || !strncmp(url, "file,", 5)) + else if (strcmp(proto_name, "file")) return AVERROR_INVALIDDATA; av_dict_copy(&tmp, *opts, 0);
It was only still supported for subfile and only used by dvd2concat. Signed-off-by: Nicolas George <george@nsup.org> --- libavformat/avio.c | 34 +--------------------------------- libavformat/dashdec.c | 2 +- libavformat/hls.c | 2 +- 3 files changed, 3 insertions(+), 35 deletions(-)