[FFmpeg-devel] [PATCH v13 1/2] avformat/imf: Demuxer
Pierre-Anthony Lemieux
pal at sandflow.com
Tue Dec 21 21:03:43 EET 2021
On Mon, Dec 20, 2021 at 12:29 PM Lynne <dev at lynne.ee> wrote:
>
> 20 Dec 2021, 20:48 by pal at sandflow.com:
>
> > On Mon, Dec 20, 2021 at 11:19 AM Lynne <dev at lynne.ee> wrote:
> >
> >>
> >> 20 Dec 2021, 19:57 by pal at sandflow.com:
> >>
> >> > From: Pierre-Anthony Lemieux <pal at palemieux.com>
> >> >
> >> > Signed-off-by: Pierre-Anthony Lemieux <pal at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list