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

[Lynne <dev@lynne.ee>]

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.

> +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.

> +    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.

> +    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.

> +        if (!asset->absolute_uri) {
> +            return AVERROR(ENOMEM);
> +        }

Wrapped single-line statement.

> +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.

> +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.

> +        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.
_______________________________________________
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".