[FFmpeg-devel] [PATCH] avformat/mov: initial support for emsg box

[James Almer]

On 4/20/2021 1:01 AM, Brad Hards wrote:
> This adds support for the EventMessage box, which is apparently described in
> ISO/IEC 23009-1:2019, and is used for KLV metadata per MISB ST 1910.
> See https://www.gwg.nga.mil/misb/docs/standards/ST1910.1.pdf Sect 6.2.1
> ---
>   libavformat/isom.h | 15 ++++++++++++
>   libavformat/mov.c  | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 76 insertions(+)
> 
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index 5a6d504090..8ea62e91f0 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -79,6 +79,18 @@ typedef struct MOVDref {
>       int16_t nlvl_to, nlvl_from;
>   } MOVDref;
>   
> +typedef struct MOVEmsg {
> +    uint8_t version;
> +    uint32_t timescale;
> +    uint32_t presentation_time_delta; ///< only valid for version 0
> +    uint64_t presentation_time; ///< only valid for version 1
> +    uint32_t event_duration;
> +    uint32_t id;
> +    char *scheme_id_uri;
> +    char *value;
> +    uint8_t *message_data;
> +} MOVEmsg;
> +
>   typedef struct MOVAtom {
>       uint32_t type;
>       int64_t size; /* total size (excluding the size and type fields) */
> @@ -297,6 +309,9 @@ typedef struct MOVContext {
>       int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
>       int have_read_mfra_size;
>       uint32_t mfra_size;
> +    // Event Messages from DASH specification, used in MISB ST 1910
> +    unsigned emsg_count;  ///< Number of event messages
> +    MOVEmsg *emsg_data; ///< Array of Event Messages

You're not freeing this array anywhere. Same with each entry's 
message_data, scheme_id_uri and value.

>   } MOVContext;
>   
>   int ff_mp4_read_descr_len(AVIOContext *pb);
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 7805330bf9..22bf0b36ea 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -5318,6 +5318,66 @@ static int mov_read_elst(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>       return 0;
>   }
>   
> +static int mov_read_emsg(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> +{
> +    int err;
> +    uint32_t bytes_read_from_atom = 0;
> +    size_t num_bytes_in_last_read;
> +    MOVEmsg *emsg;
> +
> +    if ((uint64_t)c->emsg_count+1 >= UINT_MAX / sizeof(*c->emsg_data))
> +        return AVERROR_INVALIDDATA;
> +    if ((err = av_reallocp_array(&c->emsg_data, c->emsg_count + 1,

If there can be several emsg atoms in a bitstream, it may be a better 
idea to use av_fast_realloc() instead.

Also, no limit to their lifetime? Being events, one would expect they 
could be dropped after being handled.

> +                                 sizeof(*c->emsg_data))) < 0) {
> +        c->emsg_count = 0;
> +        return err;
> +    }
> +    emsg = &c->emsg_data[c->emsg_count++];
> +
> +    emsg->version = avio_r8(pb);
> +    bytes_read_from_atom += 1;
> +    avio_rb24(pb); /* flags */
> +    bytes_read_from_atom += 3;
> +    if (emsg->version == 0) {
> +        emsg->scheme_id_uri = av_malloc(atom.size + 1);
> +        num_bytes_in_last_read = avio_get_str(pb, atom.size, emsg->scheme_id_uri, pb->buffer_size + 1);
> +        bytes_read_from_atom += num_bytes_in_last_read;
> +        av_realloc(emsg->scheme_id_uri, num_bytes_in_last_read);
> +        emsg->value = av_malloc(atom.size + 1);
> +        num_bytes_in_last_read = avio_get_str(pb, atom.size, emsg->value, pb->buffer_size + 1);
> +        bytes_read_from_atom += num_bytes_in_last_read;
> +        av_realloc(emsg->value, num_bytes_in_last_read);
> +        emsg->timescale = avio_rb32(pb);
> +        bytes_read_from_atom += 4;
> +        emsg->presentation_time_delta = avio_rb32(pb);
> +        bytes_read_from_atom += 4;
> +        emsg->event_duration = avio_rb32(pb);
> +        bytes_read_from_atom += 4;
> +        emsg->id = avio_rb32(pb);
> +        bytes_read_from_atom += 4;
> +    } else if (emsg->version == 1) {
> +        emsg->timescale = avio_rb32(pb);
> +        bytes_read_from_atom += 4;
> +        emsg->presentation_time = avio_rb64(pb);
> +        bytes_read_from_atom += 8;
> +        emsg->event_duration = avio_rb32(pb);
> +        bytes_read_from_atom += 4;
> +        emsg->id = avio_rb32(pb);
> +        bytes_read_from_atom += 4;
> +        emsg->scheme_id_uri = av_malloc(atom.size + 1);
> +        num_bytes_in_last_read = avio_get_str(pb, atom.size, emsg->scheme_id_uri, pb->buffer_size + 1);
> +        bytes_read_from_atom += num_bytes_in_last_read;
> +        av_realloc(emsg->scheme_id_uri, num_bytes_in_last_read);
> +        emsg->value = av_malloc(atom.size + 1);
> +        num_bytes_in_last_read = avio_get_str(pb, atom.size, emsg->value, pb->buffer_size + 1);
> +        bytes_read_from_atom += num_bytes_in_last_read;

Use avio_tell() instead of keeping a byte count with this 
bytes_read_from_atom variable. As is this is pretty unreadable.

> +        av_realloc(emsg->value, num_bytes_in_last_read);
> +    }
> +    emsg->message_data = av_malloc(atom.size - bytes_read_from_atom);
> +    num_bytes_in_last_read = avio_read(pb, emsg->message_data, atom.size - bytes_read_from_atom);
> +    return 0;

You're not doing anything with these event messages. You're only storing 
them in an ever growing array.

> +}
> +
>   static int mov_read_tmcd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>   {
>       MOVStreamContext *sc;
> @@ -6895,6 +6955,7 @@ static const MOVParseTableEntry mov_default_parse_table[] = {
>   { MKTAG('d','r','e','f'), mov_read_dref },
>   { MKTAG('e','d','t','s'), mov_read_default },
>   { MKTAG('e','l','s','t'), mov_read_elst },
> +{ MKTAG('e','m','s','g'), mov_read_emsg },
>   { MKTAG('e','n','d','a'), mov_read_enda },
>   { MKTAG('f','i','e','l'), mov_read_fiel },
>   { MKTAG('a','d','r','m'), mov_read_adrm },
>