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

[Pierre-Anthony Lemieux <pal@sandflow.com>]

On Mon, Dec 20, 2021 at 12:29 PM Lynne <dev@lynne.ee> wrote:
>
> 20 Dec 2021, 20:48 by pal@sandflow.com:
>
> > On Mon, Dec 20, 2021 at 11:19 AM Lynne <dev@lynne.ee> wrote:
> >
> >>
> >> 20 Dec 2021, 19:57 by pal@sandflow.com:
> >>
> >> > From: Pierre-Anthony Lemieux <pal@palemieux.com>
> >> >
> >> > Signed-off-by: Pierre-Anthony Lemieux <pal@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-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".