[FFmpeg-devel] [PATCH] ffprobe: add XML output

[Stefano Sabatini]

On date Monday 2011-10-17 10:15:22 +0200, Tomas Härdin encoded:
> On Wed, 2011-10-12 at 16:06 +0200, Stefano Sabatini wrote:
> > On date Sunday 2011-10-09 13:42:38 +0200, Stefano Sabatini encoded:
> > > ---
> > >  Makefile         |    2 +-
> > >  doc/ffprobe.texi |   23 +++++++
> > >  doc/ffprobe.xsd  |   90 ++++++++++++++++++++++++++
> 
> Extra brownie points for having a schema :)
> It seems to be missing an entry for sample_fmt:
> 
> $ xmllint --schema ffprobe.xsd ffprobe.xml
> ffprobe.xml:123: element stream: Schemas validity error : Element 'stream', attribute 'sample_fmt': The attribute 'sample_fmt' is not allowed.
> ffprobe.xml fails to validate

Yes, I added the field in a recent commit, updated locally.
  
> Schema feedback follows. Some of it may require code changes, such as preferring optional over "N/A":
> 
> > +    <xsd:element name="ffprobe" type="ffprobe:ffprobe"/>
> > +
> > +    <xsd:complexType name="ffprobe">
> 
> I'm not particularly fond of having a complexType and an element with
> the same name. It may cause problems with some tools I think.

Uhm, maybe I could change it to ffprobeType, need to check which is
the common convenction for dealing with that.

> 
> > +        <xsd:sequence>
> > +            <xsd:element name="packets"  type="ffprobe:packets" minOccurs="0" maxOccurs="1" />
> > +            <xsd:element name="streams"  type="ffprobe:streams" minOccurs="0" maxOccurs="1" />
> > +            <xsd:element name="format"   type="ffprobe:format"  minOccurs="0" maxOccurs="1" />
> > +        </xsd:sequence>
> 
> IMO just having
> 
>     <xsd:element name="packet"  type="ffprobe:packet" minOccurs="0" maxOccurs="unbounded" />
> 
> is enough (and similarly for the others). As in just using straight-up
> element arrays without wrapping them in an extra level of elements.
> Would require the code to be changed though, and I don't consider it
> that big of a problem.

Possible.

> > +    </xsd:complexType>
> > +
> > +    <xsd:complexType name="packets">
> > +        <xsd:sequence>
> > +            <xsd:element name="packet" type="ffprobe:packet" minOccurs="0" maxOccurs="unbounded"/>
> > +        </xsd:sequence>
> > +    </xsd:complexType>
> > +
> > +    <xsd:complexType name="packet">
> > +      <xsd:attribute name="codec_type"    type="xsd:string"  use="required" />
> 
> Isn't codec_type implied from <streams>?

Yes, but it's convenient to have this information duplicated here, for
avoiding to check the type of the corresponding stream.
 
> > +      <xsd:attribute name="stream_index"  type="xsd:integer" use="required" />
> 
> Let's make the Java people happy and use xsd:int instead of xsd:integer
> wherever possible - JAXB turns xsd:integer into BigInteger.

I'll do.
 
> > +      <xsd:attribute name="pts"           type="xsd:integer" use="required" />
> 
> Optional xsd:long? The user probably prefers unset timestamps over
> AV_NOPTS_VALUE.

That's something which needs some thought.

Some output formats may prefer to have each field specified
explicitely, even where there is no available value (e.g. in the
compact/CSV output, you don't want a variable number of fields per
section).

So I suppose we could make this a property of the writer
(force_missing_fields_display or an equivalent, don't have time to
find a shortest name now).

> 
> > +      <xsd:attribute name="pts_time"      type="xsd:decimal" use="required" />
> 
> xsd:decimal is non-scientific. The code seems to output in scientific
> notation for large number AFAICT, so xsd:float is appropriate here and
> elsewhere.
> Optional as well I suppose, instead of outputing "N/A".
> 
> > +      <xsd:attribute name="dts"           type="xsd:integer" use="required" />
> 
> Optional xsd:long
> 
> > +      <xsd:attribute name="dts_time"      type="xsd:decimal" use="required" />
> 
> Optional xsd:float
> 
> > +      <xsd:attribute name="duration"      type="xsd:integer" use="required" />
> 
> xsd:int. Maybe optional, but I think the user may understand 0 == unknown
> 
> > +      <xsd:attribute name="duration_time" type="xsd:decimal" use="required" />
> 
> xsd:float
> 
> > +      <xsd:attribute name="size"          type="xsd:decimal" use="required" />
> 
> xsd:int since xsd:decimal allows decimals
> 
> > +      <xsd:attribute name="pos"           type="xsd:integer" use="required" />
> 
> xsd:long
> 
> > +      <xsd:attribute name="flags"         type="xsd:string"  use="required" />
> > +    </xsd:complexType>
> > +
> > +    <xsd:complexType name="streams">
> > +        <xsd:sequence>
> > +            <xsd:element name="stream" type="ffprobe:stream" minOccurs="0" maxOccurs="unbounded"/>
> > +        </xsd:sequence>
> > +    </xsd:complexType>
> > +
> > +    <xsd:complexType name="stream">
> > +      <xsd:attribute name="index"            type="xsd:integer" use="required"/>
> 
> xsd:int
> 
> > +      <xsd:attribute name="codec_name"       type="xsd:string"  use="required"/>
> > +      <xsd:attribute name="codec_long_name"  type="xsd:string"  use="required"/>
> > +      <xsd:attribute name="codec_type"       type="xsd:string"  use="required"/>
> > +      <xsd:attribute name="codec_time_base"  type="xsd:string"  use="required"/>
> > +      <xsd:attribute name="codec_tag"        type="xsd:string"  use="required"/>
> > +      <xsd:attribute name="codec_tag_string" type="xsd:string"  use="required"/>
> > +
> > +      <!-- audio attributes -->
> > +      <xsd:attribute name="sample_rate"      type="xsd:string" />
> 
> Uhm, xsd:int? The example XML has sample_rate="8000.000000" though, for
> which xsd:float fits, but stinks.

Yes, this could be changed to a plain int (displayed as 8000).

I'll try to send an updated version soon.