[FFmpeg-devel] [PATCH v4] Patch for memory optimization with QuickTime/MP4

[Moritz Barsnick]

On Mon, Dec 09, 2019 at 10:22:15 +0000, Jörg Beckmann wrote:

Just some formal stuff:

> Subject: Patch for memory optimization with QuickTime/MP4

This subject should be created by the actual patch, because, they way
you submitted it, it will be used for pushing.

Or you create your patch with "git format-patch" and attach it, then
the actual subject of the e-mail doesn't matter. Or "git send-email"
will format the subject for you, assuming you put the correct text into
your commit message:

That said, the commit message should be introduced with a one-liner
with a module prefix, such as:

avformat/mov: memory optimization with QuickTime/MP4

(I think it should be "for", not "with", but that's up to you in the
first step.)

Then an empty line, then the remaining text. No need to mention "patch"
anywhere in the commit message, because it's obvious that a commit
corresponds to a patch.

> The last patch mail seems to be lost in space. Therefore here the next try .
>
> Cheers,
> Jörg

If you write this text here, it will be part of the pushed commit
message. If you wish to add text to a patch sent with "git send-email",
please add your additional text below the three-dash separator:

> This patch invents a new option "discard_fragments" for the MP4/Quicktime/MOV decoder. If the option is not set, nothing changes at all. If it is set, old fragments are discarded as far as possible on each call to switch_root. For pure audio streams, the memory usage is now constant. For video streams, the memory usage is reduced. I've tested it with audio streams received from a professional DAB+ receiver and with video streams created on my own with "ffmpeg -i <video>.m4v -c:a:0 copy -c:v copy -c:s copy -f ismv -movflags frag_keyframe -movflags faststart tcp://localhost:1234?listen" and "ffmpeg -i tcp://localhost:1234 -c:a copy -c:v copy -c:s copy -y <output>".
>
> Signed-off-by: Jörg Beckmann <joerg.beckmann at scisys.com>
> ---

(Add your text in here.)

You should also kindly wrap your commit message at ~80 characters, or
perhaps 72.

> +        for (i = 0; i < mov->fc->nb_streams; i++) {
> +            if (mov->fc->streams[i]->id == frag->track_id) {
> +                st = mov->fc->streams[i];
> +                break;
> +            }
> +        }
> +
> +        av_assert0(st);

This can never happen? Or can it, with an illegally formatted file, but
shouldn't? In the latter case, you would need to error out with
"invalid data". (Just wondering, not critisizing.)

> +            case AVMEDIA_TYPE_SUBTITLE:
> +                /* Freeing VIDEO tables leads to corrupted video when writing to eg. MKV */
> +                av_freep(&st->index_entries);
> +                st->nb_index_entries = 0;

av_freep() NULLs for you.

> +                av_freep(&sc->ctts_data);
> +                sc->ctts_data = NULL;

Ditto.

> +    {"discard_fragments",
> +            "Discards fragments after they have been read to support live streams.",

Nit: I think these descriptions are imperatives, so drop the 's' from
"Discards".

Cheers,
Moritz