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

* Re: [FFmpeg-devel] [PATCH v11 1/2] avformat/imf: Demuxer
       [not found] <20211214163626.22186-1-pal@sandflow.com>
@ 2021-12-17 21:46 ` Andreas Rheinhardt
  2021-12-19  5:49   ` Pierre-Anthony Lemieux
  0 siblings, 1 reply; 2+ messages in thread
From: Andreas Rheinhardt @ 2021-12-17 21:46 UTC (permalink / raw)
  To: ffmpeg-devel

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:
>     
>     - limit Track Files to MXF
>     - remove stealth variable assignment
>     - remove unused function parameter
> 
>  MAINTAINERS              |   1 +
>  configure                |   3 +-
>  doc/demuxers.texi        |   6 +
>  libavformat/Makefile     |   1 +
>  libavformat/allformats.c |   1 +
>  libavformat/imf.h        | 207 +++++++++
>  libavformat/imf_cpl.c    | 783 +++++++++++++++++++++++++++++++++
>  libavformat/imfdec.c     | 905 +++++++++++++++++++++++++++++++++++++++
>  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/libavformat/imfdec.c b/libavformat/imfdec.c
> new file mode 100644
> index 0000000000..e7c094ebd7
> --- /dev/null
> +++ b/libavformat/imfdec.c
> @@ -0,0 +1,905 @@
> +/*
> + * 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 {
> +    FFIMFUUID 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
> +    uint32_t resource_count;
> +    uint32_t 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;
> +    uint32_t track_count;
> +    IMFVirtualTrackPlaybackCtx **tracks;
> +} IMFContext;
> +
> +static int imf_uri_is_url(const char *string)
> +{
> +    return strstr(string, "://") != NULL;
> +}
> +
> +static int imf_uri_is_unix_abs_path(const char *string)
> +{
> +    return string[0] == '/';
> +}
> +
> +static int imf_uri_is_dos_abs_path(const char *string)
> +{
> +    /* Absolute path case: `C:\path\to\somwhere` */
> +    if (string[1] == ':' && string[2] == '\\')
> +        return 1;
> +
> +    /* Absolute path case: `C:/path/to/somwhere` */
> +    if (string[1] == ':' && string[2] == '/')
> +        return 1;
> +
> +    /* Network path case: `\\path\to\somwhere` */
> +    if (string[0] == '\\' && string[1] == '\\')
> +        return 1;
> +
> +    return 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;
> +    void *tmp;
> +
> +    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_imf_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;
> +    }
> +    tmp = av_realloc(asset_map->assets,
> +        (xmlChildElementCount(node) + asset_map->asset_count)
> +            * sizeof(IMFAssetLocator));

Can this overflow? If so, av_realloc_array() would take care of the
overflow for the multiplication; but it would not do anything for the
addition.

> +    if (!tmp) {
> +        av_log(NULL, AV_LOG_ERROR, "Cannot allocate IMF asset locators\n");
> +        return AVERROR(ENOMEM);
> +    }
> +    asset_map->assets = tmp;
> +
> +    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_imf_xml_read_uuid(ff_imf_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_IMF_UUID_FORMAT "\n", UID_ARG(asset->uuid));
> +
> +        if (!(node = ff_imf_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_imf_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_imf_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) {
> +            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;
> +}
> +
> +/**
> + * Initializes an IMFAssetLocatorMap structure.
> + */
> +static void imf_asset_locator_map_init(IMFAssetLocatorMap *asset_map)
> +{
> +    asset_map->assets = NULL;
> +    asset_map->asset_count = 0;
> +}
> +
> +/**
> + * Free a IMFAssetLocatorMap pointer.
> + */
> +static void imf_asset_locator_map_deinit(IMFAssetLocatorMap *asset_map)
> +{
> +    for (uint32_t i = 0; i < asset_map->asset_count; ++i)
> +        av_freep(&asset_map->assets[i].absolute_uri);
> +    av_freep(&asset_map->assets);
> +}
> +
> +static int parse_assetmap(AVFormatContext *s, const char *url)
> +{
> +    IMFContext *c = s->priv_data;
> +    AVIOContext *in = NULL;
> +    struct AVBPrint buf;
> +    AVDictionary *opts = NULL;

opts should be local to the block below (if said block is indeed needed).

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

Is there a proper initialization for in missing above? This check is
always true.

> +        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);
> +
> +    ret = avio_read_to_bprint(in, &buf, MAX_BPRINT_READ_SIZE);
> +    if (ret < 0 || !avio_feof(in) || buf.len == 0) {
> +        av_log(s, AV_LOG_ERROR, "Unable to read to asset map '%s'\n", url);
> +        if (ret == 0)
> +            ret = AVERROR_INVALIDDATA;
> +        goto clean_up;
> +    }
> +
> +    LIBXML_TEST_VERSION
> +
> +    tmp_str = av_strdup(url);
> +    if (!tmp_str) {
> +        ret = AVERROR(ENOMEM);
> +        goto clean_up;
> +    }
> +    base_url = av_dirname(tmp_str);
> +
> +    filesize = buf.len;
> +    doc = xmlReadMemory(buf.str, filesize, url, NULL, 0);
> +
> +    ret = parse_imf_asset_map_from_xml_dom(s, doc, &c->asset_locator_map, base_url);
> +    if (!ret)
> +        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)
> +        av_freep(&tmp_str);
> +    if (close_in)
> +        avio_close(in);

