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

[Anton Khirnov]

Quoting pal at sandflow.com (2021-12-13 06:43:35)
> +static int parse_assetmap(AVFormatContext *s, const char *url, AVIOContext *in)

in is always NULL, might as well make it a local variable

> +{
> +    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);
> +
> +    ret = avio_read_to_bprint(in, &buf, MAX_BPRINT_READ_SIZE);
> +    if (ret < 0 || !avio_feof(in) || (filesize = buf.len) == 0) {

Stealth assignments in if () clauses are sinful and a recipe for bugs.
Especially when a part of several statements. Please don't.

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

Is this really necessary? This might potentially take a long time and
invoke decoders and so should be avoided unless you really need it.

-- 
Anton Khirnov