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 5C99A405D7 for ; Tue, 21 Dec 2021 19:04:06 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id EAA9E68AEC8; Tue, 21 Dec 2021 21:04:03 +0200 (EET) Received: from mail-io1-f48.google.com (mail-io1-f48.google.com [209.85.166.48]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 233C468A9D3 for ; Tue, 21 Dec 2021 21:03:58 +0200 (EET) Received: by mail-io1-f48.google.com with SMTP id p65so19076760iof.3 for ; Tue, 21 Dec 2021 11:03:58 -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=vpKI95HmvQ9z8n5fToqWRgS+MCP9/3BXfJL2Hx4RLic=; b=zTTN7CYc496zQVdFLkg90jL2tyjEhbWJwdGIGUkCZR8guP81sh8pzsThpeTxpEMx1/ wRv3BrhJZDXd8FiVGiI2gTU+HJSTpz3n3R0UHnMqu83qm/HyAomhorp5XrOFNHeeRM45 yqzgJbU6XueZNUTvRawnm680nNahkG6gg+fLw2B3xVJ4XN3jdLTBW39gaBKLu3q3vZwa I1qGumB4/jExHz8GNkGNnnP/0kfGgh3mYfyK3aJ86G3zaE/Jjr+l3UlbvQ/2ouI/ll5p cOFMl3Q6jexKTjvYZeLd9aC287Ex7i+Y+ixVYLB65d4d0EJv+AgwqQ5uGB9T+kgeiA3Z w9pg== 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=vpKI95HmvQ9z8n5fToqWRgS+MCP9/3BXfJL2Hx4RLic=; b=K3REZ2rjTXbE3spmQ0/gsmPxruDrWTcFqQyf08fbI6wTz0F9NIL9PI3S36mWNBaEeo ah4lGMj7cH5tVmz3WMOF9j+DqRD6h68oJTMmQzK9Z+WSCWJOrSj2OoeUASV3EkuvCvTi SgN5pMMBcmtHClVUOGD3764r6M/CsxGFkOZtOO1PZxq9+8m+vT1NGB1ycG/d6V6BLyoE DpzwTLwGMAzLlZGZXQ5HQmn29svXalvM0fbyBN9anx8mcPa6KvxR/nLpFeJmUGP0LDQg QVsKf9wPEsE4lLkzFM/P7FMrnECigJGCieQoAgjcDTJwB9+n/eYGGrwBa18T5C0FKd79 S91Q== X-Gm-Message-State: AOAM531FqemluOOj6fzzurVgcP0SBSdVFHOLXMDmtCkwXcrYj9aMVKBy NHv/OpacIjEfxa6pAtqKwKFfOMvQ8JkYTA== X-Google-Smtp-Source: ABdhPJyW0gn2baNZvJM4Nqqwgx9i6bM0bvDSknOddsd7lCvooCD40UldeyDALUMopc6zlHEQHhaG/g== X-Received: by 2002:a05:6602:2f04:: with SMTP id q4mr2492058iow.94.1640113436216; Tue, 21 Dec 2021 11:03:56 -0800 (PST) Received: from mail-il1-f176.google.com (mail-il1-f176.google.com. [209.85.166.176]) by smtp.gmail.com with ESMTPSA id n5sm6731931ilm.87.2021.12.21.11.03.55 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 21 Dec 2021 11:03:56 -0800 (PST) Received: by mail-il1-f176.google.com with SMTP id l5so10947813ilv.7 for ; Tue, 21 Dec 2021 11:03:55 -0800 (PST) X-Received: by 2002:a05:6e02:20cb:: with SMTP id 11mr2464308ilq.163.1640113435352; Tue, 21 Dec 2021 11:03:55 -0800 (PST) MIME-Version: 1.0 References: <20211220185725.19519-1-pal@sandflow.com> In-Reply-To: From: Pierre-Anthony Lemieux Date: Tue, 21 Dec 2021 11:03:43 -0800 X-Gmail-Original-Message-ID: Message-ID: To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] [PATCH v13 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 Mon, Dec 20, 2021 at 12:29 PM Lynne wrote: > > 20 Dec 2021, 20:48 by pal@sandflow.com: > > > On Mon, Dec 20, 2021 at 11:19 AM Lynne wrote: > > > >> > >> 20 Dec 2021, 19:57 by 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: > >> > > >> > ffmpeg -assetmaps ,,... -i > >> > > >> > 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: > >> > > >> > - added libavformat/tests/imf to FATE > >> > > >> > MAINTAINERS | 1 + > >> > configure | 3 +- > >> > doc/demuxers.texi | 6 + > >> > libavformat/Makefile | 1 + > >> > libavformat/allformats.c | 1 + > >> > libavformat/imf.h | 207 +++++++++ > >> > libavformat/imf_cpl.c | 800 +++++++++++++++++++++++++++++++++++ > >> > libavformat/imfdec.c | 891 +++++++++++++++++++++++++++++++++++++++ > >> > 8 files changed, 1909 insertions(+), 1 deletion(-) > >> > create mode 100644 libavformat/imf.h > >> > create mode 100644 libavformat/imf_cpl.c > >> > create mode 100644 libavformat/imfdec.c > >> > > >> > >> You've once again gone back and completely ignored all coding style > >> issues I pointed out. > >> > > > > This was definitely not the intent, and I do not believe that *ignored > > all coding style* is accurate. For example, most of the suggestions > > you made at [1] on December 5 have been integrated, including: using > > ff_ for internal functions, using FF for structs, reducing > > line length, etc. > > > > It might be that some of the changes you suggested conflicted with > > changes that others suggested. > > > > No, I think the issue here is you didn't even bother reading the coding > style given how out of place your first patch looked. Now you're making > me waste time pointing out every single thing I can spot you've done wrong. > Please take it more seriously. I appreciate your time and understand the investment involved in onboarding new contributors. IMHO one approach to reducing the time needed to onboard contributors and making the process less-error prone is to automate code style enforcement -- perhaps building on the VIM config at [1]? FWIW, I have developed the clang-format config file at [2]. [1] https://www.ffmpeg.org/developer.html [2] https://github.com/sandflow/ffmpeg-imf/blob/e2f3520d8ed947c287b90b59ec691ec58bb1edb4/.clang-format Happy to collaborate on that going forward. > > > +static int parse_imf_asset_map_from_xml_dom(AVFormatContext *s, > > + xmlDocPtr doc, > > + IMFAssetLocatorMap *asset_map, > > + const char *base_url) > > We align function arguments to the start of the bracket. Addressed at v14. > > > + for (uint32_t i = 0; i < cpl->main_audio_track_count; i++) > > + if (memcmp(cpl->main_audio_tracks[i].base.id_uuid, uuid, sizeof(uuid)) == 0) { > > + vt = &cpl->main_audio_tracks[i]; > > + break; > > + } > > We wrap multi-line statements in blocks. We do not wrap single-line > statements in blocks. Addressed at v14. > > > + av_log(s, > > + AV_LOG_DEBUG, > > + "parsed IMF CPL: " FF_IMF_UUID_FORMAT "\n", > > + UID_ARG(c->cpl->id_uuid)); > > In case a function call is too long, we do not put each individual > argument separately on a new line, we break them up into what > makes sense. I suggest keeping them as-is since "what makes sense" is in the eye of the beholder. > > > + if (!asset->absolute_uri) { > > + return AVERROR(ENOMEM); > > + } > > Wrapped single-line statement. Addressed at v14. > > > +void ff_imf_cpl_free(FFIMFCPL *cpl) > > +{ > > + if (!cpl) > > + return; > > + xmlFree(cpl->content_title_utf8); > > + imf_marker_virtual_track_free(cpl->main_markers_track); > > + if (cpl->main_markers_track) > > + av_freep(&cpl->main_markers_track); > > + imf_trackfile_virtual_track_free(cpl->main_image_2d_track); > > + if (cpl->main_image_2d_track) > > + av_freep(&cpl->main_image_2d_track); > > + for (uint32_t i = 0; i < cpl->main_audio_track_count; i++) > > + imf_trackfile_virtual_track_free(&cpl->main_audio_tracks[i]); > > + if (cpl->main_audio_tracks) > > + av_freep(&cpl->main_audio_tracks); > > + av_freep(&cpl); > > +} > > Ever heard of newlines? I've seen them in other parts of your code, > but not here. Addressed at v14. I have gone through the patch and added line breaks to reduce code density. > > > +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}, > > +}; > > Take a look at how other code does options. The use of initializers was suggested by another reviewer [3]. [3] http://ffmpeg.org/pipermail/ffmpeg-devel/2021-December/289025.html > > > + if (av_cmp_q(av_add_q(track->current_timestamp, edit_unit_duration), > > + track->duration) > > + > 0) > > + return AVERROR_EOF; > > That's a very weird indentation. Addressed at v14. I have confirmed that the alignment matches the VIM config at [4]. [4] https://www.ffmpeg.org/developer.html > _______________________________________________ > 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".