If you open an AVIOContext via ->io_open, you should close it via
io_close; or actually: via ff_format_io_close(s, &in).

> +    av_bprint_finalize(&buf, NULL);
> +
> +    return ret;
> +}
> +
> +static IMFAssetLocator *find_asset_map_locator(IMFAssetLocatorMap *asset_map, FFIMFUUID uuid)
> +{
> +    for (uint32_t 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) {
> +        track_resource->ctx = avformat_alloc_context();
> +        if (!track_resource->ctx)
> +            return AVERROR(ENOMEM);
> +    }
> +
> +    if (track_resource->ctx->iformat) {

I do not see a scenario in which track_resource->ctx could be != NULL,
but track_resource->ctx->iformat is not: After all, you free it on error
of ff_copy_whiteblacklists() and avformat_open_input() frees it on
error, too. So you could just as well just check for track_resource->ctx
at the beginning of this function and return in this case and allocate
track_resource->ctx unconditionally in the other case.

> +        av_log(s,
> +            AV_LOG_DEBUG,
> +            "Input context already opened for %s.\n",
> +            track_resource->locator->absolute_uri);
> +        return 0;
> +    }
> +
> +    track_resource->ctx->io_open = s->io_open;
> +    track_resource->ctx->io_close = s->io_close;

We recently added an io_close2.

> +    track_resource->ctx->flags |= s->flags & ~AVFMT_FLAG_CUSTOM_IO;
> +
> +    if ((ret = ff_copy_whiteblacklists(track_resource->ctx, s)) < 0)
> +        goto cleanup;
> +
> +    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));
> +        return ret;
> +    }
> +
> +    if (av_strcasecmp(track_resource->ctx->iformat->name, "mxf")) {

strcmp -- the mxf demuxer is just called mxf. This is fixed and not
subject to change. But see below.

> +        av_log(s,
> +            AV_LOG_ERROR,
> +            "Track file kind is not MXF but %s instead\n",
> +            track_resource->ctx->iformat->name);

There is a problem here: Imagine that this is called from
imf_read_packet() via get_resource_context_for_timestamp(). Then you
error out on that imf_read_packet() call; yet the a later
imf_read_packet() call may see that track_resource->ctx is already set
and just reuse it.
The easiest fix for this is to set the ctx's format_whitelist to "mxf"
(set this via the av_opt_set()). Alternatively, one could also set the
ctx's iformat to the mxf demuxer unconditionally. Both of this would
have to be done before avformat_open_input().

> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    /* 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: %" PRIu32 "\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;

resources opened with avformat_open_input() should be freed with
avformat_close_input().

> +        }
> +    }
> +
> +    return ret;

return 0

> +cleanup:
> +    avformat_free_context(track_resource->ctx);
> +    track_resource->ctx = NULL;
> +    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;
> +    void *tmp;
> +    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_IMF_UUID_FORMAT "\n",
> +            UID_ARG(track_file_resource->track_file_uuid));
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    av_log(s,
> +        AV_LOG_DEBUG,
> +        "Found locator for " FF_IMF_UUID_FORMAT ": %s\n",
> +        UID_ARG(asset_locator->uuid),
> +        asset_locator->absolute_uri);
> +    tmp = av_fast_realloc(track->resources,
> +        &track->resources_alloc_sz,
> +        (track->resource_count + track_file_resource->base.repeat_count)
> +            * sizeof(IMFVirtualTrackResourcePlaybackCtx));
> +    if (!tmp)
> +        return AVERROR(ENOMEM);
> +    track->resources = tmp;
> +
> +    for (uint32_t 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;

vt_ctx should be local to this loop.

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

return 0

> +}
> +
> +static void imf_virtual_track_playback_context_deinit(IMFVirtualTrackPlaybackCtx *track)
> +{
> +    for (uint32_t i = 0; i < track->resource_count; ++i)
> +        if (track->resources[i].ctx && track->resources[i].ctx->iformat)
> +            avformat_close_input(&track->resources[i].ctx);

avformat_close_input() can handle the case of track->resources[i].ctx
being NULL; and given that any AVFormatContext here is either NULL or
properly opened with avformat_open_input() you can just remove these checks.

> +
> +    av_freep(&track->resources);
> +}
> +
> +static int open_virtual_track(AVFormatContext *s,
> +    FFIMFTrackFileVirtualTrack *virtual_track,
> +    int32_t track_index)
> +{
> +    IMFContext *c = s->priv_data;
> +    IMFVirtualTrackPlaybackCtx *track = NULL;
> +    void *tmp;
> +    int ret = 0;
> +
> +    if (!(track = av_mallocz(sizeof(IMFVirtualTrackPlaybackCtx))))
> +        return AVERROR(ENOMEM);
> +    track->index = track_index;
> +    track->duration = av_make_q(0, 1);
> +
> +    for (uint32_t i = 0; i < virtual_track->resource_count; i++) {
> +        av_log(s,
> +            AV_LOG_DEBUG,
> +            "Open stream from file " FF_IMF_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_IMF_UUID_FORMAT "\n",
> +                UID_ARG(virtual_track->resources[i].track_file_uuid));
> +            goto clean_up;
> +        }
> +    }
> +
> +    track->current_timestamp = av_make_q(0, track->duration.den);
> +
> +    tmp = av_realloc(c->tracks, (c->track_count + 1) * sizeof(IMFVirtualTrackPlaybackCtx *));

Missing overflow check; av_realloc_array() would do this for you.

> +    if (!tmp) {
> +        ret = AVERROR(ENOMEM);
> +        goto clean_up;
> +    }
> +    c->tracks = tmp;
> +    c->tracks[c->track_count++] = track;
> +
> +    return 0;
> +
> +clean_up:
> +    imf_virtual_track_playback_context_deinit(track);
> +    av_free(track);
> +    return ret;
> +}
> +
> +static int set_context_streams_from_tracks(AVFormatContext *s)
> +{
> +    IMFContext *c = s->priv_data;
> +
> +    AVStream *asset_stream;
> +    AVStream *first_resource_stream;

Both of these should be local to the loop.

> +
> +    int ret = 0;
> +
> +    for (uint32_t 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);
> +        if (!asset_stream) {
> +            av_log(s, AV_LOG_ERROR, "Could not create stream\n");

missing AVERROR(ENOMEM)

> +            break;
> +        }
> +        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;

just return ret

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

return 0

> +}
> +
> +static int save_avio_options(AVFormatContext *s)
> +{
> +    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_IMF_UUID_FORMAT "\n",
> +                UID_ARG(c->cpl->main_image_2d_track->base.id_uuid));
> +            return ret;
> +        }
> +
> +    for (uint32_t 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_IMF_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_read_header(AVFormatContext *s)
> +{
> +    IMFContext *c = s->priv_data;
> +    char *asset_map_path;
> +    char *tmp_str;
> +    int ret = 0;
> +
> +    c->interrupt_callback = &s->interrupt_callback;
> +    tmp_str = av_strdup(s->url);
> +    if (!tmp_str) {
> +        ret = AVERROR(ENOMEM);

return AVERROR(ENOMEM);

> +        return ret;
> +    }
> +    c->base_url = av_dirname(tmp_str);
> +    if ((ret = save_avio_options(s)) < 0)
> +        return ret;
> +
> +    av_log(s, AV_LOG_DEBUG, "start parsing IMF CPL: %s\n", s->url);
> +
> +    if ((ret = ff_imf_parse_cpl(s->pb, &c->cpl)) < 0)
> +        return ret;
> +
> +    av_log(s,
> +        AV_LOG_DEBUG,
> +        "parsed IMF CPL: " FF_IMF_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");
> +        if (!c->asset_map_paths) {
> +            ret = AVERROR(ENOMEM);
> +            return ret;
> +        }
> +        av_log(s, AV_LOG_DEBUG, "No asset maps provided, using the default ASSETMAP.xml\n");
> +    }
> +
> +    /* Parse each asset map XML file */
> +    imf_asset_locator_map_init(&c->asset_locator_map);
> +    asset_map_path = av_strtok(c->asset_map_paths, ",", &tmp_str);
> +    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))
> +            return ret;
> +
> +        asset_map_path = av_strtok(NULL, ",", &tmp_str);
> +    }
> +
> +    av_log(s, AV_LOG_DEBUG, "parsed IMF Asset Maps\n");
> +
> +    if (ret = open_cpl_tracks(s))
> +        return ret;
> +
> +    av_log(s, AV_LOG_DEBUG, "parsed IMF package\n");
> +
> +    return 0;
> +}
> +
> +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 (uint32_t 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),

