[FFmpeg-devel] [PATCH v9 1/2] avformat/imf: Demuxer

[Zane van Iperen]

On 9/12/21 13:55, pal at sandflow.com wrote:

> +
> +#define FF_UUID_FORMAT                                \
> +    "urn:uuid:%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-" \
> +    "%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
> +
> +/**
> + * UUID as defined in IETF RFC 422
> + */
> +typedef uint8_t FFUUID[16];
> +

Perhaps change to FF_IMF_UUID_FORMAT and FFIMFUUID, unless you intend these
to be used for all of FFmpeg?

I also agree with Lynne that we shouldn't ad-hoc UUIDs. libuuid is nice
and wouldn't be too bad to add as a dependency. It'll also be handy if
some other part needs to handle UUIDs in the future.

Even though it might not support "urn:uuid:", you can just offset the pointer
before giving it to uuid_parse (or uuid_parse_range).

Alternatively, if all you need to do is compare them, could you not
just compare the raw strings, skipping the parsing entirely?

> +/**
> + * Parse an IMF CompositionPlaylist element into the FFIMFCPL data structure.
> + * @param[in] doc An XML document from which the CPL is read.
> + * @param[out] cpl Pointer to a memory area (allocated by the client), where the
> + *  function writes a pointer to the newly constructed FFIMFCPL structure (or
> + *  NULL if the CPL could not be parsed). The client is responsible for freeing
> + *  the FFIMFCPL structure using ff_imf_cpl_free().
> + * @return A non-zero value in case of an error.
> + */
> +int ff_parse_imf_cpl_from_xml_dom(xmlDocPtr doc, FFIMFCPL **cpl);
> +

ff_imf_parse_cpl_from_xml_dom(), ff_parse_* is taken.
Also for consistency with everything else.

> +/**
> + * Parse an IMF Composition Playlist document into the FFIMFCPL data structure.
> + * @param[in] in The context from which the CPL is read.
> + * @param[out] cpl Pointer to a memory area (allocated by the client), where the
> + * function writes a pointer to the newly constructed FFIMFCPL structure (or
> + * NULL if the CPL could not be parsed). The client is responsible for freeing
> + * the FFIMFCPL structure using ff_imf_cpl_free().
> + * @return A non-zero value in case of an error.
> + */
> +int ff_parse_imf_cpl(AVIOContext *in, FFIMFCPL **cpl);
> +

Ditto.

> +/**
> + * Reads an unsigned 32-bit integer from an XML element
> + * @return 0 on success, < 0 AVERROR code on error.
> + */
> +int ff_xml_read_uint32(xmlNodePtr element, uint32_t *number);
> +
> +/**
> + * Reads an AVRational from an XML element
> + * @return 0 on success, < 0 AVERROR code on error.
> + */
> +int ff_xml_read_rational(xmlNodePtr element, AVRational *rational);
> +
> +/**
> + * Reads a UUID from an XML element
> + * @return 0 on success, < 0 AVERROR code on error.
> + */
> +int ff_xml_read_uuid(xmlNodePtr element, uint8_t uuid[16]);
> +
> +/**
> + * Returns the first child element with the specified local name
> + * @return A pointer to the child element, or NULL if no such child element exists.
> + */
> +xmlNodePtr ff_xml_get_child_element_by_name(xmlNodePtr parent, const char *name_utf8);
> +

If these are only used for IMF, then ff_imf_xml_*().
Afaik FFmpeg doesn't have any centralised XML helpers (and perhaps it should, there's a few
things that use libxml from a quick grep).

> +
> +const AVInputFormat ff_imf_demuxer = {
> +    .name           = "imf",
> +    .long_name      = NULL_IF_CONFIG_SMALL("IMF (Interoperable Master Format)"),
> +    .flags_internal = FF_FMT_INIT_CLEANUP,
> +    .priv_class     = &imf_class,
> +    .priv_data_size = sizeof(IMFContext),
> +    .read_header    = imf_read_header,
> +    .read_packet    = imf_read_packet,
> +    .read_close     = imf_close,
> +    .extensions     = "xml",

I'm a bit apprehensive about this. This will unconditionally set all XML files as imf.
How difficult would it be to add a probe function?

> +    .mime_type      = "application/xml,text/xml",
> +};
>