[FFmpeg-devel] [PATCH] support for flvtool2 "keyframes based" generated index in FLV format decoder

[Vladimir Pantelic]

Kharkov Alexander wrote:
>>  Could you rewrite the patch to read all the index stuff in one single
>>  function, that should allow you to drop a lot of your temporary
>>  variables and flags and will also make it much easier to understand.
> Ok, done. Patch rewritten now 'keyframes' based index filled in single function.
> 'keyframes' parser have strict requirement on structure of this part
> of metadata, if structure differ from required, parser just does not
> use such data for indexing purpose.

ok

> For backward compatible behavior, old parser executed too. New patch attached.

why do that? I can understand to seek back to the old position if the
index parsing gave errors, but in case of success why read the
same data again?

> Index: libavformat/flvdec.c
> ===================================================================
> --- libavformat/flvdec.c	(revision 26402)
> +++ libavformat/flvdec.c	(working copy)
> @@ -30,6 +30,10 @@
>   #include "avformat.h"
>   #include "flv.h"
>
> +#define METADATA_KEYFRAMES_TAG "keyframes"
> +#define METADATA_KEYFRAMES_TIMESTAMP_TAG "times"
> +#define METADATA_KEYFRAMES_BYTEOFFSET_TAG "filepositions"
> +
>   typedef struct {
>       int wrong_dts; ///<  wrong dts due to negative cts
>   } FLVContext;
> @@ -124,6 +128,64 @@
>       return length;
>   }
>
> +static void try_parse_keyframes_index(AVFormatContext *s, ByteIOContext *ioc, AVStream *vstream, int64_t max_pos) {
> +    unsigned int keylen, arraylen = 0, timeslen = 0, fileposlen = 0, i;
> +    double num_val;
> +    AMFDataType amf_type;
> +    char str_val[256];
> +    int64_t** current_array;

pointer to pointer not needed, just make current point to whatever times or pos point to.
also current_array is local to the loop below, no?


> +    int64_t *times = 0;
> +    int64_t *filepositions = 0;
> +
> +    while (url_ftell(ioc)<  max_pos - 2&&  (keylen = get_be16(ioc))) {
> +        // Read array name from context
> +        get_buffer(ioc, str_val, keylen);
> +        str_val[keylen] = '\0';
> +
> +        // Expect array object in context
> +        amf_type = get_byte(ioc);
> +        if (amf_type != AMF_DATA_TYPE_ARRAY)
> +            return;
> +
> +        arraylen = get_be32(ioc);
> +        /*
> +         * Expect only 'times' or 'filepositions' sub-arrays in other case refuse to use such metadata
> +         * for indexing
> +         */
> +        if (!strcmp(METADATA_KEYFRAMES_TIMESTAMP_TAG, str_val)) {
> +            times = av_mallocz(sizeof(int64_t) * arraylen);
> +            timeslen = arraylen;
> +            current_array =×
> +        } else if (!strcmp(METADATA_KEYFRAMES_BYTEOFFSET_TAG, str_val)) {
> +            filepositions = av_mallocz(sizeof(int64_t) * arraylen);
> +            fileposlen = arraylen;
> +            current_array =&filepositions;
> +        } else
> +            // unexpected metatag inside keyframes, will not use such metadata for indexing
> +            return;
> +
> +        for (i = 0; i<  arraylen&&  url_ftell(ioc)<  max_pos - 1; i++) {

nit: please give operators room the breathe and drop the double spaces

> +            amf_type = get_byte(ioc);
> +            if (amf_type != AMF_DATA_TYPE_NUMBER)
> +                return;
> +            num_val = av_int2dbl(get_be64(ioc));
> +            (*current_array)[i] = num_val;
> +        }
> +    }
> +    if (timeslen != fileposlen)
> +        // times->filepositions arrays have different size, will not use such metadata for indexing

obvious, no?

> +        return;
> +
> +    for (i = 0; i<  arraylen; ++i) {
> +        int64_t ts = times[i];
> +        int64_t byte_offset = filepositions[i];

why these temp vars?

> +        av_add_index_entry(vstream, byte_offset, ts*1000, 0, 0, AVINDEX_KEYFRAME);
> +    }
> +
> +    av_freep(&times);
> +    av_freep(&filepositions);
> +}
> +
>   static int amf_parse_object(AVFormatContext *s, AVStream *astream, AVStream *vstream, const char *key, int64_t max_pos, int depth) {
>       AVCodecContext *acodec, *vcodec;
>       ByteIOContext *ioc;
> @@ -148,6 +210,17 @@
>           case AMF_DATA_TYPE_OBJECT: {
>               unsigned int keylen;
>
> +            if (!strcmp(METADATA_KEYFRAMES_TAG, key)&&  depth == 1) {
> +                int64_t cur_ioc_pos = url_ftell(ioc);
> +                /*
> +                 * Try parse keyframes metatag and fill AV index.
> +                 * Reset stream to initial state after that to run default parser
> +                 * to not break default parser behavior
> +                 */
> +                try_parse_keyframes_index(s, ioc, vstream, max_pos);
> +                url_fseek(ioc, cur_ioc_pos, SEEK_SET);

as said, i'd prefer to not seek back in case of success

> +            }
> +
>               while(url_ftell(ioc)<  max_pos - 2&&  (keylen = get_be16(ioc))) {
>                   url_fskip(ioc, keylen); //skip key string
>                   if(amf_parse_object(s, NULL, NULL, NULL, max_pos, depth + 1)<  0)

Regards,