On Mon, Dec 20, 2021 at 12:29 PM Lynne <d...@lynne.ee> wrote: > > 20 Dec 2021, 20:48 by p...@sandflow.com: > > > On Mon, Dec 20, 2021 at 11:19 AM Lynne <d...@lynne.ee> wrote: > > > >> > >> 20 Dec 2021, 19:57 by p...@sandflow.com: > >> > >> > From: Pierre-Anthony Lemieux <p...@palemieux.com> > >> > > >> > Signed-off-by: Pierre-Anthony Lemieux <p...@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: > >> > > >> > - 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_<name> for internal functions, using FF<name> 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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