This presumes that track is set; yet isn't it possible for all tracks'
current_timestamps to be (AVRational){ INT32_MAX, 1 }? Maybe you should
check for <= in the above check? (If you also reverse your loop
iterator, the result would be the same as now in the non-pathological
case; if the order of tracks with the same current_timestamp matters at
all).

> +        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 (uint32_t 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=%" PRIu32
> +                ", duration=%" PRIu32
> +                ", 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));

You should probably set current_resource_index to an invalid value here.
Otherwise it might happen that if open_track_resource_context() fails,
another call to get_resource_context_for_timestamp() (from a later
imf_read_packet()) might return a pointer to an
IMFVirtualTrackResourcePlaybackCtx without AVFormatContext.

> +                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 imf_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +    IMFContext *c = s->priv_data;
> +    IMFVirtualTrackResourcePlaybackCtx *resource_to_read = NULL;
> +    AVRational edit_unit_duration;
> +    int ret = 0;
> +    IMFVirtualTrackPlaybackCtx *track;
> +    FFStream *track_stream;
> +
> +    track = get_next_track_with_minimum_timestamp(s);
> +
> +    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_deinit(&c->asset_locator_map);
> +    ff_imf_cpl_free(c->cpl);
> +
> +    for (uint32_t i = 0; i < c->track_count; ++i) {
> +        imf_virtual_track_playback_context_deinit(c->tracks[i]);
> +        av_freep(&c->tracks[i]);
> +    }
> +
> +    av_freep(&c->tracks);
> +
> +    return 0;
> +}
> +
> +static int imf_probe(const AVProbeData *p)
> +{
> +    if (!strstr(p->buf, "<CompositionPlaylist"))
> +        return 0;
> +
> +    /* check for a ContentTitle element without including ContentTitleText,
> +     * which is used by the D-Cinema CPL.
> +     */
> +    if (!strstr(p->buf, "ContentTitle>"))
> +        return 0;
> +
> +    return AVPROBE_SCORE_MAX;
> +}
> +
> +static const AVOption imf_options[] = {
> +    {
> +        .name        = "assetmaps",
> +        .help        = "Comma-separated paths to ASSETMAP files."
> +                       "If not specified, the `ASSETMAP.xml` file in the same directory as the CPL is used.",
> +        .offset      = offsetof(IMFContext, asset_map_paths),
> +        .type        = AV_OPT_TYPE_STRING,
> +        .default_val = {.str = NULL},
> +        .flags       = AV_OPT_FLAG_DECODING_PARAM,
> +    },
> +    {NULL},
> +};
> +
> +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)"),
> +    .flags_internal = FF_FMT_INIT_CLEANUP,
> +    .priv_class     = &imf_class,
> +    .priv_data_size = sizeof(IMFContext),
> +    .read_probe     = imf_probe,
> +    .read_header    = imf_read_header,
> +    .read_packet    = imf_read_packet,
> +    .read_close     = imf_close

Add a ','; otherwise one would have to add a ',' to this line if one
were to add something after this line.

> +};
> 

PS: Me not saying anything about imf_cpl.c is just the result of me not
having time to look at it.
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 2+ messages in thread