[Ffmpeg-devel] [PATCH] FLV decoder metadata reading

[Michael Niedermayer]

Hi

On Fri, Dec 08, 2006 at 12:41:32AM -0800, Allan Hsu wrote:
> This is my second try at the FLV decoder metadata patch. I've tried to
> adhere to the submission guidelines and Michael's suggestions. Please
> let me know if this patch is acceptable or if I still need to do
> anything before it can be merged in SVN.
> 
> 	-Allan
> -- 
> Allan Hsu <allan at counterpop dot net>
> 1E64 E20F 34D9 CBA7 1300 1457 AC37 CBBB 0E92 C779

> Index: libavformat/flvdec.c
> ===================================================================
> --- libavformat/flvdec.c	(revision 7260)
> +++ libavformat/flvdec.c	(working copy)
> @@ -27,6 +27,317 @@
>  #include "avformat.h"
>  #include "flv.h"
>  
> +typedef struct {
> +    int hasVideo;
> +    int hasAudio;
> +    int videocodecid;
> +    int audiocodecid;
> +    int width;
> +    int height;
> +    int samplerate;
> +    int samplesize;
> +    int isStereo;
> +} FLVDemuxContext;
> +
> +typedef struct {
> +    unsigned int pos;
> +    unsigned int prev_tag_size;
> +    unsigned int next_pos;
> +    int type;
> +    int body_size;
> +    int pts;
> +} FLVTagInfo;
> +
> +inline static void create_vp6_extradata(AVStream *stream) {
> +    if(stream->codec->extradata_size != 1) {
> +        stream->codec->extradata_size = 1;
> +        stream->codec->extradata = av_malloc(1);
> +    }
> +}
> +
> +static int amf_skip_object(ByteIOContext *ioc, AMFDataType *type) {
> +    AMFDataType objectType;
> +
> +    objectType = (type != NULL ? *type : get_byte(ioc));
> +    switch(objectType) {
> +        case      AMF_DATA_TYPE_NUMBER:
> +            url_fskip(ioc, 8); break; //double precision float
> +        case        AMF_DATA_TYPE_BOOL:
> +            url_fskip(ioc, 1); break; //byte
> +        case      AMF_DATA_TYPE_STRING:
> +            url_fskip(ioc, get_be16(ioc)); break;
> +        case      AMF_DATA_TYPE_OBJECT: {
> +            unsigned int keylen;
> +
> +            for(keylen = get_be16(ioc); keylen != 0; keylen = get_be16(ioc)) {
> +                url_fskip(ioc, keylen); //skip key string
> +                if(amf_skip_object(ioc, NULL) < 0) //skip the following object
> +                    return -1; //if we couldn't skip, bomb out.
> +            }

while(keylen = get_be16(ioc)){
}

is slightly simpler (no keylen = get_be16(ioc) duplication)


> +            if(get_byte(ioc) != AMF_END_OF_OBJECT)
> +                return -1;
> +        }
> +            break;
> +        case        AMF_DATA_TYPE_NULL:
> +        case   AMF_DATA_TYPE_UNDEFINED:
> +        case AMF_DATA_TYPE_UNSUPPORTED:
> +            break; //these take up no additional space
> +        case       AMF_DATA_TYPE_ARRAY: {
> +            unsigned int arraylen, i;
> +
> +            arraylen = get_be32(ioc);
> +            for(i = 0; i < arraylen; i++)
> +                amf_skip_object(ioc, NULL);
> +        }
> +            break;
> +        case        AMF_DATA_TYPE_DATE:
> +            url_fskip(ioc, 8 + 2); //timestamp (double) and UTC offset (int16)
> +            break;
> +        default: //unsupported, we couldn't skip.
> +            return -1;
> +    }
> +
> +    return 0;
> +}

is amf_skip_object() really needed? isnt a simple loop which reads
double/string/bool/date and either assigns it to something or not
depending on what it was enough?


> +
> +static int amf_get_object(ByteIOContext *ioc, AMFDataType type, void *dest) {
> +    AMFDataType actualType = get_byte(ioc);
> +
> +    if(actualType != type) {
> +        //type was not the one we expected; skip object, don't touch dest, return error.
> +        amf_skip_object(ioc, &actualType);
> +        return -1;
> +    }
> +
> +    //we currently only need these two types for metadata parsing.
> +    switch(type) {
> +        case AMF_DATA_TYPE_NUMBER:
> +            *(double *)dest = av_int2dbl(get_be64(ioc));
> +            break;
> +        case   AMF_DATA_TYPE_BOOL:
> +            *(unsigned char *)dest = get_byte(ioc);
> +            break;
> +        default:
> +            return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int amf_get_string(ByteIOContext *ioc, char *buffer, int buffsize) {
> +    int length;
> +
> +    length = get_be16(ioc);
> +    if(length > buffsize)
> +        return -1; //string will not fit in buffer
> +
> +    get_buffer(ioc, buffer, length);
> +
> +    buffer[length] = '\0';
> +
> +    return length;
> +}

shouldnt it rather be?

if(length >= buffsize){
    url_fskip(ioc, length);
    return -1;
}

[...]

>  static int flv_read_header(AVFormatContext *s,
>                             AVFormatParameters *ap)
>  {
> +    FLVTagInfo taginfo;
> +    FLVDemuxContext *context = s->priv_data;
>      int offset, flags, size;
>  
> -    s->ctx_flags |= AVFMTCTX_NOHEADER; //ok we have a header but theres no fps, codec type, sample_rate, ...
> -
>      url_fskip(&s->pb, 4);
>      flags = get_byte(&s->pb);
>  
>      offset = get_be32(&s->pb);
>  
> +    if(flags & FLV_HEADER_FLAG_HASVIDEO)
> +        context->hasVideo = 1;
> +    if(flags & FLV_HEADER_FLAG_HASAUDIO)
> +        context->hasAudio = 1;
> +
> +    //0 is a valid audio codec id, so set it to something that will cause a validation error if it does not get set in flv_read_metabody
> +    context->audiocodecid = -1;
> +
> +    if(   flv_read_tag_header(&s->pb, &taginfo)  < 0 || taginfo.type != FLV_TAG_TYPE_META
> +       || flv_read_metabody(s, taginfo.next_pos) < 0 || flv_validate_context(s) < 0
> +       || flv_create_streams(s) < 0) {
> +        //error reading first tag header, first tag is not metadata, or metadata incomplete.
> +        s->ctx_flags |= AVFMTCTX_NOHEADER;
> +

isnt it simpler to simple read all the metadata in flv_read_packet() and just
call flv_read_packet() from flv_read_header() once if needed?



>      if(!url_is_streamed(&s->pb)){
>          const int fsize= url_fsize(&s->pb);
>          url_fseek(&s->pb, fsize-4, SEEK_SET);
> @@ -62,7 +387,8 @@
>          }
>      }
>  
> -    url_fseek(&s->pb, offset, SEEK_SET);
> +        url_fseek(&s->pb, offset, SEEK_SET);

this seeks backward or? if so its a matter of luck if it works, if theres
too much metadata then it will fail if the stream is not "seekable"

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali