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

[Kharkov Alexander]

On 10 February 2011 01:15, Anton Khirnov <anton at khirnov.net> wrote:
>> 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,63 @@
>> ? ? ?return length;
>> ?}
>>
>> +static int try_parse_keyframes_index(AVFormatContext *s, ByteIOContext *ioc, AVStream *vstream, int64_t max_pos) {
>
> nit: the 'try_' part carries no information, get rid of it.
>
>> + ? ?unsigned int keylen, arraylen = 0, timeslen = 0, fileposlen = 0, i;
>> + ? ?double num_val;
>> + ? ?AMFDataType amf_type;
>> + ? ?char str_val[256];
>> + ? ?int64_t *times = 0;
>> + ? ?int64_t *filepositions = 0;
>
> You mean NULL
>
>> +
>> + ? ?while (url_ftell(ioc) < max_pos - 2 && (keylen = get_be16(ioc))) {
>> + ? ? ? ?int64_t* current_array;
>> + ? ? ? ?// Read array name from context
>> + ? ? ? ?get_buffer(ioc, str_val, keylen);
>> + ? ? ? ?str_val[keylen] = '\0';
>
> Buffer overflow.
>
>> +
>> + ? ? ? ?// Expect array object in context
>> + ? ? ? ?amf_type = get_byte(ioc);
>> + ? ? ? ?if (amf_type != AMF_DATA_TYPE_ARRAY)
>
> This value isn't used anywhere else, so you can use if (get_byte() ...)
> and get rid of amf_type. Same below.
>
>> + ? ? ? ? ? ?return -1;
>> +
>> + ? ? ? ?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);
>
> Use sizeof(*times). Also you should check that malloc succeeded (it
> might easily fail with a corrupted file).
>
>> + ? ? ? ? ? ?timeslen = arraylen;
>> + ? ? ? ? ? ?current_array = times;
>> + ? ? ? ?} 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 -1;
>> +
>> + ? ? ? ?for(i = 0; i < arraylen && url_ftell(ioc) < max_pos - 1; i++) {
>> + ? ? ? ? ? ?amf_type = get_byte(ioc);
>> + ? ? ? ? ? ?if (amf_type != AMF_DATA_TYPE_NUMBER)
>> + ? ? ? ? ? ? ? ?return -1;
>
> Memleak. Same below.
>
>> + ? ? ? ? ? ?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
>> + ? ? ? ?return -1;
>> +
>> + ? ?for(i = 0; i < arraylen; ++i) {
>> + ? ? ? ?av_add_index_entry(vstream, filepositions[i], times[i]*1000, 0, 0, AVINDEX_KEYFRAME);
>> + ? ?}
>> +
>> + ? ?av_freep(&times);
>> + ? ?av_freep(&filepositions);
>> + ? ?return 0;
>> +}
>> +
>> ?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 +209,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
>> + ? ? ? ? ? ? ? ? */
>> + ? ? ? ? ? ? ? ?if(try_parse_keyframes_index(s, ioc, vstream, max_pos) < 0)
>> + ? ? ? ? ? ? ? ? ? ?url_fseek(ioc, cur_ioc_pos, SEEK_SET);
>> + ? ? ? ? ? ?}
>> +
>> ? ? ? ? ? ? ?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)
>
> --
> Anton Khirnov
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk1S57cACgkQtQoSQcBnB6sR7ACdFEYox18J3IzMMeQP22yIs1Sa
> ci0AnR2e/SRMmgQBqirI/8x9ZpN1j2j2
> =eGpz
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>

All pointed issues resolved. New patch attached. Thank you for review.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: flvdec.c.diff
Type: text/x-patch
Size: 4820 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110214/dc080dad/attachment.bin>