[FFmpeg-devel] Create and populate AVStream side data packet with contents of ISOBMFF edit list entries

[Andreas Rheinhardt]

Matthew Szatmary:
> Create and populate AVStream side data packet with contents of ISOBMFF
> edit list entries
> 
> Signed-off-by: Matthew Szatmary <matt at szatmary.org>
> ---
>  Changelog           |  1 +
>  libavcodec/packet.h | 14 ++++++++++++++
>  libavformat/mov.c   | 17 ++++++++++++++++-
>  3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/Changelog b/Changelog
> index c37ffa82e1..2d719dd3b1 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -9,6 +9,7 @@ version <next>:
>  - VDPAU accelerated HEVC 10/12bit decoding
>  - ADPCM IMA Ubisoft APM encoder
>  - Rayman 2 APM muxer
> +- AV_PKT_DATA_EDIT_LIST added to AVStream side_data
> 
> 
>  version 4.3:
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index 0a19a0eff3..5faa594cf5 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -290,6 +290,20 @@ enum AVPacketSideDataType {
>       */
>      AV_PKT_DATA_S12M_TIMECODE,
> 
> +    /**
> +     * ISO media file edit list side data packet
> +     * The structure is repeated for each entry in the edit list
> +     * The number of entries can be calculated
> +     * by dividing the packet size by the entry size
> +     * Each entry is 20 bytes and is laid out as follows:
> +     * @code
> +     * s64le     duration
> +     * s64le     time
> +     * float32le rate

You are obviously copying the MOVElst structure; yet the rate is a 16.16
fixed point number in the file and not a float, so one should probably
use this.

> +     * @endcode
> +     */
> +    AV_PKT_DATA_EDIT_LIST,
> +
>      /**
>       * The number of side data types.
>       * This is not part of the public API/ABI in the sense that it may
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index d16840f3df..bb2c940e80 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4317,7 +4317,6 @@ static int mov_read_trak(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
>      av_freep(&sc->keyframes);
>      av_freep(&sc->stts_data);
>      av_freep(&sc->stps_data);
> -    av_freep(&sc->elst_data);

This is still needed, namely if an error happens before you can attach
the stream side-data. E.g. if an invalid edit list is found and
standards compliance is set to strict. Or if av_stream_new_side_data()
fails.

>      av_freep(&sc->rap_group);
> 
>      return 0;
> @@ -7662,6 +7661,22 @@ static int mov_read_header(AVFormatContext *s)
>          AVStream *st = s->streams[i];
>          MOVStreamContext *sc = st->priv_data;
> 
> +        if (sc->elst_data) {
> +            uint8_t *elst_data;
> +            elst_data = av_stream_new_side_data(st,
> AV_PKT_DATA_EDIT_LIST, sc->elst_count * 20);

I wonder whether it would be advantageouos to use
av_stream_add_side_data() here.

> +
> +            if (!elst_data)
> +                goto fail;
> +
> +            for (j = 0; j < sc->elst_count; j++) {
> +                AV_WB64((elst_data+(j*20)), sc->elst_data[j].duration);
> +                AV_WB64((elst_data+(j*20)+8), sc->elst_data[j].time);

"WB" stands for "Write Big-endian", yet your documentation says that it
is supposed to be little-endian.

> +                AV_WB32((elst_data+(j*20)+16), sc->elst_data[j].rate);
> +            }
> +
> +            av_freep(&sc->elst_data);
> +        }
> +
>          switch (st->codecpar->codec_type) {
>          case AVMEDIA_TYPE_AUDIO:
>              err = ff_replaygain_export(st, s->metadata);
>