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

[Stefano Sabatini]

On date Tuesday 2011-10-18 19:17:32 +0200, Stefano Sabatini encoded:
> 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.

Can you comment on advantages and disadvantages of this approach?

One possible advantage I can foresee is that in case of we add support
to frame printing, we won't have to put packets *and* frames in the
same container (which is not a big issue to say the truth, but still
it would be somehow ugly, while right now *is* a real problem with the
JSON output).

> 
> > > +    </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.

done

> > > +      <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).

Done.

> > 
> > > +      <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".

Changed to float, and made it optional.

> > 
> > > +      <xsd:attribute name="dts"           type="xsd:integer" use="required" />
> > 
> > Optional xsd:long

Fixed.

> > > +      <xsd:attribute name="dts_time"      type="xsd:decimal" use="required" />
> > 
> > Optional xsd:float

Changed as well.

> > 
> > > +      <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

changed to long, and made it optional.

> > 
> > > +      <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

Done.

> > > +      <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).

Now it's an int.
 
> I'll try to send an updated version soon.

Pending problems:
XSD validation only works with xml=q=1, and no special formatting
options selected (e.g. -pretty).

XSD validation also doesn't work in case the codec context contains
private options (check show_stream), not sure what's the best option
here from the UI point of view, alternatives are:

* make private options printing configurable, warn user that the XML
  output may not be validated against the XSD if enabled
* replace print_str() with print_str_opt() in the code which prints
  private options, so they won't be displayed in JSON/XML, so XML
  will validate but it will miss potentially useful information
* manually update the XSD anytime a new private field is added, or
  alternatively, add a script for doing it automatically, which looks
  quite overkill and/or brittle

Suggestions are welcome.
-- 
FFmpeg = Funny & Fanciful Mega Pure Enhancing Geisha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ffprobe-add-XML-writer.patch
Type: text/x-diff
Size: 14703 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111125/c083606d/attachment.bin>