[FFmpeg-devel,v7,1/2] avformat/imf: Demuxer

From: Pierre-Anthony Lemieux <pal@palemieux.com>

Signed-off-by: Pierre-Anthony Lemieux <pal@palemieux.com>
---

Notes:
    The IMF demuxer accepts as input an IMF CPL. The assets referenced by the CPL can be
    contained in multiple deliveries, each defined by an ASSETMAP file:
    
    ffmpeg -assetmaps <path of ASSETMAP1>,<path of ASSETMAP>,... -i <path of CPL>
    
    If -assetmaps is not specified, FFMPEG looks for a file called ASSETMAP.xml in the same directory as the CPL.
    
    EXAMPLE:
        ffmpeg -i http://ffmpeg-imf-samples-public.s3-website-us-west-1.amazonaws.com/countdown/CPL_f5095caa-f204-4e1c-8a84-7af48c7ae16b.xml out.mp4
    
    The Interoperable Master Format (IMF) is a file-based media format for the
    delivery and storage of professional audio-visual masters.
    An IMF Composition consists of an XML playlist (the Composition Playlist)
    and a collection of MXF files (the Track Files). The Composition Playlist (CPL)
    assembles the Track Files onto a timeline, which consists of multiple tracks.
    The location of the Track Files referenced by the Composition Playlist is stored
    in one or more XML documents called Asset Maps. More details at https://www.imfug.com/explainer.
    The IMF standard was first introduced in 2013 and is managed by the SMPTE.
    
    CHANGE NOTES:
    
    - reduced line width
    - use ff_ and FF prefixes for non-local functions and structures
    - modified copyright header
    - fixed rational initialization
    - removed extraneous call to xmlCleanupParser()
    - fix if/for single line braces
    - replace av_realloc_f with av_fast_realloc when allocating CPL Resources and
      ASSETMAP assets

 MAINTAINERS              |   1 +
 configure                |   3 +-
 doc/demuxers.texi        |  19 +
 libavformat/Makefile     |   1 +
 libavformat/allformats.c |   1 +
 libavformat/imf.h        | 206 +++++++++
 libavformat/imf_cpl.c    | 769 +++++++++++++++++++++++++++++++++
 libavformat/imfdec.c     | 907 +++++++++++++++++++++++++++++++++++++++
 8 files changed, 1906 insertions(+), 1 deletion(-)
 create mode 100644 libavformat/imf.h
 create mode 100644 libavformat/imf_cpl.c
 create mode 100644 libavformat/imfdec.c

6 Dec 2021, 04:18 by pal@sandflow.com:

> From: Pierre-Anthony Lemieux <pal@palemieux.com>
> +
> +/**
> + * Implements IMP CPL processing
> + *
> + * @author Pierre-Anthony Lemieux
> + * @file
> + * @ingroup lavu_imf
> + */
> +
> +#include "imf.h"
> +#include "libavformat/mxf.h"
> +#include "libavutil/bprint.h"
> +#include "libavutil/error.h"
> +#include <libxml/parser.h>
> +
> +xmlNodePtr ff_xml_get_child_element_by_name(xmlNodePtr parent, const char *name_utf8)
> +{
> +    xmlNodePtr cur_element;
> +
> +    cur_element = xmlFirstElementChild(parent);
> +    while (cur_element) {
> +        if (xmlStrcmp(cur_element->name, name_utf8) == 0)
> +            return cur_element;
> +        cur_element = xmlNextElementSibling(cur_element);
> +    }
> +    return NULL;
> +}
> +
> +int ff_xml_read_UUID(xmlNodePtr element, uint8_t uuid[16])
> +{
> +    xmlChar *element_text = NULL;
> +    int scanf_ret;
> +    int ret = 0;
> +
> +    element_text = xmlNodeListGetString(element->doc, element->xmlChildrenNode, 1);
> +    scanf_ret = sscanf(element_text,
> +        FF_UUID_FORMAT,
> +        &uuid[0],
> +        &uuid[1],
> +        &uuid[2],
> +        &uuid[3],
> +        &uuid[4],
> +        &uuid[5],
> +        &uuid[6],
> +        &uuid[7],
> +        &uuid[8],
> +        &uuid[9],
> +        &uuid[10],
> +        &uuid[11],
> +        &uuid[12],
> +        &uuid[13],
> +        &uuid[14],
> +        &uuid[15]);
> +    if (scanf_ret != 16) {
> +        av_log(NULL, AV_LOG_ERROR, "Invalid UUID\n");
> +        ret = AVERROR_INVALIDDATA;
> +    }
> +    xmlFree(element_text);
> +
> +    return ret;
> +}
>

Please don't NIH-parse UUIDs. They're more complicated
than this. Use libuuid, it's stable, bug-free and universally
available. I submitted a patch for it last month for an
unrelated use, which I decided to drop, but you
can see how it was done there.

Also, please follow the code style. We use snake_case
for function names, which means there are no capital
letters in them.
There are also still one or two one-line statements
left that are wrapped in brackets.

We check every single allocation we make, since malloc()
can fail on Windows. So that means all av_strdup calls,
av_append_path_component calls and so on can
return NULL, and result in a nasty crash.

Do not use strtok. It's not thread-safe at all. Use av_strtok,
which is thread-safe.

ff_imf_cpl_free() has a weird branch that can just be
done with a return if cpl is NULL.

There are places where you call av_free() and then
set the pointer to NULL. We have a convenience
function for this, av_freep().

Please use defined-length integers instead of "unsigned long".
If something needs to be 64-bits, use (u)int64_t. If something
doesn't need to be 64-bits, use uint32_t for unsigned, or plain
int for signed. We always require plain ints to be 32-bits.

Also, instead of printing 64-bit ints via "%lu", use "%"PRIu64"
and "%"PRIi64". MSVC has problems with %lu.

pal@sandflow.com:
> From: Pierre-Anthony Lemieux <pal@palemieux.com>
> 
> Signed-off-by: Pierre-Anthony Lemieux <pal@palemieux.com>
> ---
> 
> Notes:
>     The IMF demuxer accepts as input an IMF CPL. The assets referenced by the CPL can be
>     contained in multiple deliveries, each defined by an ASSETMAP file:
>     
>     ffmpeg -assetmaps <path of ASSETMAP1>,<path of ASSETMAP>,... -i <path of CPL>
>     
>     If -assetmaps is not specified, FFMPEG looks for a file called ASSETMAP.xml in the same directory as the CPL.
>     
>     EXAMPLE:
>         ffmpeg -i http://ffmpeg-imf-samples-public.s3-website-us-west-1.amazonaws.com/countdown/CPL_f5095caa-f204-4e1c-8a84-7af48c7ae16b.xml out.mp4
>     
>     The Interoperable Master Format (IMF) is a file-based media format for the
>     delivery and storage of professional audio-visual masters.
>     An IMF Composition consists of an XML playlist (the Composition Playlist)
>     and a collection of MXF files (the Track Files). The Composition Playlist (CPL)
>     assembles the Track Files onto a timeline, which consists of multiple tracks.
>     The location of the Track Files referenced by the Composition Playlist is stored
>     in one or more XML documents called Asset Maps. More details at https://www.imfug.com/explainer.
>     The IMF standard was first introduced in 2013 and is managed by the SMPTE.
>     
>     CHANGE NOTES:
>     
>     - reduced line width
>     - use ff_ and FF prefixes for non-local functions and structures
>     - modified copyright header
>     - fixed rational initialization
>     - removed extraneous call to xmlCleanupParser()
>     - fix if/for single line braces
>     - replace av_realloc_f with av_fast_realloc when allocating CPL Resources and
>       ASSETMAP assets
> 
>  MAINTAINERS              |   1 +
>  configure                |   3 +-
>  doc/demuxers.texi        |  19 +
>  libavformat/Makefile     |   1 +
>  libavformat/allformats.c |   1 +
>  libavformat/imf.h        | 206 +++++++++
>  libavformat/imf_cpl.c    | 769 +++++++++++++++++++++++++++++++++
>  libavformat/imfdec.c     | 907 +++++++++++++++++++++++++++++++++++++++
>  8 files changed, 1906 insertions(+), 1 deletion(-)
>  create mode 100644 libavformat/imf.h
>  create mode 100644 libavformat/imf_cpl.c
>  create mode 100644 libavformat/imfdec.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dcac46003e..7a6972fe1a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -433,6 +433,7 @@ Muxers/Demuxers:
>    idroqdec.c                            Mike Melanson
>    iff.c                                 Jaikrishnan Menon
>    img2*.c                               Michael Niedermayer
> +  imf*.c                                Marc-Antoine Arnaud, Pierre-Anthony Lemieux, Valentin Noël
>    ipmovie.c                             Mike Melanson
>    ircam*                                Paul B Mahol
>    iss.c                                 Stefan Gehrer
> diff --git a/configure b/configure
> index a98a18abaa..022f17767b 100755
> --- a/configure
> +++ b/configure
> @@ -298,7 +298,7 @@ External library support:
>    --enable-libxvid         enable Xvid encoding via xvidcore,
>                             native MPEG-4/Xvid encoder exists [no]
>    --enable-libxml2         enable XML parsing using the C library libxml2, needed
> -                           for dash demuxing support [no]
> +                           for dash and imf demuxing support [no]
>    --enable-libzimg         enable z.lib, needed for zscale filter [no]
>    --enable-libzmq          enable message passing via libzmq [no]
>    --enable-libzvbi         enable teletext support via libzvbi [no]
> @@ -3400,6 +3400,7 @@ hls_muxer_select="mpegts_muxer"
>  hls_muxer_suggest="gcrypt openssl"
>  image2_alias_pix_demuxer_select="image2_demuxer"
>  image2_brender_pix_demuxer_select="image2_demuxer"
> +imf_demuxer_deps="libxml2"
>  ipod_muxer_select="mov_muxer"
>  ismv_muxer_select="mov_muxer"
>  ivf_muxer_select="av1_metadata_bsf vp9_superframe_bsf"
> diff --git a/doc/demuxers.texi b/doc/demuxers.texi
> index cab8a7072c..614f2e754a 100644
> --- a/doc/demuxers.texi
> +++ b/doc/demuxers.texi
> @@ -267,6 +267,12 @@ which streams to actually receive.
>  Each stream mirrors the @code{id} and @code{bandwidth} properties from the
>  @code{<Representation>} as metadata keys named "id" and "variant_bitrate" respectively.
>  
> +@section imf
> +
> +Interoperable Master Format demuxer.
> +
> +This demuxer presents audio and video streams found in an IMF Composition.
> +
>  @section flv, live_flv, kux
>  
>  Adobe Flash Video Format demuxer.
> @@ -770,6 +776,19 @@ MJPEG stream. Turning this option on by setting it to 1 will result in a stricte
>  of the boundary value.
>  @end table
>  
> +@section mxf
> +
> +MXF demuxer.
> +
> +@table @option
> +
> +@item -eia608_extract @var{bool}
> +Extract EIA-608 captions from SMPTE 436M track
> +
> +@item -skip_audio_reordering @var{bool}
> +This option will disable the audio reordering based on Multi-Channel Audio (MCA) labelling (SMPTE ST-377-4).
> +@end table
> +

Why are you adding documentation for mxf in this patch?

