[FFmpeg-devel] ffprobe: add -show_headers_first option

Submitted by Stefano Sabatini on Sept. 4, 2016, 4:24 p.m.

Details

Message ID 1473006263-16027-1-git-send-email-stefasab@gmail.com
State New
Headers show

Commit Message

Stefano Sabatini Sept. 4, 2016, 4:24 p.m.
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(-)

Comments

Nicolas George Sept. 4, 2016, 5:09 p.m.
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,
Dave Rice Sept. 4, 2016, 6:35 p.m.
> 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
Nicolas George Sept. 4, 2016, 6:39 p.m.
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,
Stefano Sabatini Sept. 8, 2016, 11:25 a.m.
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).

[...]
Nicolas George Sept. 10, 2016, 11:51 a.m.
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,
Stefano Sabatini Sept. 11, 2016, 5:42 p.m.
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

[...]
Stefano Sabatini Sept. 12, 2016, 8:04 a.m.
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.
Stefano Sabatini Sept. 18, 2016, 5:21 p.m.
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.

Patch hide | download patch | download mbox

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, },
 };