From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 4B65940072 for ; Sun, 19 Dec 2021 05:50:11 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 6CDB368AEFD; Sun, 19 Dec 2021 07:50:09 +0200 (EET) Received: from mail-il1-f180.google.com (mail-il1-f180.google.com [209.85.166.180]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 47ED568A7A9 for ; Sun, 19 Dec 2021 07:50:03 +0200 (EET) Received: by mail-il1-f180.google.com with SMTP id l5so5036282ilv.7 for ; Sat, 18 Dec 2021 21:50:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sandflow-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=uQ3o10v8TFGlaQMADplQfgG/QzhDdrE15s4TdhoYOig=; b=U5WCmxcXp9gtXVpY4Nie0XrtYLn4hIzHnP22auXj/cvQNw+hwjebymoviy0piLWF8o 6tWh8m3s0gog72VnGFCWiXUTgNhMGQ2XUzV43+JlFKUvj4Du3xaszch/WConbGKcW9ZK 0brJPyEUmrYRxMh0juQDbhmzChKudufiIHMfZGGBEWlIO4MhO+3MClQ4TLaa0IyJcBEi VETXjlU/53IZ2aatt0FdRAj+izj+1U6i/yJBpFJ4LkU7wlOhf+P6SMx5TW6bF2zx9/jU x3VHdxEGgp/z88Y3FA7mO1OlL3FYUfgrHk0kdC+z2ZZJvGlWtt0N8huy2JA8hGt2yZbh /kNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=uQ3o10v8TFGlaQMADplQfgG/QzhDdrE15s4TdhoYOig=; b=nQ0IYamIUT7blDmf1eZ3BSX0yDfROc+jbYAvwYdY98itOwWfim39QNWomRUHoB37B7 WMJmk+GyXy3iwKf+Qza5DZPIB3bHkeFA7DwJsdRdyg8IPHg6RX1x+Yun6ApTnmaN6TIq NLurmcTAzdmdlkGVP7XRbdTBI1Lgya0CVIIyGo6IOKAB6mOM7t1+q2E3xg5A9jqw6302 W3ByjXgfKcFIS6+SnXf2FmPjdUTjVqk1XOzv4xb4ZniJuP+SXwNvFF9DCcF37iQysU87 uRhlMW+DjZv7DGw4HFnhiuZ1Mj7IHEaCblFlAW1Cc58cBnqNp93UAV2Fh45Q2USCUDMN yaxg== X-Gm-Message-State: AOAM530FVI4zDi/sHi4W0AJ5fRcke80Ir93XGMNTXU6G3fMngZWISfbE BfEFEU5wg8rmgBzfOZV58LZVQ4Uz1ehVZA== X-Google-Smtp-Source: ABdhPJw0B3TamF7hek59batWuaHZ3bpQ9vJZcaKjA23bF7AxtCVlUSiprup55v5aAYTka/vTHYbFuA== X-Received: by 2002:a92:d410:: with SMTP id q16mr1194135ilm.295.1639893001528; Sat, 18 Dec 2021 21:50:01 -0800 (PST) Received: from mail-il1-f179.google.com (mail-il1-f179.google.com. [209.85.166.179]) by smtp.gmail.com with ESMTPSA id x15sm6887634iob.8.2021.12.18.21.50.01 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 18 Dec 2021 21:50:01 -0800 (PST) Received: by mail-il1-f179.google.com with SMTP id f17so5027249ilj.11 for ; Sat, 18 Dec 2021 21:50:01 -0800 (PST) X-Received: by 2002:a92:c60f:: with SMTP id p15mr5457594ilm.179.1639893000571; Sat, 18 Dec 2021 21:50:00 -0800 (PST) MIME-Version: 1.0 References: <20211214163626.22186-1-pal@sandflow.com> In-Reply-To: From: Pierre-Anthony Lemieux Date: Sat, 18 Dec 2021 21:49:48 -0800 X-Gmail-Original-Message-ID: Message-ID: To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] [PATCH v11 1/2] avformat/imf: Demuxer X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Fri, Dec 17, 2021 at 1:46 PM Andreas Rheinhardt wrote: > > pal@sandflow.com: > > From: Pierre-Anthony Lemieux > > > > Signed-off-by: Pierre-Anthony Lemieux > > --- > > > > 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: > > + } > > + 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. Addressed at v12. av_realloc() replaced with av_realloc_array(). Added checks for multiplication and addition overflows before allocation. Note: this would not overflow in usual operation, but could presumably do so with malformed files, malicious or not. > > > + if (!tmp) { > > + av_log(NULL, AV_LOG_ERROR, "Cannot allocate IMF asset locators\n"); > > + return AVERROR(ENOMEM); > > + } > > +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. Addressed at v12. Removed check. > > > + close_in = 1; > > + 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). Addressed at v12. > > > > + av_bprint_finalize(&buf, NULL); > > + > > + > > + 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. Addressed at v12. Codepath cleaned. > > > + 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. Addressed at v12. io_close2 added. > > > + 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. Mooted by the 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(). Addressed at v12. iformat->name check replaced with av_opt_set(...format_whitelist...) > > > + return AVERROR_INVALIDDATA; > > + } > > + > > + /* Compare the source timebase to the resource edit rate, considering the first stream of the source file */ > > + 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(). Addressed at v12. > > > + } > > + } > > + > > + return ret; > > return 0 Addressed at v12. > > > +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 Addressed at v12. > > > +} > > + > > +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. > Addressed at v12. Checks removed. > > + > > + 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. Addressed at v12. Replaced with av_realloc_array(). > > > + 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. Addressed at v12. > > > + > > + 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) Addressed at v12. > > > + 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 Addressed at v12. > > > + } > > + 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 Addressed at v12. > > > +} > > + > > +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; > > + > > + 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); Addressed at v12. > > > + 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, > > + "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). Addressed at v12. Loop reversed and <= used. > > > + 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. Addressed at v12. The next resource is first opened, and then the current resource is closed. > > > + 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; > > +}; > > + > > +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. Addressed at v12. > > > +}; > > > > PS: Me not saying anything about imf_cpl.c is just the result of me not > having time to look at it. FWIW I ported your recommendations to imf_cpl.c whenever applicable. > _______________________________________________ > 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". _______________________________________________ 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".