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

[Pierre-Anthony Lemieux]

On Tue, Dec 14, 2021 at 2:33 AM Anton Khirnov <anton at khirnov.net> wrote:
>
> 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

Addressed by v11 of the patchset.

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

Addressed by v11 of the patchset.

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

Addressed by v11 of the patchset.

By restricting Track Files to MXF, I think we can avoid
avformat_find_stream_info() since the MXF demuxer fills in
streams->time_base on opening.

>
> --
> Anton Khirnov