>  @section rawvideo
>  
>  Raw video demuxer.
> diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c
> new file mode 100644
> index 0000000000..ec4ad28444
> --- /dev/null
> +++ b/libavformat/imfdec.c
> @@ -0,0 +1,907 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/*
> + *
> + * Copyright (c) Sandflow Consulting LLC
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + *
> + * * Redistributions of source code must retain the above copyright notice, this
> + *   list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright notice,
> + *   this list of conditions and the following disclaimer in the documentation
> + *   and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +/**
> + * Demuxes an IMF Composition
> + *
> + * References
> + * OV 2067-0:2018 - SMPTE Overview Document - Interoperable Master Format
> + * ST 2067-2:2020 - SMPTE Standard - Interoperable Master Format — Core Constraints
> + * ST 2067-3:2020 - SMPTE Standard - Interoperable Master Format — Composition Playlist
> + * ST 2067-5:2020 - SMPTE Standard - Interoperable Master Format — Essence Component
> + * ST 2067-20:2016 - SMPTE Standard - Interoperable Master Format — Application #2
> + * ST 2067-21:2020 - SMPTE Standard - Interoperable Master Format — Application #2 Extended
> + * ST 2067-102:2017 - SMPTE Standard - Interoperable Master Format — Common Image Pixel Color Schemes
> + * ST 429-9:2007 - SMPTE Standard - D-Cinema Packaging — Asset Mapping and File Segmentation
> + *
> + * @author Marc-Antoine Arnaud
> + * @author Valentin Noel
> + * @author Nicholas Vanderzwet
> + * @file
> + * @ingroup lavu_imf
> + */
> +
> +#include "imf.h"
> +#include "internal.h"
> +#include "libavutil/avstring.h"
> +#include "libavutil/bprint.h"
> +#include "libavutil/opt.h"
> +#include "mxf.h"
> +#include "url.h"
> +#include <inttypes.h>
> +#include <libxml/parser.h>
> +
> +#define MAX_BPRINT_READ_SIZE (UINT_MAX - 1)
> +#define DEFAULT_ASSETMAP_SIZE 8 * 1024
> +#define AVRATIONAL_FORMAT "%d/%d"
> +#define AVRATIONAL_ARG(rational) rational.num, rational.den
> +
> +/**
> + * IMF Asset locator
> + */
> +typedef struct IMFAssetLocator {
> +    FFUUID uuid;
> +    char *absolute_uri;
> +} IMFAssetLocator;
> +
> +/**
> + * IMF Asset locator map
> + * Results from the parsing of one or more ASSETMAP XML files
> + */
> +typedef struct IMFAssetLocatorMap {
> +    uint32_t asset_count;
> +    IMFAssetLocator *assets;
> +} IMFAssetLocatorMap;
> +
> +typedef struct IMFVirtualTrackResourcePlaybackCtx {
> +    IMFAssetLocator *locator;
> +    FFIMFTrackFileResource *resource;
> +    AVFormatContext *ctx;
> +} IMFVirtualTrackResourcePlaybackCtx;
> +
> +typedef struct IMFVirtualTrackPlaybackCtx {
> +    // Track index in playlist
> +    int32_t index;
> +    // Time counters
> +    AVRational current_timestamp;
> +    AVRational duration;
> +    // Resources
> +    unsigned int resource_count;
> +    unsigned int resources_alloc_sz;
> +    IMFVirtualTrackResourcePlaybackCtx *resources;
> +    // Decoding cursors
> +    uint32_t current_resource_index;
> +    int64_t last_pts;
> +} IMFVirtualTrackPlaybackCtx;
> +
> +typedef struct IMFContext {
> +    const AVClass *class;
> +    const char *base_url;
> +    char *asset_map_paths;
> +    AVIOInterruptCB *interrupt_callback;
> +    AVDictionary *avio_opts;
> +    FFIMFCPL *cpl;
> +    IMFAssetLocatorMap *asset_locator_map;
> +    unsigned int track_count;
> +    IMFVirtualTrackPlaybackCtx **tracks;
> +} IMFContext;
> +
> +static int imf_uri_is_url(const char *string)
> +{
> +    char *substr = strstr(string, "://");

Just because strstr() is not const-correct does not mean that you should
be, too. Use const char *substr.

> +    return substr != NULL;
> +}
> +
> +static int imf_uri_is_unix_abs_path(const char *string)
> +{
> +    char *substr = strstr(string, "/");

This is equivalent to strchr(string, '/').

> +    int index = (int)(substr - string);

This might be undefined behaviour string is long (pointer->integer
conversions are UB if the result can't be represented in the destination
type).

> +    return index == 0;

This makes the whole function equivalent to "return string[0] == '/';".

> +}
> +
> +static int imf_uri_is_dos_abs_path(const char *string)
> +{
> +    // Absolute path case: `C:\path\to\somwhere`
> +    char *substr = strstr(string, ":\\");
> +    int index = (int)(substr - string);
> +    if (index == 1)
> +        return 1;
> +
> +    // Absolute path case: `C:/path/to/somwhere`
> +    substr = strstr(string, ":/");
> +    index = (int)(substr - string);
> +    if (index == 1)
> +        return 1;
> +
> +    // Network path case: `\\path\to\somwhere`
> +    substr = strstr(string, "\\\\");
> +    index = (int)(substr - string);
> +    return index == 0;
> +}
> +
> +/**
> + * Parse a ASSETMAP XML file to extract the UUID-URI mapping of assets.
> + * @param s the current format context, if any (can be NULL).
> + * @param doc the XML document to be parsed.
> + * @param asset_map pointer on the IMFAssetLocatorMap to fill.
> + * @param base_url the url of the asset map XML file, if any (can be NULL).
> + * @return a negative value in case of error, 0 otherwise.
> + */
> +static int parse_imf_asset_map_from_xml_dom(AVFormatContext *s,
> +    xmlDocPtr doc,
> +    IMFAssetLocatorMap *asset_map,
> +    const char *base_url)
> +{
> +    xmlNodePtr asset_map_element = NULL;
> +    xmlNodePtr node = NULL;
> +    xmlNodePtr asset_element = NULL;
> +    char *uri;
> +    int ret = 0;
> +    IMFAssetLocator *asset = NULL;
> +
> +    asset_map_element = xmlDocGetRootElement(doc);
> +
> +    if (!asset_map_element) {
> +        av_log(s, AV_LOG_ERROR, "Unable to parse asset map XML - missing root node\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    if (asset_map_element->type != XML_ELEMENT_NODE || av_strcasecmp(asset_map_element->name, "AssetMap")) {
> +        av_log(s,
> +            AV_LOG_ERROR,
> +            "Unable to parse asset map XML - wrong root node name[%s] type[%d]\n",
> +            asset_map_element->name,
> +            (int)asset_map_element->type);
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    // parse asset locators
> +
> +    if (!(node = ff_xml_get_child_element_by_name(asset_map_element, "AssetList"))) {
> +        av_log(s, AV_LOG_ERROR, "Unable to parse asset map XML - missing AssetList node\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +    asset_map->assets = av_realloc_f(asset_map->assets,
> +        xmlChildElementCount(node),
> +        sizeof(IMFAssetLocator));

Leak in case of error: All the absolute_uri strings already contained in
the array will leak.

> +    if (!asset_map->assets) {
> +        av_log(NULL, AV_LOG_PANIC, "Cannot allocate IMF asset locators\n");
> +        return AVERROR(ENOMEM);
> +    }
> +    asset_map->asset_count = 0;

And this implicitly leaks the absolute_uri strings even on success (only
in case there were any such strings, i.e. if the loop in
imf_read_header() (that loops over parse_assetmap()) is actually a
loop). They will either be overwritten below or just not be freed at the
end.

> +    asset_element = xmlFirstElementChild(node);
> +    while (asset_element) {
> +        if (av_strcasecmp(asset_element->name, "Asset") != 0)
> +            continue;
> +
> +        asset = &(asset_map->assets[asset_map->asset_count]);
> +
> +        if (ff_xml_read_UUID(ff_xml_get_child_element_by_name(asset_element, "Id"), asset->uuid)) {
> +            av_log(s, AV_LOG_ERROR, "Could not parse UUID from asset in asset map.\n");
> +            return AVERROR_INVALIDDATA;
> +        }
> +
> +        av_log(s, AV_LOG_DEBUG, "Found asset id: " FF_UUID_FORMAT "\n", UID_ARG(asset->uuid));
> +
> +        if (!(node = ff_xml_get_child_element_by_name(asset_element, "ChunkList"))) {
> +            av_log(s, AV_LOG_ERROR, "Unable to parse asset map XML - missing ChunkList node\n");
> +            return AVERROR_INVALIDDATA;
> +        }
> +
> +        if (!(node = ff_xml_get_child_element_by_name(node, "Chunk"))) {
> +            av_log(s, AV_LOG_ERROR, "Unable to parse asset map XML - missing Chunk node\n");
> +            return AVERROR_INVALIDDATA;
> +        }
> +
> +        uri = xmlNodeGetContent(ff_xml_get_child_element_by_name(node, "Path"));
> +        if (!imf_uri_is_url(uri) && !imf_uri_is_unix_abs_path(uri) && !imf_uri_is_dos_abs_path(uri))
> +            asset->absolute_uri = av_append_path_component(base_url, uri);
> +        else
> +            asset->absolute_uri = av_strdup(uri);
> +        xmlFree(uri);
> +        if (!asset->absolute_uri) {
> +            av_log(NULL, AV_LOG_PANIC, "Cannot allocate asset locator absolute URI\n");
> +            return AVERROR(ENOMEM);
> +        }
> +
> +        av_log(s, AV_LOG_DEBUG, "Found asset absolute URI: %s\n", asset->absolute_uri);
> +
> +        asset_map->asset_count++;
> +        asset_element = xmlNextElementSibling(asset_element);
> +    }
> +
> +    return ret;
> +}
> +
> +/**
> + * Allocate a IMFAssetLocatorMap pointer and return it.
> + * @return the allocated IMFAssetLocatorMap pointer.
> + */
> +static IMFAssetLocatorMap *imf_asset_locator_map_alloc(void)
> +{
> +    IMFAssetLocatorMap *asset_map;
> +
> +    asset_map = av_malloc(sizeof(IMFAssetLocatorMap));
> +    if (!asset_map)
> +        return NULL;
> +
> +    asset_map->assets = NULL;
> +    asset_map->asset_count = 0;

This function could just be replaced by av_mallocz(). And if you did
this, this whole function would become superfluous.
But it is superfluous anyway, see below.

> +    return asset_map;
> +}
> +
> +/**
> + * Free a IMFAssetLocatorMap pointer.
> + */
> +static void imf_asset_locator_map_free(IMFAssetLocatorMap *asset_map)
> +{
> +    if (!asset_map)
> +        return;
> +
> +    for (int i = 0; i < asset_map->asset_count; ++i) {
> +        av_free(asset_map->assets[i].absolute_uri);
> +    }
> +
> +    av_freep(&asset_map->assets);
> +    av_freep(&asset_map);

This is bad style: You are only resetting your local variable pointer to
NULL, but you leave the pointer in the IMFContext.

> +}
> +
> +static int parse_assetmap(AVFormatContext *s, const char *url, AVIOContext *in)
> +{
> +    IMFContext *c = s->priv_data;
> +    struct AVBPrint buf;
> +    AVDictionary *opts = NULL;
> +    xmlDoc *doc = NULL;
> +    const char *base_url;
> +    char *tmp_str = NULL;
> +    int close_in = 0;
> +    int ret;
> +    int64_t filesize;
> +
> +    av_log(s, AV_LOG_DEBUG, "Asset Map URL: %s\n", url);
> +
> +    if (!in) {
> +        close_in = 1;
> +
> +        av_dict_copy(&opts, c->avio_opts, 0);
> +        ret = s->io_open(s, &in, url, AVIO_FLAG_READ, &opts);
> +        av_dict_free(&opts);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
> +    filesize = avio_size(in);
> +    filesize = filesize > 0 ? filesize : DEFAULT_ASSETMAP_SIZE;
> +
> +    av_bprint_init(&buf, filesize + 1, AV_BPRINT_SIZE_UNLIMITED);

The AVBPrint API is (currently) limited to unsigned values, ergo it may
very well be that filesize + 1 can't be represented in an unsigned. That
being said, I don't know how big asset map files can get.

> +
> +    ret = avio_read_to_bprint(in, &buf, MAX_BPRINT_READ_SIZE);
> +    if (ret < 0 || !avio_feof(in) || (filesize = buf.len) == 0) {
> +        av_log(s, AV_LOG_ERROR, "Unable to read to asset map '%s'\n", url);
> +        if (ret == 0)
> +            ret = AVERROR_INVALIDDATA;

You could just goto cleanup below here; this would save a level of
indentation below.

> +    } else {
> +        LIBXML_TEST_VERSION
> +
> +        tmp_str = strdup(url);

We have av_strdup() (although I don't really know whether this is any
better than ordinary strdup()).

> +        if (!tmp_str) {
> +            av_log(s, AV_LOG_PANIC, "Unable to duplicate string\n");

Excessive logging; also use AV_LOG_ERROR, AV_LOG_PANIC is used for
"Something went really wrong and we will crash now."

> +            ret = AVERROR(ENOMEM);
> +            goto clean_up;
> +        }
> +        base_url = av_dirname(tmp_str);
> +        if (c->asset_locator_map == NULL) {
> +            c->asset_locator_map = imf_asset_locator_map_alloc();

c->asset_locator_map will only ever have one element; in other words,
there is no need to allocate it separately at all: Just put it in
IMFContext.

> +            if (!c->asset_locator_map) {
> +                av_log(s, AV_LOG_PANIC, "Unable to allocate asset map locator\n");
> +                ret = AVERROR(ENOMEM);
> +                goto clean_up;
> +            }
> +        }
> +
> +        doc = xmlReadMemory(buf.str, filesize, url, NULL, 0);
> +
> +        if (!(ret = parse_imf_asset_map_from_xml_dom(s, doc, c->asset_locator_map, base_url)))
> +            av_log(s,
> +                AV_LOG_DEBUG,
> +                "Found %d assets from %s\n",
> +                c->asset_locator_map->asset_count,
> +                url);
> +
> +        xmlFreeDoc(doc);
> +    }
> +
> +clean_up:
> +    if (tmp_str)
> +        free(tmp_str);
> +    if (close_in)
> +        avio_close(in);
> +    av_bprint_finalize(&buf, NULL);
> +
> +    return ret;
> +}
> +
> +static IMFAssetLocator *find_asset_map_locator(IMFAssetLocatorMap *asset_map, FFUUID uuid)
> +{
> +    for (int i = 0; i < asset_map->asset_count; ++i)
> +        if (memcmp(asset_map->assets[i].uuid, uuid, 16) == 0)
> +            return &(asset_map->assets[i]);
> +    return NULL;
> +}
> +
> +static int open_track_resource_context(AVFormatContext *s,
> +    IMFVirtualTrackResourcePlaybackCtx *track_resource)
> +{
> +    IMFContext *c = s->priv_data;
> +    int ret = 0;
> +    int64_t entry_point;
> +    AVDictionary *opts = NULL;
> +
> +    if (!track_resource->ctx) {

The only way for ctx to be set here is if the context is in an
inconsistent state (see below).

> +        track_resource->ctx = avformat_alloc_context();
> +        if (!track_resource->ctx) {
> +            av_log(NULL, AV_LOG_PANIC, "Cannot allocate Track Resource Context\n");
> +            return AVERROR(ENOMEM);
> +        }
> +    }
> +
> +    if (track_resource->ctx->iformat) {

In particular, this branch here can't happen unless you are in an
inconsistent state in which the above check was a use-after-free.

> +        av_log(s,
> +            AV_LOG_DEBUG,
> +            "Input context already opened for %s.\n",
> +            track_resource->locator->absolute_uri);
> +        return ret;

return 0

> +    }
> +
> +    track_resource->ctx->io_open = s->io_open;
> +    track_resource->ctx->io_close = s->io_close;
> +    track_resource->ctx->flags |= s->flags & ~AVFMT_FLAG_CUSTOM_IO;
> +
> +    if ((ret = ff_copy_whiteblacklists(track_resource->ctx, s)) < 0)
> +        goto cleanup;

avformat_close_input() may only be called if avformat_open_input() has
been called; without this, you must call avformat_free_context().

> +
> +    av_dict_copy(&opts, c->avio_opts, 0);
> +    ret = avformat_open_input(&track_resource->ctx,
> +        track_resource->locator->absolute_uri,
> +        NULL,
> +        &opts);
> +    av_dict_free(&opts);
> +    if (ret < 0) {
> +        av_log(s,
> +            AV_LOG_ERROR,
> +            "Could not open %s input context: %s\n",
> +            track_resource->locator->absolute_uri,
> +            av_err2str(ret));
> +        goto cleanup;

You can return directly here, as avformat_open_input() frees the ctx on
failure.

> +    }
> +
> +    ret = avformat_find_stream_info(track_resource->ctx, NULL);
> +    if (ret < 0) {
> +        av_log(s,
> +            AV_LOG_ERROR,
> +            "Could not find %s stream information: %s\n",
> +            track_resource->locator->absolute_uri,
> +            av_err2str(ret));
> +        goto cleanup;
> +    }
> +
> +    // Compare the source timebase to the resource edit rate, considering the first stream of the source file
> +    if (av_cmp_q(track_resource->ctx->streams[0]->time_base, av_inv_q(track_resource->resource->base.edit_rate)))
> +        av_log(s,
> +            AV_LOG_WARNING,
> +            "Incoherent source stream timebase %d/%d regarding resource edit rate: %d/%d",
> +            track_resource->ctx->streams[0]->time_base.num,
> +            track_resource->ctx->streams[0]->time_base.den,
> +            track_resource->resource->base.edit_rate.den,
> +            track_resource->resource->base.edit_rate.num);
> +
> +    entry_point = (int64_t)track_resource->resource->base.entry_point
> +        * track_resource->resource->base.edit_rate.den
> +        * AV_TIME_BASE
> +        / track_resource->resource->base.edit_rate.num;
> +
> +    if (entry_point) {
> +        av_log(s,
> +            AV_LOG_DEBUG,
> +            "Seek at resource %s entry point: %ld\n",
> +            track_resource->locator->absolute_uri,
> +            track_resource->resource->base.entry_point);
> +        ret = avformat_seek_file(track_resource->ctx, -1, entry_point, entry_point, entry_point, 0);
> +        if (ret < 0) {
> +            av_log(s,
> +                AV_LOG_ERROR,
> +                "Could not seek at %" PRId64 "on %s: %s\n",
> +                entry_point,
> +                track_resource->locator->absolute_uri,
> +                av_err2str(ret));
> +            goto cleanup;
> +        }
> +    }
> +
> +    return ret;
> +cleanup:
> +    avformat_free_context(track_resource->ctx);

This will not reset track_resource->ctx to NULL, leaving track_resource
in an inconsistent state. This matters when called from
get_resource_context_for_timestamp() (but not for
open_track_file_resource() where track_resource lives on the stack).

> +    return ret;
> +}
> +
> +static int open_track_file_resource(AVFormatContext *s,
> +    FFIMFTrackFileResource *track_file_resource,
> +    IMFVirtualTrackPlaybackCtx *track)
> +{
> +    IMFContext *c = s->priv_data;
> +    IMFAssetLocator *asset_locator;
> +    IMFVirtualTrackResourcePlaybackCtx vt_ctx;
> +    int ret;
> +
> +    asset_locator = find_asset_map_locator(c->asset_locator_map, track_file_resource->track_file_uuid);
> +    if (!asset_locator) {
> +        av_log(s,
> +            AV_LOG_ERROR,
> +            "Could not find asset locator for UUID: " FF_UUID_FORMAT "\n",
> +            UID_ARG(track_file_resource->track_file_uuid));
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    av_log(s,
> +        AV_LOG_DEBUG,
> +        "Found locator for " FF_UUID_FORMAT ": %s\n",
> +        UID_ARG(asset_locator->uuid),
> +        asset_locator->absolute_uri);
> +
> +    track->resources = av_fast_realloc(track->resources,
> +        &track->resources_alloc_sz,
> +        (track->resource_count + track_file_resource->base.repeat_count)
> +            * sizeof(IMFVirtualTrackResourcePlaybackCtx));

Leak on error.

> +    if (!track->resources) {
> +        av_log(NULL, AV_LOG_PANIC, "Cannot allocate Virtual Track playback context\n");
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    for (int i = 0; i < track_file_resource->base.repeat_count; ++i) {
> +        vt_ctx.locator = asset_locator;
> +        vt_ctx.resource = track_file_resource;
> +        vt_ctx.ctx = NULL;
> +        if ((ret = open_track_resource_context(s, &vt_ctx)) != 0)
> +            return ret;
> +        track->resources[track->resource_count++] = vt_ctx;
> +        track->duration = av_add_q(track->duration,
> +            av_make_q((int)track_file_resource->base.duration * track_file_resource->base.edit_rate.den,
> +                track_file_resource->base.edit_rate.num));
> +    }
> +
> +    return ret;
> +}
> +
> +static int open_virtual_track(AVFormatContext *s,
> +    FFIMFTrackFileVirtualTrack *virtual_track,
> +    int32_t track_index)
> +{
> +    IMFContext *c = s->priv_data;
> +    IMFVirtualTrackPlaybackCtx *track;
> +    int ret = 0;
> +
> +    track = av_mallocz(sizeof(IMFVirtualTrackPlaybackCtx));
> +    if (!track) {
> +        av_log(NULL, AV_LOG_PANIC, "Cannot allocate IMF Virtual Track Playback context\n");

These malloc-failure messages are excessive: Just forward the error code.

> +        return AVERROR(ENOMEM);
> +    }
> +    track->index = track_index;
> +    track->duration = av_make_q(0, 1);
> +
> +    for (int i = 0; i < virtual_track->resource_count; i++) {

resource_count is unsigned, so the loop variable should be, too.
This is also true for other loop variables.

> +        av_log(s,
> +            AV_LOG_DEBUG,
> +            "Open stream from file " FF_UUID_FORMAT ", stream %d\n",
> +            UID_ARG(virtual_track->resources[i].track_file_uuid),
> +            i);
> +        if ((ret = open_track_file_resource(s, &virtual_track->resources[i], track)) != 0) {
> +            av_log(s,
> +                AV_LOG_ERROR,
> +                "Could not open image track resource " FF_UUID_FORMAT "\n",
> +                UID_ARG(virtual_track->resources[i].track_file_uuid));
> +            return ret;

track leaks here.

> +        }
> +    }
> +
> +    track->current_timestamp = av_make_q(0, track->duration.den);
> +
> +    c->tracks = av_realloc_f(c->tracks, c->track_count + 1, sizeof(IMFVirtualTrackPlaybackCtx));
> +    if (!c->tracks) {

track leaks here as well as all the c->track_count entries of c->tracks
and everything that is in them. In general, this means that you can't
use av_realloc_f() for non-POD structures.
Furthermore, in this scenario the current code produces c->tracks ==
NULL with c->track_count > 0, which will lead to a crash in imf_close().

> +        av_log(NULL, AV_LOG_PANIC, "Cannot allocate Virtual Track playback context\n");
> +        return AVERROR(ENOMEM);
> +    }
> +    c->tracks[c->track_count++] = track;
> +
> +    return ret;
> +}
> +
> +static void imf_virtual_track_playback_context_free(IMFVirtualTrackPlaybackCtx *track)
> +{
> +    if (!track)

This check would be redundant if track_count were trustworthy (which it
should be).

> +        return;
> +
> +    for (int i = 0; i < track->resource_count; ++i)
> +        if (track->resources[i].ctx && track->resources[i].ctx->iformat) {
> +            avformat_close_input(&(track->resources[i].ctx));
> +            track->resources[i].ctx = NULL;

avformat_close_input()

> +        }
> +
> +    av_freep(&(track->resources));
> +}
> +
> +static int set_context_streams_from_tracks(AVFormatContext *s)
> +{
> +    IMFContext *c = s->priv_data;
> +
> +    AVStream *asset_stream;
> +    AVStream *first_resource_stream;
> +
> +    int ret = 0;
> +
> +    for (int i = 0; i < c->track_count; ++i) {
> +        // Open the first resource of the track to get stream information
> +        first_resource_stream = c->tracks[i]->resources[0].ctx->streams[0];
> +
> +        av_log(s, AV_LOG_DEBUG, "Open the first resource of track %d\n", c->tracks[i]->index);
> +
> +        // Copy stream information
> +        asset_stream = avformat_new_stream(s, NULL);

Unchecked allocation.

> +        asset_stream->id = i;
> +        ret = avcodec_parameters_copy(asset_stream->codecpar, first_resource_stream->codecpar);
> +        if (ret < 0) {
> +            av_log(s, AV_LOG_ERROR, "Could not copy stream parameters\n");
> +            break;
> +        }
> +        avpriv_set_pts_info(asset_stream,
> +            first_resource_stream->pts_wrap_bits,
> +            first_resource_stream->time_base.num,
> +            first_resource_stream->time_base.den);
> +        asset_stream->duration = (int64_t)av_q2d(av_mul_q(c->tracks[i]->duration,
> +            av_inv_q(asset_stream->time_base)));
> +    }
> +
> +    return ret;
> +}
> +
> +static int save_avio_options(AVFormatContext *s)

This whole function is just duplicated from hls and dashdec; it is bad
enough to have two of these. A third is not acceptable.

> +{
> +    IMFContext *c = s->priv_data;
> +    static const char *const opts[] = {
> +        "headers", "http_proxy", "user_agent", "cookies", "referer", "rw_timeout", "icy", NULL};
> +    const char *const *opt = opts;
> +    uint8_t *buf;
> +    int ret = 0;
> +
> +    while (*opt) {
> +        if (av_opt_get(s->pb, *opt, AV_OPT_SEARCH_CHILDREN | AV_OPT_ALLOW_NULL, &buf) >= 0) {
> +            ret = av_dict_set(&c->avio_opts, *opt, buf, AV_DICT_DONT_STRDUP_VAL);
> +            if (ret < 0)
> +                return ret;
> +        }
> +        opt++;
> +    }
> +
> +    return ret;
> +}
> +
> +static int open_cpl_tracks(AVFormatContext *s)
> +{
> +    IMFContext *c = s->priv_data;
> +    int32_t track_index = 0;
> +    int ret;
> +
> +    if (c->cpl->main_image_2d_track)
> +        if ((ret = open_virtual_track(s, c->cpl->main_image_2d_track, track_index++)) != 0) {
> +            av_log(s,
> +                AV_LOG_ERROR,
> +                "Could not open image track " FF_UUID_FORMAT "\n",
> +                UID_ARG(c->cpl->main_image_2d_track->base.id_uuid));
> +            return ret;
> +        }
> +
> +    for (int i = 0; i < c->cpl->main_audio_track_count; ++i)
> +        if ((ret = open_virtual_track(s, &c->cpl->main_audio_tracks[i], track_index++)) != 0) {
> +            av_log(s,
> +                AV_LOG_ERROR,
> +                "Could not open audio track " FF_UUID_FORMAT "\n",
> +                UID_ARG(c->cpl->main_audio_tracks[i].base.id_uuid));
> +            return ret;
> +        }
> +
> +    return set_context_streams_from_tracks(s);
> +}
> +
> +static int imf_close(AVFormatContext *s);
> +
> +static int imf_read_header(AVFormatContext *s)
> +{
> +    IMFContext *c = s->priv_data;
> +    char *asset_map_path;
> +    int ret;
> +
> +    c->interrupt_callback = &s->interrupt_callback;
> +    c->base_url = av_dirname(av_strdup(s->url));

Unchecked allocation.

> +    if ((ret = save_avio_options(s)) < 0)
> +        goto fail;
> +
> +    av_log(s, AV_LOG_DEBUG, "start parsing IMF CPL: %s\n", s->url);
> +
> +    if ((ret = ff_parse_imf_cpl(s->pb, &c->cpl)) < 0)
> +        goto fail;
> +
> +    av_log(s,
> +        AV_LOG_DEBUG,
> +        "parsed IMF CPL: " FF_UUID_FORMAT "\n",
> +        UID_ARG(c->cpl->id_uuid));
> +
> +    if (!c->asset_map_paths)
> +        c->asset_map_paths = av_append_path_component(c->base_url, "ASSETMAP.xml");
> +
> +    // Parse each asset map XML file
> +    asset_map_path = strtok(c->asset_map_paths, ",");
> +    while (asset_map_path != NULL) {
> +        av_log(s, AV_LOG_DEBUG, "start parsing IMF Asset Map: %s\n", asset_map_path);
> +
> +        if ((ret = parse_assetmap(s, asset_map_path, NULL)) < 0)
> +            goto fail;
> +
> +        asset_map_path = strtok(NULL, ",");

av_strtok(): strtok is not thread-safe.

> +    }
> +
> +    av_log(s, AV_LOG_DEBUG, "parsed IMF Asset Maps\n");
> +
> +    if ((ret = open_cpl_tracks(s)) != 0)
> +        goto fail;
> +
> +    av_log(s, AV_LOG_DEBUG, "parsed IMF package\n");
> +    return ret;

return 0;

> +
> +fail:
> +    imf_close(s);
> +    return ret;

This and the forward declaration of imf_close() can be removed with the
FF_FMT_INIT_CLEANUP flag.

> +}
> +
> +static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVFormatContext *s)
> +{
> +    IMFContext *c = s->priv_data;
> +    IMFVirtualTrackPlaybackCtx *track;
> +
> +    AVRational minimum_timestamp = av_make_q(INT32_MAX, 1);
> +    for (int i = 0; i < c->track_count; ++i) {
> +        av_log(
> +            s,
> +            AV_LOG_DEBUG,
> +            "Compare track %d timestamp " AVRATIONAL_FORMAT
> +            " to minimum " AVRATIONAL_FORMAT
> +            " (over duration: " AVRATIONAL_FORMAT
> +            ")\n",
> +            i,
> +            AVRATIONAL_ARG(c->tracks[i]->current_timestamp),
> +            AVRATIONAL_ARG(minimum_timestamp),
> +            AVRATIONAL_ARG(c->tracks[i]->duration));
> +        if (av_cmp_q(c->tracks[i]->current_timestamp, minimum_timestamp) < 0) {
> +            track = c->tracks[i];
> +            minimum_timestamp = track->current_timestamp;
> +        }
> +    }
> +
> +    av_log(s,
> +        AV_LOG_DEBUG,
> +        "Found next track to read: %d (timestamp: %lf / %lf)\n",
> +        track->index,
> +        av_q2d(track->current_timestamp),
> +        av_q2d(minimum_timestamp));
> +    return track;
> +}
> +
> +static IMFVirtualTrackResourcePlaybackCtx *get_resource_context_for_timestamp(AVFormatContext *s,
> +    IMFVirtualTrackPlaybackCtx *track)
> +{
> +    AVRational edit_unit_duration = av_inv_q(track->resources[0].resource->base.edit_rate);
> +    AVRational cumulated_duration = av_make_q(0, edit_unit_duration.den);
> +
> +    av_log(s,
> +        AV_LOG_DEBUG,
> +        "Looking for track %d resource for timestamp = %lf / %lf\n",
> +        track->index,
> +        av_q2d(track->current_timestamp),
> +        av_q2d(track->duration));
> +    for (int i = 0; i < track->resource_count; ++i) {
> +        cumulated_duration = av_add_q(cumulated_duration,
> +            av_make_q((int)track->resources[i].resource->base.duration * edit_unit_duration.num,
> +                edit_unit_duration.den));
> +
> +        if (av_cmp_q(av_add_q(track->current_timestamp, edit_unit_duration), cumulated_duration) <= 0) {
> +            av_log(s,
> +                AV_LOG_DEBUG,
> +                "Found resource %d in track %d to read for timestamp %lf "
> +                "(on cumulated=%lf): entry=%ld, duration=%lu, editrate=" AVRATIONAL_FORMAT
> +                " | edit_unit_duration=%lf\n",
> +                i,
> +                track->index,
> +                av_q2d(track->current_timestamp),
> +                av_q2d(cumulated_duration),
> +                track->resources[i].resource->base.entry_point,
> +                track->resources[i].resource->base.duration,
> +                AVRATIONAL_ARG(track->resources[i].resource->base.edit_rate),
> +                av_q2d(edit_unit_duration));
> +
> +            if (track->current_resource_index != i) {
> +                av_log(s,
> +                    AV_LOG_DEBUG,
> +                    "Switch resource on track %d: re-open context\n",
> +                    track->index);
> +                avformat_close_input(&(track->resources[track->current_resource_index].ctx));
> +                if (open_track_resource_context(s, &(track->resources[i])) != 0)
> +                    return NULL;
> +                track->current_resource_index = i;
> +            }
> +            return &(track->resources[track->current_resource_index]);
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static int ff_imf_read_packet(AVFormatContext *s, AVPacket *pkt)

Why do you use the prefix for non-static, library internal functions?

> +{
> +    IMFContext *c = s->priv_data;
> +
> +    IMFVirtualTrackResourcePlaybackCtx *resource_to_read = NULL;
> +    AVRational edit_unit_duration;
> +    int ret = 0;
> +
> +    IMFVirtualTrackPlaybackCtx *track = get_next_track_with_minimum_timestamp(s);
> +    FFStream *track_stream;
> +
> +    if (av_cmp_q(track->current_timestamp, track->duration) == 0)
> +        return AVERROR_EOF;
> +
> +    resource_to_read = get_resource_context_for_timestamp(s, track);
> +
> +    if (!resource_to_read) {
> +        edit_unit_duration = av_inv_q(track->resources[track->current_resource_index].resource->base.edit_rate);
> +        if (av_cmp_q(av_add_q(track->current_timestamp, edit_unit_duration),
> +                track->duration)
> +            > 0)
> +            return AVERROR_EOF;
> +
> +        av_log(s, AV_LOG_ERROR, "Could not find IMF track resource to read\n");
> +        return AVERROR_STREAM_NOT_FOUND;
> +    }
> +
> +    while (!ff_check_interrupt(c->interrupt_callback) && !ret) {
> +        ret = av_read_frame(resource_to_read->ctx, pkt);
> +        av_log(s,
> +            AV_LOG_DEBUG,
> +            "Got packet: pts=%" PRId64
> +            ", dts=%" PRId64
> +            ", duration=%" PRId64
> +            ", stream_index=%d, pos=%" PRId64
> +            "\n",
> +            pkt->pts,
> +            pkt->dts,
> +            pkt->duration,
> +            pkt->stream_index,
> +            pkt->pos);
> +        track_stream = ffstream(s->streams[track->index]);
> +        if (ret >= 0) {
> +            // Update packet info from track
> +            if (pkt->dts < track_stream->cur_dts && track->last_pts > 0)
> +                pkt->dts = track_stream->cur_dts;
> +
> +            pkt->pts = track->last_pts;
> +            pkt->dts = pkt->dts
> +                - (int64_t)track->resources[track->current_resource_index].resource->base.entry_point;
> +            pkt->stream_index = track->index;
> +
> +            // Update track cursors
> +            track->current_timestamp = av_add_q(track->current_timestamp,
> +                av_make_q((int)pkt->duration * resource_to_read->ctx->streams[0]->time_base.num,
> +                    resource_to_read->ctx->streams[0]->time_base.den));
> +            track->last_pts += pkt->duration;
> +
> +            return 0;
> +        } else if (ret != AVERROR_EOF) {
> +            av_log(s,
> +                AV_LOG_ERROR,
> +                "Could not get packet from track %d: %s\n",
> +                track->index,
> +                av_err2str(ret));
> +            return ret;
> +        }
> +    }
> +
> +    return AVERROR_EOF;
> +}
> +
> +static int imf_close(AVFormatContext *s)
> +{
> +    IMFContext *c = s->priv_data;
> +
> +    av_log(s, AV_LOG_DEBUG, "Close IMF package\n");
> +    av_dict_free(&c->avio_opts);
> +    av_freep(&c->base_url);
> +    imf_asset_locator_map_free(c->asset_locator_map);
> +    ff_imf_cpl_free(c->cpl);
> +
> +    for (int i = 0; i < c->track_count; ++i) {
> +        imf_virtual_track_playback_context_free(c->tracks[i]);
> +        av_freep(&(c->tracks[i]));
> +    }
> +
> +    av_freep(&(c->tracks));
> +
> +    return 0;
> +}
> +
> +static const AVOption imf_options[] = {
> +    {"assetmaps",
> +        "IMF CPL-related asset map comma-separated absolute paths. "
> +        "If not specified, the CPL sibling `ASSETMAP.xml` file is used.",
> +        offsetof(IMFContext, asset_map_paths),
> +        AV_OPT_TYPE_STRING,
> +        {.str = NULL},
> +        0,
> +        0,
> +        AV_OPT_FLAG_DECODING_PARAM},
> +    {NULL}};

This is the weirdest way of specifying AVOptions I have ever seen. One
does not even see that the final '}' does indeed close the first '{’.
If you don't want to put everything into one gigantic line (as is
common, but has its own drawbacks), then use designated initializers and
don't initialize elements to values that they would have anyway (e.g.
the zeros for min/max are unnecessary).

> +
> +static const AVClass imf_class = {
> +    .class_name = "imf",
> +    .item_name = av_default_item_name,
> +    .option = imf_options,
> +    .version = LIBAVUTIL_VERSION_INT,
> +};
> +
> +const AVInputFormat ff_imf_demuxer = {
> +    .name = "imf",
> +    .long_name = NULL_IF_CONFIG_SMALL("IMF (Interoperable Master Format)"),
> +    .priv_class = &imf_class,
> +    .priv_data_size = sizeof(IMFContext),
> +    .read_header = imf_read_header,
> +    .read_packet = ff_imf_read_packet,
> +    .read_close = imf_close,
> +    .extensions = "xml",
> +    .mime_type = "application/xml,text/xml",

Align this and the AVClass on '='.

> +};
>

On Mon, Dec 6, 2021 at 9:45 AM Lynne <dev@lynne.ee> wrote:
>
> 6 Dec 2021, 04:18 by pal@sandflow.com:
>
> > From: Pierre-Anthony Lemieux <pal@palemieux.com>
> > +
> > +/**
>
> Please don't NIH-parse UUIDs. They're more complicated
> than this. Use libuuid, it's stable, bug-free and universally
> available. I submitted a patch for it last month for an
> unrelated use, which I decided to drop, but you
> can see how it was done there.

libuuid might be overkill and not a great match in the case of the IMF demuxer:

- libuuid does not parse the URN-encoding of UUIDs, which is used in
the IMF CPL ("urn:uuid:017b57a3-c61e-4b50-8c62-4586ea8a7769")
- the IMF demuxer does not need to generate UUIDs
- the IMF demuxer does not depend about the internal fields/format of
UUIDs or need to manipulate UUIDs: it merely compares them
byte-by-byte

>
> Also, please follow the code style. We use snake_case
> for function names, which means there are no capital
> letters in them.

Addressed in patch v8.

> There are also still one or two one-line statements
> left that are wrapped in brackets.

Addressed in patch v8.

>
> We check every single allocation we make, since malloc()
> can fail on Windows. So that means all av_strdup calls,
> av_append_path_component calls and so on can
> return NULL, and result in a nasty crash.

Addressed in patch v8.

>
> Do not use strtok. It's not thread-safe at all. Use av_strtok,
> which is thread-safe.

Addressed in patch v8.

>
> ff_imf_cpl_free() has a weird branch that can just be
> done with a return if cpl is NULL.

Addressed in patch v8.

>
> There are places where you call av_free() and then
> set the pointer to NULL. We have a convenience
> function for this, av_freep().

Addressed in patch v8.
>
> Please use defined-length integers instead of "unsigned long".
> If something needs to be 64-bits, use (u)int64_t. If something
> doesn't need to be 64-bits, use uint32_t for unsigned, or plain
> int for signed. We always require plain ints to be 32-bits.

Addressed in patch v8.

>
> Also, instead of printing 64-bit ints via "%lu", use "%"PRIu64"
> and "%"PRIi64". MSVC has problems with %lu.

Addressed in patch v8.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On Mon, Dec 6, 2021 at 12:55 PM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> pal@sandflow.com:
> > From: Pierre-Anthony Lemieux <pal@palemieux.com>
> >
> > Signed-off-by: Pierre-Anthony Lemieux <pal@palemieux.com>
> > ---
> >
> > Notes:
> >     The IMF demuxer accepts as input an IMF CPL. The assets referenced by the CPL can be
> >     contained in multiple deliveries, each defined by an ASSETMAP file:
> >
> >     ffmpeg -assetmaps <path of ASSETMAP1>,<path of ASSETMAP>,... -i <path of CPL>
> >
> >     If -assetmaps is not specified, FFMPEG looks for a file called ASSETMAP.xml in the same directory as the CPL.
> >
> >     EXAMPLE:
> >         ffmpeg -i http://ffmpeg-imf-samples-public.s3-website-us-west-1.amazonaws.com/countdown/CPL_f5095caa-f204-4e1c-8a84-7af48c7ae16b.xml out.mp4
> >
> >     The Interoperable Master Format (IMF) is a file-based media format for the
> >     delivery and storage of professional audio-visual masters.
> >     An IMF Composition consists of an XML playlist (the Composition Playlist)
> >     and a collection of MXF files (the Track Files). The Composition Playlist (CPL)
> >     assembles the Track Files onto a timeline, which consists of multiple tracks.
> >     The location of the Track Files referenced by the Composition Playlist is stored
> >     in one or more XML documents called Asset Maps. More details at https://www.imfug.com/explainer.
> >     The IMF standard was first introduced in 2013 and is managed by the SMPTE.
> >
> >     CHANGE NOTES:
> >
> >     - reduced line width
> >     - use ff_ and FF prefixes for non-local functions and structures
> >     - modified copyright header
> >     - fixed rational initialization
> >     - removed extraneous call to xmlCleanupParser()
> >     - fix if/for single line braces
> >     - replace av_realloc_f with av_fast_realloc when allocating CPL Resources and
> >       ASSETMAP assets
> >
> >  MAINTAINERS              |   1 +
> >  configure                |   3 +-
> >  doc/demuxers.texi        |  19 +
> >  libavformat/Makefile     |   1 +
> >  libavformat/allformats.c |   1 +
> >  libavformat/imf.h        | 206 +++++++++
> >  libavformat/imf_cpl.c    | 769 +++++++++++++++++++++++++++++++++
> >  libavformat/imfdec.c     | 907 +++++++++++++++++++++++++++++++++++++++
> >  8 files changed, 1906 insertions(+), 1 deletion(-)
> >  create mode 100644 libavformat/imf.h
> >  create mode 100644 libavformat/imf_cpl.c
> >  create mode 100644 libavformat/imfdec.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index dcac46003e..7a6972fe1a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -433,6 +433,7 @@ Muxers/Demuxers:
> >    idroqdec.c                            Mike Melanson
> >    iff.c                                 Jaikrishnan Menon
> >    img2*.c                               Michael Niedermayer
> > +  imf*.c                                Marc-Antoine Arnaud, Pierre-Anthony Lemieux, Valentin Noël
> >    ipmovie.c                             Mike Melanson
> >    ircam*                                Paul B Mahol
> >    iss.c                                 Stefan Gehrer
> > diff --git a/configure b/configure
> > index a98a18abaa..022f17767b 100755
> > --- a/configure
> > +++ b/configure
> > @@ -298,7 +298,7 @@ External library support:
> >    --enable-libxvid         enable Xvid encoding via xvidcore,
> >                             native MPEG-4/Xvid encoder exists [no]
> >    --enable-libxml2         enable XML parsing using the C library libxml2, needed
> > -                           for dash demuxing support [no]
> > +                           for dash and imf demuxing support [no]
> >    --enable-libzimg         enable z.lib, needed for zscale filter [no]
> >    --enable-libzmq          enable message passing via libzmq [no]
> >    --enable-libzvbi         enable teletext support via libzvbi [no]
> > @@ -3400,6 +3400,7 @@ hls_muxer_select="mpegts_muxer"
> >  hls_muxer_suggest="gcrypt openssl"
> >  image2_alias_pix_demuxer_select="image2_demuxer"
> >  image2_brender_pix_demuxer_select="image2_demuxer"
> > +imf_demuxer_deps="libxml2"
> >  ipod_muxer_select="mov_muxer"
> >  ismv_muxer_select="mov_muxer"
> >  ivf_muxer_select="av1_metadata_bsf vp9_superframe_bsf"
> > diff --git a/doc/demuxers.texi b/doc/demuxers.texi
> > index cab8a7072c..614f2e754a 100644
> > --- a/doc/demuxers.texi
> > +++ b/doc/demuxers.texi
> > @@ -267,6 +267,12 @@ which streams to actually receive.
> >  Each stream mirrors the @code{id} and @code{bandwidth} properties from the
> >  @code{<Representation>} as metadata keys named "id" and "variant_bitrate" respectively.
> >
> > +@section imf
> > +
> > +Interoperable Master Format demuxer.
> > +
> > +This demuxer presents audio and video streams found in an IMF Composition.
> > +
> >  @section flv, live_flv, kux
> >
> >  Adobe Flash Video Format demuxer.
> > @@ -770,6 +776,19 @@ MJPEG stream. Turning this option on by setting it to 1 will result in a stricte
> >  of the boundary value.
> >  @end table
> >
> > +@section mxf
> > +
> > +MXF demuxer.
> > +
> > +@table @option
> > +
> > +@item -eia608_extract @var{bool}
> > +Extract EIA-608 captions from SMPTE 436M track
> > +
> > +@item -skip_audio_reordering @var{bool}
> > +This option will disable the audio reordering based on Multi-Channel Audio (MCA) labelling (SMPTE ST-377-4).
> > +@end table
> > +
>
> Why are you adding documentation for mxf in this patch?

Fixed at patch v8. I had applied the MCA patchset on my branch. That
patchset is necessary to process the MXF files used in IMF.

>
> >  @section rawvideo
> >
> >  Raw video demuxer.
> > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c
> > new file mode 100644
> > index 0000000000..ec4ad28444
> > --- /dev/null
> > +++ b/libavformat/imfdec.c
> > @@ -0,0 +1,907 @@
> > +/*
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * FFmpeg is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with FFmpeg; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> > + */
> > +
> > +/*
> > + *
> > + * Copyright (c) Sandflow Consulting LLC
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions are met:
> > + *
> > + * * Redistributions of source code must retain the above copyright notice, this
> > + *   list of conditions and the following disclaimer.
> > + * * Redistributions in binary form must reproduce the above copyright notice,
> > + *   this list of conditions and the following disclaimer in the documentation
> > + *   and/or other materials provided with the distribution.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> > + * POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +
> > +/**
> > + * Demuxes an IMF Composition
> > + *
> > + * References
> > + * OV 2067-0:2018 - SMPTE Overview Document - Interoperable Master Format
> > + * ST 2067-2:2020 - SMPTE Standard - Interoperable Master Format — Core Constraints
> > + * ST 2067-3:2020 - SMPTE Standard - Interoperable Master Format — Composition Playlist
> > + * ST 2067-5:2020 - SMPTE Standard - Interoperable Master Format — Essence Component
> > + * ST 2067-20:2016 - SMPTE Standard - Interoperable Master Format — Application #2
> > + * ST 2067-21:2020 - SMPTE Standard - Interoperable Master Format — Application #2 Extended
> > + * ST 2067-102:2017 - SMPTE Standard - Interoperable Master Format — Common Image Pixel Color Schemes
> > + * ST 429-9:2007 - SMPTE Standard - D-Cinema Packaging — Asset Mapping and File Segmentation
> > + *
> > + * @author Marc-Antoine Arnaud
> > + * @author Valentin Noel
> > + * @author Nicholas Vanderzwet
> > + * @file
> > + * @ingroup lavu_imf
> > + */
> > +
> > +#include "imf.h"
> > +#include "internal.h"
> > +#include "libavutil/avstring.h"
> > +#include "libavutil/bprint.h"
> > +#include "libavutil/opt.h"
> > +#include "mxf.h"
> > +#include "url.h"
> > +#include <inttypes.h>
> > +#include <libxml/parser.h>
> > +
> > +#define MAX_BPRINT_READ_SIZE (UINT_MAX - 1)
> > +#define DEFAULT_ASSETMAP_SIZE 8 * 1024
> > +#define AVRATIONAL_FORMAT "%d/%d"
> > +#define AVRATIONAL_ARG(rational) rational.num, rational.den
> > +
> > +/**
> > + * IMF Asset locator
> > + */
> > +typedef struct IMFAssetLocator {
> > +    FFUUID uuid;
> > +    char *absolute_uri;
> > +} IMFAssetLocator;
> > +
> > +/**
> > + * IMF Asset locator map
> > + * Results from the parsing of one or more ASSETMAP XML files
> > + */
> > +typedef struct IMFAssetLocatorMap {
> > +    uint32_t asset_count;
> > +    IMFAssetLocator *assets;
> > +} IMFAssetLocatorMap;
> > +
> > +typedef struct IMFVirtualTrackResourcePlaybackCtx {
> > +    IMFAssetLocator *locator;
> > +    FFIMFTrackFileResource *resource;
> > +    AVFormatContext *ctx;
> > +} IMFVirtualTrackResourcePlaybackCtx;
> > +
> > +typedef struct IMFVirtualTrackPlaybackCtx {
> > +    // Track index in playlist
> > +    int32_t index;
> > +    // Time counters
> > +    AVRational current_timestamp;
> > +    AVRational duration;
> > +    // Resources
> > +    unsigned int resource_count;
> > +    unsigned int resources_alloc_sz;
> > +    IMFVirtualTrackResourcePlaybackCtx *resources;
> > +    // Decoding cursors
> > +    uint32_t current_resource_index;
> > +    int64_t last_pts;
> > +} IMFVirtualTrackPlaybackCtx;
> > +
> > +typedef struct IMFContext {
> > +    const AVClass *class;
> > +    const char *base_url;
> > +    char *asset_map_paths;
> > +    AVIOInterruptCB *interrupt_callback;
> > +    AVDictionary *avio_opts;
> > +    FFIMFCPL *cpl;
> > +    IMFAssetLocatorMap *asset_locator_map;
> > +    unsigned int track_count;
> > +    IMFVirtualTrackPlaybackCtx **tracks;
> > +} IMFContext;
> > +
> > +static int imf_uri_is_url(const char *string)
> > +{
> > +    char *substr = strstr(string, "://");
>
> Just because strstr() is not const-correct does not mean that you should
> be, too. Use const char *substr.

Addressed at patch v8. The imf_uri_is* functions have been simplified.

>
> > +    return substr != NULL;
> > +}
> > +
> > +static int imf_uri_is_unix_abs_path(const char *string)
> > +{
> > +    char *substr = strstr(string, "/");
>
> This is equivalent to strchr(string, '/').

Addressed at patch v8. The imf_uri_is* functions have been simplified.

>
> > +    int index = (int)(substr - string);
>
> This might be undefined behaviour string is long (pointer->integer
> conversions are UB if the result can't be represented in the destination
> type).

Addressed at patch v8. The imf_uri_is* functions have been simplified.

>
> > +    return index == 0;
>
> This makes the whole function equivalent to "return string[0] == '/';".

Addressed at patch v8. The imf_uri_is* functions have been simplified.

>
> > +}
> > +
> > +static int imf_uri_is_dos_abs_path(const char *string)
> > +{
> > +    // Absolute path case: `C:\path\to\somwhere`
> > +    char *substr = strstr(string, ":\\");
> > +    int index = (int)(substr - string);
> > +    if (index == 1)
> > +        return 1;
> > +
> > +    // Absolute path case: `C:/path/to/somwhere`
> > +    substr = strstr(string, ":/");
> > +    index = (int)(substr - string);
> > +    if (index == 1)
> > +        return 1;
> > +
> > +    // Network path case: `\\path\to\somwhere`
> > +    substr = strstr(string, "\\\\");
> > +    index = (int)(substr - string);
> > +    return index == 0;
> > +}
> > +
> > +/**
> > + * Parse a ASSETMAP XML file to extract the UUID-URI mapping of assets.
> > + * @param s the current format context, if any (can be NULL).
> > + * @param doc the XML document to be parsed.
> > + * @param asset_map pointer on the IMFAssetLocatorMap to fill.
> > + * @param base_url the url of the asset map XML file, if any (can be NULL).
> > + * @return a negative value in case of error, 0 otherwise.
> > + */
> > +static int parse_imf_asset_map_from_xml_dom(AVFormatContext *s,
> > +    xmlDocPtr doc,
> > +    IMFAssetLocatorMap *asset_map,
> > +    const char *base_url)
> > +{
> > +    xmlNodePtr asset_map_element = NULL;
> > +    xmlNodePtr node = NULL;
> > +    xmlNodePtr asset_element = NULL;
> > +    char *uri;
> > +    int ret = 0;
> > +    IMFAssetLocator *asset = NULL;
> > +
> > +    asset_map_element = xmlDocGetRootElement(doc);
> > +
> > +    if (!asset_map_element) {
> > +        av_log(s, AV_LOG_ERROR, "Unable to parse asset map XML - missing root node\n");
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    if (asset_map_element->type != XML_ELEMENT_NODE || av_strcasecmp(asset_map_element->name, "AssetMap")) {
> > +        av_log(s,
> > +            AV_LOG_ERROR,
> > +            "Unable to parse asset map XML - wrong root node name[%s] type[%d]\n",
> > +            asset_map_element->name,
> > +            (int)asset_map_element->type);
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    // parse asset locators
> > +
> > +    if (!(node = ff_xml_get_child_element_by_name(asset_map_element, "AssetList"))) {
> > +        av_log(s, AV_LOG_ERROR, "Unable to parse asset map XML - missing AssetList node\n");
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +    asset_map->assets = av_realloc_f(asset_map->assets,
> > +        xmlChildElementCount(node),
> > +        sizeof(IMFAssetLocator));
>
> Leak in case of error: All the absolute_uri strings already contained in
> the array will leak.

Addressed at patch v8. I have gone through the codebase and made sure
that arrays that contain elements referencing heap objects are not
deleted by failed calls to realloc().

>
> > +    if (!asset_map->assets) {
> > +        av_log(NULL, AV_LOG_PANIC, "Cannot allocate IMF asset locators\n");
> > +        return AVERROR(ENOMEM);
> > +    }
> > +    asset_map->asset_count = 0;
>
> And this implicitly leaks the absolute_uri strings even on success (only
> in case there were any such strings, i.e. if the loop in
> imf_read_header() (that loops over parse_assetmap()) is actually a
> loop). They will either be overwritten below or just not be freed at the
> end.

Addressed at patch v8.

>
> > +    asset_element = xmlFirstElementChild(node);
> > +    while (asset_element) {
> > +        if (av_strcasecmp(asset_element->name, "Asset") != 0)
> > +            continue;
> > +
> > +        asset = &(asset_map->assets[asset_map->asset_count]);
> > +
> > +        if (ff_xml_read_UUID(ff_xml_get_child_element_by_name(asset_element, "Id"), asset->uuid)) {
> > +            av_log(s, AV_LOG_ERROR, "Could not parse UUID from asset in asset map.\n");
> > +            return AVERROR_INVALIDDATA;
> > +        }
> > +
> > +        av_log(s, AV_LOG_DEBUG, "Found asset id: " FF_UUID_FORMAT "\n", UID_ARG(asset->uuid));
> > +
> > +        if (!(node = ff_xml_get_child_element_by_name(asset_element, "ChunkList"))) {
> > +            av_log(s, AV_LOG_ERROR, "Unable to parse asset map XML - missing ChunkList node\n");
> > +            return AVERROR_INVALIDDATA;
> > +        }
> > +
> > +        if (!(node = ff_xml_get_child_element_by_name(node, "Chunk"))) {
> > +            av_log(s, AV_LOG_ERROR, "Unable to parse asset map XML - missing Chunk node\n");
> > +            return AVERROR_INVALIDDATA;
> > +        }
> > +
> > +        uri = xmlNodeGetContent(ff_xml_get_child_element_by_name(node, "Path"));
> > +        if (!imf_uri_is_url(uri) && !imf_uri_is_unix_abs_path(uri) && !imf_uri_is_dos_abs_path(uri))
> > +            asset->absolute_uri = av_append_path_component(base_url, uri);
> > +        else
> > +            asset->absolute_uri = av_strdup(uri);
> > +        xmlFree(uri);
> > +        if (!asset->absolute_uri) {
> > +            av_log(NULL, AV_LOG_PANIC, "Cannot allocate asset locator absolute URI\n");
> > +            return AVERROR(ENOMEM);
> > +        }
> > +
> > +        av_log(s, AV_LOG_DEBUG, "Found asset absolute URI: %s\n", asset->absolute_uri);
> > +
> > +        asset_map->asset_count++;
> > +        asset_element = xmlNextElementSibling(asset_element);
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +/**
> > + * Allocate a IMFAssetLocatorMap pointer and return it.
> > + * @return the allocated IMFAssetLocatorMap pointer.
> > + */
> > +static IMFAssetLocatorMap *imf_asset_locator_map_alloc(void)
> > +{
> > +    IMFAssetLocatorMap *asset_map;
> > +
> > +    asset_map = av_malloc(sizeof(IMFAssetLocatorMap));
> > +    if (!asset_map)
> > +        return NULL;
> > +
> > +    asset_map->assets = NULL;
> > +    asset_map->asset_count = 0;
>
> This function could just be replaced by av_mallocz(). And if you did
> this, this whole function would become superfluous.
> But it is superfluous anyway, see below.

Addressed at patch v8. imf_asset_locator_map_alloc() no longer
allocates heap for an IMFAssetLocatorMap structure but just
initializes it.

>
> > +    return asset_map;
> > +}
> > +
> > +/**
> > + * Free a IMFAssetLocatorMap pointer.
> > + */
> > +static void imf_asset_locator_map_free(IMFAssetLocatorMap *asset_map)
> > +{
> > +    if (!asset_map)
> > +        return;
> > +
> > +    for (int i = 0; i < asset_map->asset_count; ++i) {
> > +        av_free(asset_map->assets[i].absolute_uri);
> > +    }
> > +
> > +    av_freep(&asset_map->assets);
> > +    av_freep(&asset_map);
>
> This is bad style: You are only resetting your local variable pointer to
> NULL, but you leave the pointer in the IMFContext.

Addressed at patch v8.

>
> > +}
> > +
> > +static int parse_assetmap(AVFormatContext *s, const char *url, AVIOContext *in)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    struct AVBPrint buf;
> > +    AVDictionary *opts = NULL;
> > +    xmlDoc *doc = NULL;
> > +    const char *base_url;
> > +    char *tmp_str = NULL;
> > +    int close_in = 0;
> > +    int ret;
> > +    int64_t filesize;
> > +
> > +    av_log(s, AV_LOG_DEBUG, "Asset Map URL: %s\n", url);
> > +
> > +    if (!in) {
> > +        close_in = 1;
> > +
> > +        av_dict_copy(&opts, c->avio_opts, 0);
> > +        ret = s->io_open(s, &in, url, AVIO_FLAG_READ, &opts);
> > +        av_dict_free(&opts);
> > +        if (ret < 0)
> > +            return ret;
> > +    }
> > +
> > +    filesize = avio_size(in);
> > +    filesize = filesize > 0 ? filesize : DEFAULT_ASSETMAP_SIZE;
> > +
> > +    av_bprint_init(&buf, filesize + 1, AV_BPRINT_SIZE_UNLIMITED);
>
> The AVBPrint API is (currently) limited to unsigned values, ergo it may
> very well be that filesize + 1 can't be represented in an unsigned. That
> being said, I don't know how big asset map files can get.

ASSETMAP files should be smaller than 10 MB in practice.

>
> > +
> > +    ret = avio_read_to_bprint(in, &buf, MAX_BPRINT_READ_SIZE);
> > +    if (ret < 0 || !avio_feof(in) || (filesize = buf.len) == 0) {
> > +        av_log(s, AV_LOG_ERROR, "Unable to read to asset map '%s'\n", url);
> > +        if (ret == 0)
> > +            ret = AVERROR_INVALIDDATA;
>
> You could just goto cleanup below here; this would save a level of
> indentation below.

Addressed at patch v8.

>
> > +    } else {
> > +        LIBXML_TEST_VERSION
> > +
> > +        tmp_str = strdup(url);
>
> We have av_strdup() (although I don't really know whether this is any
> better than ordinary strdup()).
>
> > +        if (!tmp_str) {
> > +            av_log(s, AV_LOG_PANIC, "Unable to duplicate string\n");
>
> Excessive logging; also use AV_LOG_ERROR, AV_LOG_PANIC is used for
> "Something went really wrong and we will crash now."

Addressed at patch v8. Replaced all AV_LOG_PANIC with AV_LOG_ERROR.

>
> > +            ret = AVERROR(ENOMEM);
> > +            goto clean_up;
> > +        }
> > +        base_url = av_dirname(tmp_str);
> > +        if (c->asset_locator_map == NULL) {
> > +            c->asset_locator_map = imf_asset_locator_map_alloc();
>
> c->asset_locator_map will only ever have one element; in other words,
> there is no need to allocate it separately at all: Just put it in
> IMFContext.

Addressed at patch v8.

>
> > +            if (!c->asset_locator_map) {
> > +                av_log(s, AV_LOG_PANIC, "Unable to allocate asset map locator\n");
> > +                ret = AVERROR(ENOMEM);
> > +                goto clean_up;
> > +            }
> > +        }
> > +
> > +        doc = xmlReadMemory(buf.str, filesize, url, NULL, 0);
> > +
> > +        if (!(ret = parse_imf_asset_map_from_xml_dom(s, doc, c->asset_locator_map, base_url)))
> > +            av_log(s,
> > +                AV_LOG_DEBUG,
> > +                "Found %d assets from %s\n",
> > +                c->asset_locator_map->asset_count,
> > +                url);
> > +
> > +        xmlFreeDoc(doc);
> > +    }
> > +
> > +clean_up:
> > +    if (tmp_str)
> > +        free(tmp_str);
> > +    if (close_in)
> > +        avio_close(in);
> > +    av_bprint_finalize(&buf, NULL);
> > +
> > +    return ret;
> > +}
> > +
> > +static IMFAssetLocator *find_asset_map_locator(IMFAssetLocatorMap *asset_map, FFUUID uuid)
> > +{
> > +    for (int i = 0; i < asset_map->asset_count; ++i)
> > +        if (memcmp(asset_map->assets[i].uuid, uuid, 16) == 0)
> > +            return &(asset_map->assets[i]);
> > +    return NULL;
> > +}
> > +
> > +static int open_track_resource_context(AVFormatContext *s,
> > +    IMFVirtualTrackResourcePlaybackCtx *track_resource)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    int ret = 0;
> > +    int64_t entry_point;
> > +    AVDictionary *opts = NULL;
> > +
> > +    if (!track_resource->ctx) {
>
> The only way for ctx to be set here is if the context is in an
> inconsistent state (see below).

track_resource->ctx can be released in
get_resource_context_for_timestamp() by a call to
avformat_close_input().

>
> > +        track_resource->ctx = avformat_alloc_context();
> > +        if (!track_resource->ctx) {
> > +            av_log(NULL, AV_LOG_PANIC, "Cannot allocate Track Resource Context\n");
> > +            return AVERROR(ENOMEM);
> > +        }
> > +    }
> > +
> > +    if (track_resource->ctx->iformat) {
>
> In particular, this branch here can't happen unless you are in an
> inconsistent state in which the above check was a use-after-free.
>
> > +        av_log(s,
> > +            AV_LOG_DEBUG,
> > +            "Input context already opened for %s.\n",
> > +            track_resource->locator->absolute_uri);
> > +        return ret;
>
> return 0

Addressed at patch v8.

>
> > +    }
> > +
> > +    track_resource->ctx->io_open = s->io_open;
> > +    track_resource->ctx->io_close = s->io_close;
> > +    track_resource->ctx->flags |= s->flags & ~AVFMT_FLAG_CUSTOM_IO;
> > +
> > +    if ((ret = ff_copy_whiteblacklists(track_resource->ctx, s)) < 0)
> > +        goto cleanup;
>
> avformat_close_input() may only be called if avformat_open_input() has
> been called; without this, you must call avformat_free_context().

Clean-up calls avformat_free_context().

>
> > +
> > +    av_dict_copy(&opts, c->avio_opts, 0);
> > +    ret = avformat_open_input(&track_resource->ctx,
> > +        track_resource->locator->absolute_uri,
> > +        NULL,
> > +        &opts);
> > +    av_dict_free(&opts);
> > +    if (ret < 0) {
> > +        av_log(s,
> > +            AV_LOG_ERROR,
> > +            "Could not open %s input context: %s\n",
> > +            track_resource->locator->absolute_uri,
> > +            av_err2str(ret));
> > +        goto cleanup;
>
> You can return directly here, as avformat_open_input() frees the ctx on
> failure.

Addressed at patch v8.

>
> > +    }
> > +
> > +    ret = avformat_find_stream_info(track_resource->ctx, NULL);
> > +    if (ret < 0) {
> > +        av_log(s,
> > +            AV_LOG_ERROR,
> > +            "Could not find %s stream information: %s\n",
> > +            track_resource->locator->absolute_uri,
> > +            av_err2str(ret));
> > +        goto cleanup;
> > +    }
> > +
> > +    // Compare the source timebase to the resource edit rate, considering the first stream of the source file
> > +    if (av_cmp_q(track_resource->ctx->streams[0]->time_base, av_inv_q(track_resource->resource->base.edit_rate)))
> > +        av_log(s,
> > +            AV_LOG_WARNING,
> > +            "Incoherent source stream timebase %d/%d regarding resource edit rate: %d/%d",
> > +            track_resource->ctx->streams[0]->time_base.num,
> > +            track_resource->ctx->streams[0]->time_base.den,
> > +            track_resource->resource->base.edit_rate.den,
> > +            track_resource->resource->base.edit_rate.num);
> > +
> > +    entry_point = (int64_t)track_resource->resource->base.entry_point
> > +        * track_resource->resource->base.edit_rate.den
> > +        * AV_TIME_BASE
> > +        / track_resource->resource->base.edit_rate.num;
> > +
> > +    if (entry_point) {
> > +        av_log(s,
> > +            AV_LOG_DEBUG,
> > +            "Seek at resource %s entry point: %ld\n",
> > +            track_resource->locator->absolute_uri,
> > +            track_resource->resource->base.entry_point);
> > +        ret = avformat_seek_file(track_resource->ctx, -1, entry_point, entry_point, entry_point, 0);
> > +        if (ret < 0) {
> > +            av_log(s,
> > +                AV_LOG_ERROR,
> > +                "Could not seek at %" PRId64 "on %s: %s\n",
> > +                entry_point,
> > +                track_resource->locator->absolute_uri,
> > +                av_err2str(ret));
> > +            goto cleanup;
> > +        }
> > +    }
> > +
> > +    return ret;
> > +cleanup:
> > +    avformat_free_context(track_resource->ctx);
>
> This will not reset track_resource->ctx to NULL, leaving track_resource
> in an inconsistent state. This matters when called from
> get_resource_context_for_timestamp() (but not for
> open_track_file_resource() where track_resource lives on the stack).

Addressed at patch v8. track_resource->ctx set to NULL after
avformat_free_context().

>
> > +    return ret;
> > +}
> > +
> > +static int open_track_file_resource(AVFormatContext *s,
> > +    FFIMFTrackFileResource *track_file_resource,
> > +    IMFVirtualTrackPlaybackCtx *track)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    IMFAssetLocator *asset_locator;
> > +    IMFVirtualTrackResourcePlaybackCtx vt_ctx;
> > +    int ret;
> > +
> > +    asset_locator = find_asset_map_locator(c->asset_locator_map, track_file_resource->track_file_uuid);
> > +    if (!asset_locator) {
> > +        av_log(s,
> > +            AV_LOG_ERROR,
> > +            "Could not find asset locator for UUID: " FF_UUID_FORMAT "\n",
> > +            UID_ARG(track_file_resource->track_file_uuid));
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    av_log(s,
> > +        AV_LOG_DEBUG,
> > +        "Found locator for " FF_UUID_FORMAT ": %s\n",
> > +        UID_ARG(asset_locator->uuid),
> > +        asset_locator->absolute_uri);
> > +
> > +    track->resources = av_fast_realloc(track->resources,
> > +        &track->resources_alloc_sz,
> > +        (track->resource_count + track_file_resource->base.repeat_count)
> > +            * sizeof(IMFVirtualTrackResourcePlaybackCtx));
>
> Leak on error.

Addressed at patch v8.

>
> > +    if (!track->resources) {
> > +        av_log(NULL, AV_LOG_PANIC, "Cannot allocate Virtual Track playback context\n");
> > +        return AVERROR(ENOMEM);
> > +    }
> > +
> > +    for (int i = 0; i < track_file_resource->base.repeat_count; ++i) {
> > +        vt_ctx.locator = asset_locator;
> > +        vt_ctx.resource = track_file_resource;
> > +        vt_ctx.ctx = NULL;
> > +        if ((ret = open_track_resource_context(s, &vt_ctx)) != 0)
> > +            return ret;
> > +        track->resources[track->resource_count++] = vt_ctx;
> > +        track->duration = av_add_q(track->duration,
> > +            av_make_q((int)track_file_resource->base.duration * track_file_resource->base.edit_rate.den,
> > +                track_file_resource->base.edit_rate.num));
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static int open_virtual_track(AVFormatContext *s,
> > +    FFIMFTrackFileVirtualTrack *virtual_track,
> > +    int32_t track_index)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    IMFVirtualTrackPlaybackCtx *track;
> > +    int ret = 0;
> > +
> > +    track = av_mallocz(sizeof(IMFVirtualTrackPlaybackCtx));
> > +    if (!track) {
> > +        av_log(NULL, AV_LOG_PANIC, "Cannot allocate IMF Virtual Track Playback context\n");
>
> These malloc-failure messages are excessive: Just forward the error code.

Addressed at patch v8. I have removed the error message in the case of
unlikely malloc failures, but kept it when the malloc failure might
point to unreasonably large files.

>
> > +        return AVERROR(ENOMEM);
> > +    }
> > +    track->index = track_index;
> > +    track->duration = av_make_q(0, 1);
> > +
> > +    for (int i = 0; i < virtual_track->resource_count; i++) {
>
> resource_count is unsigned, so the loop variable should be, too.
> This is also true for other loop variables.

Addressed at patch v8.

>
> > +        av_log(s,
> > +            AV_LOG_DEBUG,
> > +            "Open stream from file " FF_UUID_FORMAT ", stream %d\n",
> > +            UID_ARG(virtual_track->resources[i].track_file_uuid),
> > +            i);
> > +        if ((ret = open_track_file_resource(s, &virtual_track->resources[i], track)) != 0) {
> > +            av_log(s,
> > +                AV_LOG_ERROR,
> > +                "Could not open image track resource " FF_UUID_FORMAT "\n",
> > +                UID_ARG(virtual_track->resources[i].track_file_uuid));
> > +            return ret;
>
> track leaks here.

Addressed at patch v8.

>
> > +        }
> > +    }
> > +
> > +    track->current_timestamp = av_make_q(0, track->duration.den);
> > +
> > +    c->tracks = av_realloc_f(c->tracks, c->track_count + 1, sizeof(IMFVirtualTrackPlaybackCtx));
> > +    if (!c->tracks) {
>
> track leaks here as well as all the c->track_count entries of c->tracks
> and everything that is in them. In general, this means that you can't
> use av_realloc_f() for non-POD structures.
> Furthermore, in this scenario the current code produces c->tracks ==
> NULL with c->track_count > 0, which will lead to a crash in imf_close().

Addressed at patch v8.

>
> > +        av_log(NULL, AV_LOG_PANIC, "Cannot allocate Virtual Track playback context\n");
> > +        return AVERROR(ENOMEM);
> > +    }
> > +    c->tracks[c->track_count++] = track;
> > +
> > +    return ret;
> > +}
> > +
> > +static void imf_virtual_track_playback_context_free(IMFVirtualTrackPlaybackCtx *track)
> > +{
> > +    if (!track)
>
> This check would be redundant if track_count were trustworthy (which it
> should be).

Addressed at patch v8.

>
> > +        return;
> > +
> > +    for (int i = 0; i < track->resource_count; ++i)
> > +        if (track->resources[i].ctx && track->resources[i].ctx->iformat) {
> > +            avformat_close_input(&(track->resources[i].ctx));
> > +            track->resources[i].ctx = NULL;
>
> avformat_close_input()

Addressed at patch v8.

>
> > +        }
> > +
> > +    av_freep(&(track->resources));
> > +}
> > +
> > +static int set_context_streams_from_tracks(AVFormatContext *s)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +
> > +    AVStream *asset_stream;
> > +    AVStream *first_resource_stream;
> > +
> > +    int ret = 0;
> > +
> > +    for (int i = 0; i < c->track_count; ++i) {
> > +        // Open the first resource of the track to get stream information
> > +        first_resource_stream = c->tracks[i]->resources[0].ctx->streams[0];
> > +
> > +        av_log(s, AV_LOG_DEBUG, "Open the first resource of track %d\n", c->tracks[i]->index);
> > +
> > +        // Copy stream information
> > +        asset_stream = avformat_new_stream(s, NULL);
>
> Unchecked allocation.

Addressed at patch v8.

>
> > +        asset_stream->id = i;
> > +        ret = avcodec_parameters_copy(asset_stream->codecpar, first_resource_stream->codecpar);
> > +        if (ret < 0) {
> > +            av_log(s, AV_LOG_ERROR, "Could not copy stream parameters\n");
> > +            break;
> > +        }
> > +        avpriv_set_pts_info(asset_stream,
> > +            first_resource_stream->pts_wrap_bits,
> > +            first_resource_stream->time_base.num,
> > +            first_resource_stream->time_base.den);
> > +        asset_stream->duration = (int64_t)av_q2d(av_mul_q(c->tracks[i]->duration,
> > +            av_inv_q(asset_stream->time_base)));
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static int save_avio_options(AVFormatContext *s)
>
> This whole function is just duplicated from hls and dashdec; it is bad
> enough to have two of these. A third is not acceptable.

Ok. Happy to create a separate patch/start a separate thread. Do you
have suggestion on where the common code should go?

>
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    static const char *const opts[] = {
> > +        "headers", "http_proxy", "user_agent", "cookies", "referer", "rw_timeout", "icy", NULL};
> > +    const char *const *opt = opts;
> > +    uint8_t *buf;
> > +    int ret = 0;
> > +
> > +    while (*opt) {
> > +        if (av_opt_get(s->pb, *opt, AV_OPT_SEARCH_CHILDREN | AV_OPT_ALLOW_NULL, &buf) >= 0) {
> > +            ret = av_dict_set(&c->avio_opts, *opt, buf, AV_DICT_DONT_STRDUP_VAL);
> > +            if (ret < 0)
> > +                return ret;
> > +        }
> > +        opt++;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static int open_cpl_tracks(AVFormatContext *s)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    int32_t track_index = 0;
> > +    int ret;
> > +
> > +    if (c->cpl->main_image_2d_track)
> > +        if ((ret = open_virtual_track(s, c->cpl->main_image_2d_track, track_index++)) != 0) {
> > +            av_log(s,
> > +                AV_LOG_ERROR,
> > +                "Could not open image track " FF_UUID_FORMAT "\n",
> > +                UID_ARG(c->cpl->main_image_2d_track->base.id_uuid));
> > +            return ret;
> > +        }
> > +
> > +    for (int i = 0; i < c->cpl->main_audio_track_count; ++i)
> > +        if ((ret = open_virtual_track(s, &c->cpl->main_audio_tracks[i], track_index++)) != 0) {
> > +            av_log(s,
> > +                AV_LOG_ERROR,
> > +                "Could not open audio track " FF_UUID_FORMAT "\n",
> > +                UID_ARG(c->cpl->main_audio_tracks[i].base.id_uuid));
> > +            return ret;
> > +        }
> > +
> > +    return set_context_streams_from_tracks(s);
> > +}
> > +
> > +static int imf_close(AVFormatContext *s);
> > +
> > +static int imf_read_header(AVFormatContext *s)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    char *asset_map_path;
> > +    int ret;
> > +
> > +    c->interrupt_callback = &s->interrupt_callback;
> > +    c->base_url = av_dirname(av_strdup(s->url));
>
> Unchecked allocation.

Addressed at patch v8.


>
> > +    if ((ret = save_avio_options(s)) < 0)
> > +        goto fail;
> > +
> > +    av_log(s, AV_LOG_DEBUG, "start parsing IMF CPL: %s\n", s->url);
> > +
> > +    if ((ret = ff_parse_imf_cpl(s->pb, &c->cpl)) < 0)
> > +        goto fail;
> > +
> > +    av_log(s,
> > +        AV_LOG_DEBUG,
> > +        "parsed IMF CPL: " FF_UUID_FORMAT "\n",
> > +        UID_ARG(c->cpl->id_uuid));
> > +
> > +    if (!c->asset_map_paths)
> > +        c->asset_map_paths = av_append_path_component(c->base_url, "ASSETMAP.xml");
> > +
> > +    // Parse each asset map XML file
> > +    asset_map_path = strtok(c->asset_map_paths, ",");
> > +    while (asset_map_path != NULL) {
> > +        av_log(s, AV_LOG_DEBUG, "start parsing IMF Asset Map: %s\n", asset_map_path);
> > +
> > +        if ((ret = parse_assetmap(s, asset_map_path, NULL)) < 0)
> > +            goto fail;
> > +
> > +        asset_map_path = strtok(NULL, ",");
>
> av_strtok(): strtok is not thread-safe.

Addressed at patch v8.

>
> > +    }
> > +
> > +    av_log(s, AV_LOG_DEBUG, "parsed IMF Asset Maps\n");
> > +
> > +    if ((ret = open_cpl_tracks(s)) != 0)
> > +        goto fail;
> > +
> > +    av_log(s, AV_LOG_DEBUG, "parsed IMF package\n");
> > +    return ret;
>
> return 0;

Addressed at patch v8.

>
> > +
> > +fail:
> > +    imf_close(s);
> > +    return ret;
>
> This and the forward declaration of imf_close() can be removed with the
> FF_FMT_INIT_CLEANUP flag.

Addressed at patch v8.

>
> > +}
> > +
> > +static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVFormatContext *s)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    IMFVirtualTrackPlaybackCtx *track;
> > +
> > +    AVRational minimum_timestamp = av_make_q(INT32_MAX, 1);
> > +    for (int i = 0; i < c->track_count; ++i) {
> > +        av_log(
> > +            s,
> > +            AV_LOG_DEBUG,
> > +            "Compare track %d timestamp " AVRATIONAL_FORMAT
> > +            " to minimum " AVRATIONAL_FORMAT
> > +            " (over duration: " AVRATIONAL_FORMAT
> > +            ")\n",
> > +            i,
> > +            AVRATIONAL_ARG(c->tracks[i]->current_timestamp),
> > +            AVRATIONAL_ARG(minimum_timestamp),
> > +            AVRATIONAL_ARG(c->tracks[i]->duration));
> > +        if (av_cmp_q(c->tracks[i]->current_timestamp, minimum_timestamp) < 0) {
> > +            track = c->tracks[i];
> > +            minimum_timestamp = track->current_timestamp;
> > +        }
> > +    }
> > +
> > +    av_log(s,
> > +        AV_LOG_DEBUG,
> > +        "Found next track to read: %d (timestamp: %lf / %lf)\n",
> > +        track->index,
> > +        av_q2d(track->current_timestamp),
> > +        av_q2d(minimum_timestamp));
> > +    return track;
> > +}
> > +
> > +static IMFVirtualTrackResourcePlaybackCtx *get_resource_context_for_timestamp(AVFormatContext *s,
> > +    IMFVirtualTrackPlaybackCtx *track)
> > +{
> > +    AVRational edit_unit_duration = av_inv_q(track->resources[0].resource->base.edit_rate);
> > +    AVRational cumulated_duration = av_make_q(0, edit_unit_duration.den);
> > +
> > +    av_log(s,
> > +        AV_LOG_DEBUG,
> > +        "Looking for track %d resource for timestamp = %lf / %lf\n",
> > +        track->index,
> > +        av_q2d(track->current_timestamp),
> > +        av_q2d(track->duration));
> > +    for (int i = 0; i < track->resource_count; ++i) {
> > +        cumulated_duration = av_add_q(cumulated_duration,
> > +            av_make_q((int)track->resources[i].resource->base.duration * edit_unit_duration.num,
> > +                edit_unit_duration.den));
> > +
> > +        if (av_cmp_q(av_add_q(track->current_timestamp, edit_unit_duration), cumulated_duration) <= 0) {
> > +            av_log(s,
> > +                AV_LOG_DEBUG,
> > +                "Found resource %d in track %d to read for timestamp %lf "
> > +                "(on cumulated=%lf): entry=%ld, duration=%lu, editrate=" AVRATIONAL_FORMAT
> > +                " | edit_unit_duration=%lf\n",
> > +                i,
> > +                track->index,
> > +                av_q2d(track->current_timestamp),
> > +                av_q2d(cumulated_duration),
> > +                track->resources[i].resource->base.entry_point,
> > +                track->resources[i].resource->base.duration,
> > +                AVRATIONAL_ARG(track->resources[i].resource->base.edit_rate),
> > +                av_q2d(edit_unit_duration));
> > +
> > +            if (track->current_resource_index != i) {
> > +                av_log(s,
> > +                    AV_LOG_DEBUG,
> > +                    "Switch resource on track %d: re-open context\n",
> > +                    track->index);
> > +                avformat_close_input(&(track->resources[track->current_resource_index].ctx));
> > +                if (open_track_resource_context(s, &(track->resources[i])) != 0)
> > +                    return NULL;
> > +                track->current_resource_index = i;
> > +            }
> > +            return &(track->resources[track->current_resource_index]);
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> > +static int ff_imf_read_packet(AVFormatContext *s, AVPacket *pkt)
>
> Why do you use the prefix for non-static, library internal functions?

Addressed at patch v8. Probably a refactoring error.

>
> > +{
> > +    IMFContext *c = s->priv_data;
> > +
> > +    IMFVirtualTrackResourcePlaybackCtx *resource_to_read = NULL;
> > +    AVRational edit_unit_duration;
> > +    int ret = 0;
> > +
> > +    IMFVirtualTrackPlaybackCtx *track = get_next_track_with_minimum_timestamp(s);
> > +    FFStream *track_stream;
> > +
> > +    if (av_cmp_q(track->current_timestamp, track->duration) == 0)
> > +        return AVERROR_EOF;
> > +
> > +    resource_to_read = get_resource_context_for_timestamp(s, track);
> > +
> > +    if (!resource_to_read) {
> > +        edit_unit_duration = av_inv_q(track->resources[track->current_resource_index].resource->base.edit_rate);
> > +        if (av_cmp_q(av_add_q(track->current_timestamp, edit_unit_duration),
> > +                track->duration)
> > +            > 0)
> > +            return AVERROR_EOF;
> > +
> > +        av_log(s, AV_LOG_ERROR, "Could not find IMF track resource to read\n");
> > +        return AVERROR_STREAM_NOT_FOUND;
> > +    }
> > +
> > +    while (!ff_check_interrupt(c->interrupt_callback) && !ret) {
> > +        ret = av_read_frame(resource_to_read->ctx, pkt);
> > +        av_log(s,
> > +            AV_LOG_DEBUG,
> > +            "Got packet: pts=%" PRId64
> > +            ", dts=%" PRId64
> > +            ", duration=%" PRId64
> > +            ", stream_index=%d, pos=%" PRId64
> > +            "\n",
> > +            pkt->pts,
> > +            pkt->dts,
> > +            pkt->duration,
> > +            pkt->stream_index,
> > +            pkt->pos);
> > +        track_stream = ffstream(s->streams[track->index]);
> > +        if (ret >= 0) {
> > +            // Update packet info from track
> > +            if (pkt->dts < track_stream->cur_dts && track->last_pts > 0)
> > +                pkt->dts = track_stream->cur_dts;
> > +
> > +            pkt->pts = track->last_pts;
> > +            pkt->dts = pkt->dts
> > +                - (int64_t)track->resources[track->current_resource_index].resource->base.entry_point;
> > +            pkt->stream_index = track->index;
> > +
> > +            // Update track cursors
> > +            track->current_timestamp = av_add_q(track->current_timestamp,
> > +                av_make_q((int)pkt->duration * resource_to_read->ctx->streams[0]->time_base.num,
> > +                    resource_to_read->ctx->streams[0]->time_base.den));
> > +            track->last_pts += pkt->duration;
> > +
> > +            return 0;
> > +        } else if (ret != AVERROR_EOF) {
> > +            av_log(s,
> > +                AV_LOG_ERROR,
> > +                "Could not get packet from track %d: %s\n",
> > +                track->index,
> > +                av_err2str(ret));
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    return AVERROR_EOF;
> > +}
> > +
> > +static int imf_close(AVFormatContext *s)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +
> > +    av_log(s, AV_LOG_DEBUG, "Close IMF package\n");
> > +    av_dict_free(&c->avio_opts);
> > +    av_freep(&c->base_url);
> > +    imf_asset_locator_map_free(c->asset_locator_map);
> > +    ff_imf_cpl_free(c->cpl);
> > +
> > +    for (int i = 0; i < c->track_count; ++i) {
> > +        imf_virtual_track_playback_context_free(c->tracks[i]);
> > +        av_freep(&(c->tracks[i]));
> > +    }
> > +
> > +    av_freep(&(c->tracks));
> > +
> > +    return 0;
> > +}
> > +
> > +static const AVOption imf_options[] = {
> > +    {"assetmaps",
> > +        "IMF CPL-related asset map comma-separated absolute paths. "
> > +        "If not specified, the CPL sibling `ASSETMAP.xml` file is used.",
> > +        offsetof(IMFContext, asset_map_paths),
> > +        AV_OPT_TYPE_STRING,
> > +        {.str = NULL},
> > +        0,
> > +        0,
> > +        AV_OPT_FLAG_DECODING_PARAM},
> > +    {NULL}};
>
> This is the weirdest way of specifying AVOptions I have ever seen. One
> does not even see that the final '}' does indeed close the first '{’.
> If you don't want to put everything into one gigantic line (as is
> common, but has its own drawbacks), then use designated initializers and
> don't initialize elements to values that they would have anyway (e.g.
> the zeros for min/max are unnecessary).

Addressed at patch v8.

>
> > +
> > +static const AVClass imf_class = {
> > +    .class_name = "imf",
> > +    .item_name = av_default_item_name,
> > +    .option = imf_options,
> > +    .version = LIBAVUTIL_VERSION_INT,
> > +};
> > +
> > +const AVInputFormat ff_imf_demuxer = {
> > +    .name = "imf",
> > +    .long_name = NULL_IF_CONFIG_SMALL("IMF (Interoperable Master Format)"),
> > +    .priv_class = &imf_class,
> > +    .priv_data_size = sizeof(IMFContext),
> > +    .read_header = imf_read_header,
> > +    .read_packet = ff_imf_read_packet,
> > +    .read_close = imf_close,
> > +    .extensions = "xml",
> > +    .mime_type = "application/xml,text/xml",
>
> Align this and the AVClass on '='.

Addressed at patch v8.

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