| Message ID | CAB0OVGpH+bMTN5f=HmCyOPibd7Q=_FdAXb98=k4rRc+v8JHzNg@mail.gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Sat, Feb 9, 2019 at 2:58 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > Hi! > > Currently if the "loop" option gets specified for -f image2pipe, there is > no indication for the user that the option will not work, patch attached. > > Please comment, Carl Eugen Seems like a good idea. The reason for the pipe version's AVOption structure to be outside of the if is the IMAGEAUTO_DEMUXER macro, so I guess unless we like really long #ifs then this is as good as it gets. Quickly checked and it seems like generally the separation of options seems correct... but I'm not sure about "ts_from_file" . The filename variable will probably be full of \0s at the point where stat() is called, as it's only set to something else under "!s->is_pipe" branch in the beginning of the function if I'm seeing correctly - thus making this option effectively unusable under the _pipe demuxers? Jan
2019-02-10 1:55 GMT+01:00, Jan Ekström <jeebjp@gmail.com>: > On Sat, Feb 9, 2019 at 2:58 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> >> Hi! >> >> Currently if the "loop" option gets specified for -f image2pipe, there is >> no indication for the user that the option will not work, patch attached. >> >> Please comment, Carl Eugen > > Seems like a good idea. > > The reason for the pipe version's AVOption structure to be outside of > the if is the IMAGEAUTO_DEMUXER macro, so I guess unless we like > really long #ifs then this is as good as it gets. An alternative is to move the options into the IMAGEAUTO_DEMUXER macro but I did not see an advantage. > Quickly checked and it seems like generally the separation of options > seems correct... but I'm not sure about "ts_from_file" . The filename > variable will probably be full of \0s at the point where stat() is > called, as it's only set to something else under "!s->is_pipe" branch > in the beginning of the function if I'm seeing correctly - thus making > this option effectively unusable under the _pipe demuxers? Definitely, I left ts_from_file where it was and pushed. Thank you, Carl Eugen
On Sat, Feb 09, 2019 at 01:58:06PM +0100, Carl Eugen Hoyos wrote: > Hi! > > Currently if the "loop" option gets specified for -f image2pipe, there is > no indication for the user that the option will not work, patch attached. > > Please comment, Carl Eugen > img2dec.c | 33 ++++++++++++++++++++------------- > 1 file changed, 20 insertions(+), 13 deletions(-) > eb41db12f922fc0780a808df6d8f832620ef9d65 0001-lavf-img2dec-Split-img2-and-img2pipe-options.patch > From 21b11da9af5166e4bda7398a471a1e1a49352ac3 Mon Sep 17 00:00:00 2001 > From: Carl Eugen Hoyos <ceffmpeg@gmail.com> > Date: Sat, 9 Feb 2019 13:49:51 +0100 > Subject: [PATCH] lavf/img2dec: Split img2 and img2pipe options. > > --- > libavformat/img2dec.c | 33 ++++++++++++++++++++------------- > 1 file changed, 20 insertions(+), 13 deletions(-) this breaks -loop ./ffmpeg -loop 1 -i ~/fatesamples/png1/lena-rgb24.png -t 1 test.avi [...]
2019-02-10 18:44 GMT+01:00, Michael Niedermayer <michaelni@gmx.at>: > On Sat, Feb 09, 2019 at 01:58:06PM +0100, Carl Eugen Hoyos wrote: >> Hi! >> >> Currently if the "loop" option gets specified for -f image2pipe, there is >> no indication for the user that the option will not work, patch attached. >> >> Please comment, Carl Eugen > >> img2dec.c | 33 ++++++++++++++++++++------------- >> 1 file changed, 20 insertions(+), 13 deletions(-) >> eb41db12f922fc0780a808df6d8f832620ef9d65 >> 0001-lavf-img2dec-Split-img2-and-img2pipe-options.patch >> From 21b11da9af5166e4bda7398a471a1e1a49352ac3 Mon Sep 17 00:00:00 2001 >> From: Carl Eugen Hoyos <ceffmpeg@gmail.com> >> Date: Sat, 9 Feb 2019 13:49:51 +0100 >> Subject: [PATCH] lavf/img2dec: Split img2 and img2pipe options. >> >> --- >> libavformat/img2dec.c | 33 ++++++++++++++++++++------------- >> 1 file changed, 20 insertions(+), 13 deletions(-) > > this breaks -loop > > ./ffmpeg -loop 1 -i ~/fatesamples/png1/lena-rgb24.png -t 1 test.avi I moved the option that is obviously needed for the auto-detecting demuxers. The original issue was that the image2pipe demuxer happily accepts "loop" but ignores it - confusing users, I wonder if the only solution is to split the options between image2pipe and the auto-detecting demuxers or if there is another solution (or if both patches should be reverted). Sorry, Carl Eugen
On 10-02-2019 11:31 PM, Carl Eugen Hoyos wrote: > 2019-02-10 18:44 GMT+01:00, Michael Niedermayer <michaelni@gmx.at>: >> On Sat, Feb 09, 2019 at 01:58:06PM +0100, Carl Eugen Hoyos wrote: >>> Hi! >>> >>> Currently if the "loop" option gets specified for -f image2pipe, there is >>> no indication for the user that the option will not work, patch attached. >>> >>> Please comment, Carl Eugen >>> img2dec.c | 33 ++++++++++++++++++++------------- >>> 1 file changed, 20 insertions(+), 13 deletions(-) >>> eb41db12f922fc0780a808df6d8f832620ef9d65 >>> 0001-lavf-img2dec-Split-img2-and-img2pipe-options.patch >>> From 21b11da9af5166e4bda7398a471a1e1a49352ac3 Mon Sep 17 00:00:00 2001 >>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com> >>> Date: Sat, 9 Feb 2019 13:49:51 +0100 >>> Subject: [PATCH] lavf/img2dec: Split img2 and img2pipe options. >>> >>> --- >>> libavformat/img2dec.c | 33 ++++++++++++++++++++------------- >>> 1 file changed, 20 insertions(+), 13 deletions(-) >> this breaks -loop >> >> ./ffmpeg -loop 1 -i ~/fatesamples/png1/lena-rgb24.png -t 1 test.avi > I moved the option that is obviously needed for the auto-detecting demuxers. > > The original issue was that the image2pipe demuxer happily accepts > "loop" but ignores it - confusing users, I suggest to keep ignoring it and print a warning for is_pipe && loop in read_header. Gyan
From 21b11da9af5166e4bda7398a471a1e1a49352ac3 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Sat, 9 Feb 2019 13:49:51 +0100 Subject: [PATCH] lavf/img2dec: Split img2 and img2pipe options. --- libavformat/img2dec.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c index e82b1df..c1568c1 100644 --- a/libavformat/img2dec.c +++ b/libavformat/img2dec.c @@ -564,29 +564,29 @@ static int img_read_seek(AVFormatContext *s, int stream_index, int64_t timestamp #define OFFSET(x) offsetof(VideoDemuxData, x) #define DEC AV_OPT_FLAG_DECODING_PARAM +#define COMMON_OPTIONS \ + { "framerate", "set the video framerate", OFFSET(framerate), AV_OPT_TYPE_VIDEO_RATE, {.str = "25"}, 0, INT_MAX, DEC }, \ + { "pixel_format", "set video pixel format", OFFSET(pixel_format), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, \ + { "video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, {.str = NULL}, 0, 0, DEC }, \ + { "ts_from_file", "set frame timestamp from file's one", OFFSET(ts_from_file), AV_OPT_TYPE_INT, {.i64 = 0}, 0, 2, DEC, "ts_type" }, \ + { "none", "none", 0, AV_OPT_TYPE_CONST, {.i64 = 0 }, 0, 2, DEC, "ts_type" }, \ + { "sec", "second precision", 0, AV_OPT_TYPE_CONST, {.i64 = 1 }, 0, 2, DEC, "ts_type" }, \ + { "ns", "nano second precision", 0, AV_OPT_TYPE_CONST, {.i64 = 2 }, 0, 2, DEC, "ts_type" }, + +#if CONFIG_IMAGE2_DEMUXER const AVOption ff_img_options[] = { - { "framerate", "set the video framerate", OFFSET(framerate), AV_OPT_TYPE_VIDEO_RATE, {.str = "25"}, 0, INT_MAX, DEC }, + COMMON_OPTIONS { "loop", "force loop over input file sequence", OFFSET(loop), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, DEC }, - { "pattern_type", "set pattern type", OFFSET(pattern_type), AV_OPT_TYPE_INT, {.i64=PT_DEFAULT}, 0, INT_MAX, DEC, "pattern_type"}, { "glob_sequence","select glob/sequence pattern type", 0, AV_OPT_TYPE_CONST, {.i64=PT_GLOB_SEQUENCE}, INT_MIN, INT_MAX, DEC, "pattern_type" }, { "glob", "select glob pattern type", 0, AV_OPT_TYPE_CONST, {.i64=PT_GLOB }, INT_MIN, INT_MAX, DEC, "pattern_type" }, { "sequence", "select sequence pattern type", 0, AV_OPT_TYPE_CONST, {.i64=PT_SEQUENCE }, INT_MIN, INT_MAX, DEC, "pattern_type" }, { "none", "disable pattern matching", 0, AV_OPT_TYPE_CONST, {.i64=PT_NONE }, INT_MIN, INT_MAX, DEC, "pattern_type" }, - - { "pixel_format", "set video pixel format", OFFSET(pixel_format), AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, DEC }, { "start_number", "set first number in the sequence", OFFSET(start_number), AV_OPT_TYPE_INT, {.i64 = 0 }, INT_MIN, INT_MAX, DEC }, { "start_number_range", "set range for looking at the first sequence number", OFFSET(start_number_range), AV_OPT_TYPE_INT, {.i64 = 5}, 1, INT_MAX, DEC }, - { "video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, {.str = NULL}, 0, 0, DEC }, - { "frame_size", "force frame size in bytes", OFFSET(frame_size), AV_OPT_TYPE_INT, {.i64 = 0 }, 0, INT_MAX, DEC }, - { "ts_from_file", "set frame timestamp from file's one", OFFSET(ts_from_file), AV_OPT_TYPE_INT, {.i64 = 0 }, 0, 2, DEC, "ts_type" }, - { "none", "none", 0, AV_OPT_TYPE_CONST, {.i64 = 0 }, 0, 2, DEC, "ts_type" }, - { "sec", "second precision", 0, AV_OPT_TYPE_CONST, {.i64 = 1 }, 0, 2, DEC, "ts_type" }, - { "ns", "nano second precision", 0, AV_OPT_TYPE_CONST, {.i64 = 2 }, 0, 2, DEC, "ts_type" }, { NULL }, }; -#if CONFIG_IMAGE2_DEMUXER static const AVClass img2_class = { .class_name = "image2 demuxer", .item_name = av_default_item_name, @@ -606,11 +606,18 @@ AVInputFormat ff_image2_demuxer = { .priv_class = &img2_class, }; #endif + +const AVOption ff_img2pipe_options[] = { + COMMON_OPTIONS + { "frame_size", "force frame size in bytes", OFFSET(frame_size), AV_OPT_TYPE_INT, {.i64 = 0 }, 0, INT_MAX, DEC }, + { NULL }, +}; + #if CONFIG_IMAGE2PIPE_DEMUXER static const AVClass img2pipe_class = { .class_name = "image2pipe demuxer", .item_name = av_default_item_name, - .option = ff_img_options, + .option = ff_img2pipe_options, .version = LIBAVUTIL_VERSION_INT, }; AVInputFormat ff_image2pipe_demuxer = { @@ -1023,7 +1030,7 @@ static int gif_probe(AVProbeData *p) static const AVClass imgname ## _class = {\ .class_name = AV_STRINGIFY(imgname) " demuxer",\ .item_name = av_default_item_name,\ - .option = ff_img_options,\ + .option = ff_img2pipe_options,\ .version = LIBAVUTIL_VERSION_INT,\ };\ AVInputFormat ff_image_ ## imgname ## _pipe_demuxer = {\ -- 1.7.10.4