[FFmpeg-devel] IMF demuxer ping

[Pierre-Anthony Lemieux]

On Sun, Dec 5, 2021 at 2:13 AM Lynne <dev at lynne.ee> wrote:
>
> 5 Dec 2021, 02:33 by pal at sandflow.com:
>
> > Hi all,
> >
> > Quick ping re: libavformat/imf demuxer patch set.
> >
> > All outstanding feedback (thanks!) has been addressed as far as I know.
> >
> >  What are the next steps?
> >
> > It would be good to make progress while it is fresh in peoples' minds.
> >
> > Latest patchset at [1] and tracking PR at [2] (including memory leak checking):
> >
> > [1] http://ffmpeg.org/pipermail/ffmpeg-devel/2021-November/288173.html
> > [2] https://github.com/sandflow/ffmpeg-imf/pull/74
> >
> > Looking forward to (finally) adding support for IMF demuxing to ffmpeg.
> >
>
> There are still many issues.
> Put your custom header beneath the standard ffmpeg header.

Ok. See v7 of the patchset.

> Or
> remove it altogether if you're allowed to.
> You're not following our code style at all:
> All internal functions (except intra-file functions) must have
> an ff_<name> prefix. If it's a struct, the prefix is FF<name>.

Ok. See v7 of the patchset.

> We do not put brackets on one-line statements, unless it's
> as a part of an if {} else { } group where one statement has
> more than 2 lines.

Ok. See v7 of the patchset.

> We're lenient towards 80-char line limits, where if it looks bad
> and hampers readability, we let it go on, but the patches aren't
> even trying.

Ok. See v7 of the patchset. Some lines are slightly over 80 columns.

> It's all here:
> https://ffmpeg.org/developer.html#Coding-Rules-1
>
> "rsrc->edit_rate = av_make_q(0, 0);" <- this means that if
> something goes wrong, there will be a division by 0. Timebases
> must be initialized by av_make_q(0, 1);

Ok. See v7 of the patchset.

>
> Could you try to reduce the total number of allocations per-packet
> by allocating all that you can during init-time, even if it's potentially
> unused, and use av_fast_realloc for really necessary allocations during
> packet parsing? That way, decoding would be quicker.

Ok. See v7 of the patchset. av_realloc_f() is replaced by
av_fast_realloc() when allocating Composition Resources and Asset Map
Locators, which are the most likely to cause a performance hit with
large IMF Compositions that contain hundreds of underlying track
files.

>
> Squash the first 3 patches (header, processor and demuxer) patches
> together, and move the build stuff from the final patch in with the
> first, only keeping the Makefile code for building the tests into the 4th.
> Otherwise, you can't build them.

Ok. See v7 of the patchset.

>
> Remove the "@author" and "@file" or other special documentation
> commands, doxygen doesn't even use them. Use the copyright
> field on top.

Copyright and authorship are distinct. I think it is a good idea to
give credit to primary authors/contributors, regardless on who holds
the copyright.

The use of @author seems widespread in the codebase and is mentioned
in the documentation at [1].

[1] https://ffmpeg.org/developer.html#Coding-Rules-1

Perhaps there is an alternative to @author if @author is now deprecated?

>
> Since IMF Is meant for intermediate work, it does allow for weird
> non-linear streams with loops, skips and such. We do (or attempt
> to) do linearization inside the demuxer, due to bad choices, unless
> an option is set, like with mp4/mov. So this demuxer should do
> that as well.

Do you mean `-ignore_editlist`?

An IMF Composition consists of a standalone playlist (the Composition
Playlist) that references MXF files (the Track Files). It is not
possible to process the Track Files without the Composition Playlist,
i.e. the edit list.

What scenario(s) do you have in mind?

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