| Message ID | 1473006263-16027-1-git-send-email-stefasab@gmail.com |
|---|---|
| State | New |
| Headers | show |
Le nonidi 19 fructidor, an CCXXIV, Stefano Sabatini a écrit : > This is meant to be used for generating output suitable for the > ffprobe_default demuxer. > --- > ffprobe.c | 30 ++++++++++++++++++++++++++---- > 1 file changed, 26 insertions(+), 4 deletions(-) I find the naming a bit clumsy. It says what it says, but what it the next step? -show_format_after_frames_but_before_streams? I would like to suggest another avenue, a little more work but not by much: deprecate all the -show_<section> options, and replace them by a single -show option that takes the sections name in order: ffprobe -show packets,streams,format would be equivalent to the current "ffprobe -show_frames -show_streams -show_format", but: ffprobe -show format,streams,packets would yield the same result as with -show_headers_first. The syntax could later be updated for various extra features: ffprobe -show format,streams,packets,rewind,packets+frames=silent=1,streams I do not know how it interacts with the -show_entries feature, but I must say I never understood how the -show_entries feature works. Regards,
> On Sep 4, 2016, at 1:09 PM, Nicolas George <george@nsup.org> wrote: > > Le nonidi 19 fructidor, an CCXXIV, Stefano Sabatini a écrit : >> This is meant to be used for generating output suitable for the >> ffprobe_default demuxer. >> --- >> ffprobe.c | 30 ++++++++++++++++++++++++++---- >> 1 file changed, 26 insertions(+), 4 deletions(-) > > I find the naming a bit clumsy. It says what it says, but what it the next > step? -show_format_after_frames_but_before_streams? > > I would like to suggest another avenue, a little more work but not by much: > deprecate all the -show_<section> options, and replace them by a single > -show option that takes the sections name in order: > > ffprobe -show packets,streams,format > > would be equivalent to the current "ffprobe -show_frames -show_streams > -show_format", but: > > ffprobe -show format,streams,packets > > would yield the same result as with -show_headers_first. > > The syntax could later be updated for various extra features: > > ffprobe -show format,streams,packets,rewind,packets+frames=silent=1,streams > > I do not know how it interacts with the -show_entries feature, but I must > say I never understood how the -show_entries feature works. I’m not clear as to the reasoning for this, as the ordering of the elements doesn’t effect any of the meaing of the xml. However I wanted to note that such reordering would create invalid ffprobe xml. ffprobe.xsd defines the elements in a `sequence`, so if the order of elements is really arbitrary it should be changed to a `choice`. Dave Rice
Le nonidi 19 fructidor, an CCXXIV, Dave Rice a écrit : > I’m not clear as to the reasoning for this, as the ordering of the > elements doesn’t effect any of the meaing of the xml. However I wanted to > note that such reordering would create invalid ffprobe xml. ffprobe.xsd > defines the elements in a `sequence`, so if the order of elements is > really arbitrary it should be changed to a `choice`. Then we change the schema at the same time. I do not see a problem. Regards,
On date Sunday 2016-09-04 19:09:32 +0200, Nicolas George encoded: > Le nonidi 19 fructidor, an CCXXIV, Stefano Sabatini a écrit : > > This is meant to be used for generating output suitable for the > > ffprobe_default demuxer. > > --- > > ffprobe.c | 30 ++++++++++++++++++++++++++---- > > 1 file changed, 26 insertions(+), 4 deletions(-) > > I find the naming a bit clumsy. It says what it says, but what it the next > step? -show_format_after_frames_but_before_streams? > > I would like to suggest another avenue, a little more work but not by much: > deprecate all the -show_<section> options, and replace them by a single > -show option that takes the sections name in order: > > ffprobe -show packets,streams,format > > would be equivalent to the current "ffprobe -show_frames -show_streams > -show_format", but: > > ffprobe -show format,streams,packets We have the -show_entries option, and we can extend it to consider order. The equivalent syntax will then be: ffprobe -show_entries format,streams,packets > > would yield the same result as with -show_headers_first. > > The syntax could later be updated for various extra features: > > ffprobe -show format,streams,packets,rewind,packets+frames=silent=1,streams Now the problem with this idea is the interaction with -read_packets. The option was useful to force reading of packets so that the gained information was also available in the streams and format sections (but without showing the packets output). I can think about extending the -show_entries syntax, so that we could have: -show_entries !packets,format,streams What I don't like is that this special syntax will only work with packets (it makes no sense for the other sections). [...]
Le tridi 23 fructidor, an CCXXIV, Stefano Sabatini a écrit : > We have the -show_entries option, and we can extend it to consider > order. The equivalent syntax will then be: > > ffprobe -show_entries format,streams,packets As I said, I never really understood what -show_entries does. I had remembered it was for enabling/disabling fields within sections, not enabling/disabling sections. I suppose the reasoning is that it does not make sense to enable a field in a section if the section is not printed. Still, the result is that the same effect can be achieved by several different means, with slightly different contours, and I think this is not good for anybody: users are confused about the "right" way of doing it, maintainers have a higher risk of breaking something. I think it may be time to put aside all the options that were added to implement features incrementally in a backward-compatible way, and rebuild a brand new syntax for all the features at once. > > ffprobe -show format,streams,packets,rewind,packets+frames=silent=1,streams > Now the problem with this idea is the interaction with > -read_packets. The option was useful to force reading of packets so > that the gained information was also available in the streams and > format sections (but without showing the packets output). > > I can think about extending the -show_entries syntax, so that we could > have: > > -show_entries !packets,format,streams > > What I don't like is that this special syntax will only work with > packets (it makes no sense for the other sections). This issue of reading packets to extract information but not clutter the output with them is the reason I suggested in my example "packets+frames=silent=1". Still, the inconsistent syntax does not bother me either way: I think users can understand that a feature (doing the probe but suppressing the output) only works for a particular section, but they can also understand that the same feature works for all sections but is only useful for one. Therefore, we do whichever is simpler to implement and just make sure to explain it in the doc. There is another, more confusing case, where "print sections in order" can not work: packets and frames must be probed and printed together. I think it would help to thinks of the steps not as sections to print but as tasks to perform. Most tasks are just printing a section, but the task about packets has three optional subtasks: printing the packets, decoding the frames, printing the frames (of course, printing the frames without decoding makes no sense). If we design things to allow tasks to have aliases ("f" for "formats") and allow aliases to have different defaults for options, things can work in a way that is both convenient and logical. Consider a single task, "packets_and_frames", with three options, "print_packets=[01]", "decode_frames=[01]", "print_frames=[01]", and aliases: "packets" causing the options to default to 1/0/0, "frames" to 0/1/1; any other combination can be achieved by setting the options explicitly. The enabling or disabling of individual fields can also be an option to tasks. That allow to support the same features as -show_entries in a more logical way. Regards,
On date Saturday 2016-09-10 13:51:34 +0200, Nicolas George encoded: > Le tridi 23 fructidor, an CCXXIV, Stefano Sabatini a écrit : > > We have the -show_entries option, and we can extend it to consider > > order. The equivalent syntax will then be: > > > > ffprobe -show_entries format,streams,packets > > As I said, I never really understood what -show_entries does. I had > remembered it was for enabling/disabling fields within sections, not > enabling/disabling sections. I suppose the reasoning is that it does not > make sense to enable a field in a section if the section is not printed. -show_entries is the generalization of the various -show_* options, with the added bonus that it make it possible to specify which sections and fields to show (and possibly ignore all the other ones). > > Still, the result is that the same effect can be achieved by several > different means, with slightly different contours, and I think this is not > good for anybody: users are confused about the "right" way of doing it, > maintainers have a higher risk of breaking something. > > I think it may be time to put aside all the options that were added to > implement features incrementally in a backward-compatible way, and rebuild a > brand new syntax for all the features at once. I see your point, but while -show_entries has its warts, I don't think it really complicates the syntax, since there is a straight mapping from the legacy -show_X options to -show_entries X. > > > ffprobe -show format,streams,packets,rewind,packets+frames=silent=1,streams > > Now the problem with this idea is the interaction with > > -read_packets. The option was useful to force reading of packets so > > that the gained information was also available in the streams and > > format sections (but without showing the packets output). > > > > I can think about extending the -show_entries syntax, so that we could > > have: > > > > -show_entries !packets,format,streams > > > > What I don't like is that this special syntax will only work with > > packets (it makes no sense for the other sections). > > This issue of reading packets to extract information but not clutter the > output with them is the reason I suggested in my example > "packets+frames=silent=1". > > Still, the inconsistent syntax does not bother me either way: I think users > can understand that a feature (doing the probe but suppressing the output) > only works for a particular section, but they can also understand that the > same feature works for all sections but is only useful for one. Therefore, > we do whichever is simpler to implement and just make sure to explain it in > the doc. > > There is another, more confusing case, where "print sections in order" can > not work: packets and frames must be probed and printed together. > > I think it would help to think of the steps not as sections to print but as > tasks to perform. Most tasks are just printing a section, but the task about > packets has three optional subtasks: printing the packets, decoding the > frames, printing the frames (of course, printing the frames without decoding > makes no sense). > > If we design things to allow tasks to have aliases ("f" for "formats") and > allow aliases to have different defaults for options, things can work in a > way that is both convenient and logical. Consider a single task, > "packets_and_frames", with three options, "print_packets=[01]", > "decode_frames=[01]", "print_frames=[01]", and aliases: "packets" causing > the options to default to 1/0/0, "frames" to 0/1/1; any other combination > can be achieved by setting the options explicitly. At the moment we could simplify the logic, and assume a single section named packets_and_frames (this can be probably be done with small effort): I'm working on the code these days and many difficulties stem from the fact that the data structure describing the sections is not a tree. > The enabling or disabling of individual fields can also be an option to > tasks. That allow to support the same features as -show_entries in a more > logical way. In other words you're suggesting to move to an option systems and drop/deprecate the current system based on marking what needs to be shown (which is easy as long as you have a fixed order). But even assuming an option-based syntax specifying the operations you still have problems with the order of operations. In the specific case of the subject patch, I will need to specify the order of sections to print, and the options system (at least the one implemented in ffmpeg) is order-independent, so: show_format,show_streams,show_packets will have the same effect of: show_packets,show_format,show_streams [...]
On date Sunday 2016-09-11 19:42:45 +0200, Stefano Sabatini encoded: > On date Saturday 2016-09-10 13:51:34 +0200, Nicolas George encoded: [...] > > The enabling or disabling of individual fields can also be an option to > > tasks. That allow to support the same features as -show_entries in a more > > logical way. > > In other words you're suggesting to move to an option systems and > drop/deprecate the current system based on marking what needs to be > shown (which is easy as long as you have a fixed order). But even > assuming an option-based syntax specifying the operations you still > have problems with the order of operations. I assumed the case we have several options, in case we have a single option taking the operations specification this is no more an issue.
On date Saturday 2016-09-10 13:51:34 +0200, Nicolas George encoded: > Le tridi 23 fructidor, an CCXXIV, Stefano Sabatini a écrit : [...] > This issue of reading packets to extract information but not clutter the > output with them is the reason I suggested in my example > "packets+frames=silent=1". > > Still, the inconsistent syntax does not bother me either way: I think users > can understand that a feature (doing the probe but suppressing the output) > only works for a particular section, but they can also understand that the > same feature works for all sections but is only useful for one. Therefore, > we do whichever is simpler to implement and just make sure to explain it in > the doc. > > There is another, more confusing case, where "print sections in order" can > not work: packets and frames must be probed and printed together. > > I think it would help to thinks of the steps not as sections to print but as > tasks to perform. Most tasks are just printing a section, but the task about > packets has three optional subtasks: printing the packets, decoding the > frames, printing the frames (of course, printing the frames without decoding > makes no sense). > > If we design things to allow tasks to have aliases ("f" for "formats") and > allow aliases to have different defaults for options, things can work in a > way that is both convenient and logical. Consider a single task, > "packets_and_frames", with three options, "print_packets=[01]", > "decode_frames=[01]", "print_frames=[01]", and aliases: "packets" causing > the options to default to 1/0/0, "frames" to 0/1/1; any other combination > can be achieved by setting the options explicitly. > > The enabling or disabling of individual fields can also be an option to > tasks. That allow to support the same features as -show_entries in a more > logical way. So I thought more about this idea of a sort -dotasks option. The problem with sorting the output of my proposed approach fights with -read_packets. Indeed if we enable output ordering, still we don't know when the program is supposed to read the packets (unless we extend the syntax with something as !packets,format). An hypotetical -dotasks option may solve the problem as this: -dotasks format,packets=silent,streams What if you want only to print some specific entries? We can use it in combination with -show_entries, for example: -dotasks packets=silent,streams,format -show_entries format=format_name The problem with this approach is that it will conflict with -show_entries, for example in: -dotasks packets=silent,format -show_entries streams,packets Unless you're suggesting to entirely dig the -show_entries option. Otherwise we would need to figure out how to make the two options coexist.
diff --git a/ffprobe.c b/ffprobe.c index 657867d..42a8d8e 100644 --- a/ffprobe.c +++ b/ffprobe.c @@ -98,6 +98,7 @@ static int use_value_prefix = 0; static int use_byte_value_binary_prefix = 0; static int use_value_sexagesimal_format = 0; static int show_private_data = 1; +static int show_headers_first = 0; static char *print_format; static char *stream_specifier; @@ -2683,6 +2684,26 @@ static int probe_file(WriterContext *wctx, const char *filename) } } + if (show_headers_first && do_show_format) { + ret = show_format(wctx, &ifile); + CHECK_END; + } + + if (show_headers_first && do_show_chapters) { + ret = show_chapters(wctx, &ifile); + CHECK_END; + } + + if (show_headers_first && do_show_streams) { + ret = show_streams(wctx, &ifile); + CHECK_END; + } + + if (show_headers_first && do_show_programs) { + ret = show_programs(wctx, &ifile); + CHECK_END; + } + if (do_read_frames || do_read_packets) { if (do_show_frames && do_show_packets && wctx->writer->flags & WRITER_FLAG_PUT_PACKETS_AND_FRAMES_IN_SAME_CHAPTER) @@ -2699,20 +2720,20 @@ static int probe_file(WriterContext *wctx, const char *filename) CHECK_END; } - if (do_show_programs) { + if (!show_headers_first && do_show_programs) { ret = show_programs(wctx, &ifile); CHECK_END; } - if (do_show_streams) { + if (!show_headers_first && do_show_streams) { ret = show_streams(wctx, &ifile); CHECK_END; } - if (do_show_chapters) { + if (!show_headers_first && do_show_chapters) { ret = show_chapters(wctx, &ifile); CHECK_END; } - if (do_show_format) { + if (!show_headers_first && do_show_format) { ret = show_format(wctx, &ifile); CHECK_END; } @@ -3213,6 +3234,7 @@ static const OptionDef real_options[] = { { "read_intervals", HAS_ARG, {.func_arg = opt_read_intervals}, "set read intervals", "read_intervals" }, { "default", HAS_ARG | OPT_AUDIO | OPT_VIDEO | OPT_EXPERT, {.func_arg = opt_default}, "generic catch all option", "" }, { "i", HAS_ARG, {.func_arg = opt_input_file_i}, "read specified file", "input_file"}, + { "show_headers_first", OPT_BOOL, {&show_headers_first}, "show headers before the packets/frames" }, { NULL, }, };