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

[Pierre-Anthony Lemieux]

On Sun, Dec 12, 2021 at 7:58 PM Zane van Iperen <zane at zanevaniperen.com> wrote:
>
>
> 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?

Addressed by patchset v10.

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

We would first need to make sure that the string is exactly 45 chars.

On the output side, it looks like uuid_unparse() requires the caller
to allocate a 36-char buffer. Wouldn't that complicate calls like the
following:

        av_log(s, AV_LOG_DEBUG, "Found asset id: " FF_IMF_UUID_FORMAT
"\n", UID_ARG(asset->uuid));

In fact, I wonder why libuuid [1] doesn't offer a simple printf format
string, like FF_IMF_UUID_FORMAT? Maybe I missed it?

[1] https://github.com/util-linux/util-linux/blob/master/libuuid/src/uuid.h

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

The sscanf() in ff_imf_xml_read_uuid() would be replaced by a strlen()
followed by a memcpy(), and FFIMFUUID would be char[46] instead of
uint8_t[16]. I am not convinced the code would be more
maintainable/simpler.

I am wondering if FFMPEG shouldn't ultimately define a thin wrapper
over libuuid, like it does for memory and string functions?

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

Addressed by patchset v10.

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

Addressed by patchset v10.

>
> > +/**
> > + * 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_*().

Addressed by patchset v10.

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

Happy to look at refactoring the use of libxml after the IMF demuxer
is merged -- just trying to keep the patch surface small.

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

Probe function added in patchset v10.

>
> > +    .mime_type      = "application/xml,text/xml",
> > +};
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".