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

Michael Niedermayer michaelni
Sun Dec 10 18:53:00 CET 2006


Hi

On Sun, Dec 10, 2006 at 06:11:44AM -0800, Allan Hsu wrote:
> 
> On Dec 10, 2006, at 1:48 AM, Michael Niedermayer wrote:
> 
> >Hi
> >
> >On Sat, Dec 09, 2006 at 07:25:11PM -0800, Allan Hsu wrote:
> >[...]
> >
> >>>
> >>>>
> >>>>Fixed.
> >>>>
> >>>>[...]
> >>>>>>+    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"
> >>>>[...]
> >>>>
> >>>>I tried to implement your suggested changes to flv_read_header()  
> >>>>and
> >>>>flv_read_packet(), but it only made the backward-seeking issue  
> >>>>worse.
> >>>>Since metadata packets don't have a stream, they don't have a valid
> >>>>stream index and so the next audio/video packet is returned by
> >>>>flv_read_packet(), which makes the maximum length of a backward  
> >>>>seek
> >>>>in flv_read_header() quite large.
> >>>>
> >>>>Instead, I've opted to try and reduce the size of any backward
> >>>>seeks in
> >>>>the attached revision of the patch. Backward seeks will only  
> >>>>occur in
> >>>>the case that there is not enough metadata. In this case, if the
> >>>>first
> >>>>tag was a metadata tag, the backwards seek should go to the second
> >>>>packet, where flv_read_metabody() should have stopped, which should
> >>>>be effectively no movement. If the first tag wasn't a metadata tag,
> >>>>the backward seek should be 15 bytes (the size of the FLV tag  
> >>>>header
> >>>>read by flv_read_tag_header()).
> >>>
> >>>what about putting the inside of the for() loop from  
> >>>flv_read_packet()
> >>>into its own function and then calling that from flv_read_packet 
> >>>() and
> >>>flv_read_header()
> >>>that way FLVTagInfo and flv_read_tag_header() should become unneeded
> >>
> >>I've looked at this and I'm not sure if this would help all that
> >>much. There is a lot of logic inside the for() loop that wants to
> >>skip the current packet and try the next packet before exiting the
> >>loop. To keep that logic around, we would either have to leave it in
> >>the loop or push the looping behavior into the separate function.
> >>
> >>I tried a version that left the tag skipping logic in the loop, and
> >>the separate function ended up being almost identical to
> >>flv_read_tag_header() except with a special case for metadata tag
> >>handling. This function didn't make a lot of sense, functionality-
> >>wise, and it ended up passing a large amount of data back, which
> >>means FLVTagInfo never really went away. flv_read_packet() still
> >>requires this information after calling this function: body size, tag
> >>type, next_pos, and pts.
> >>
> >>I didn't try the version that pushed the looping into the separate
> >>function because it seemed like this function would have caused the
> >>same large backwards-seek issue we were trying to avoid in
> >>flv_read_header() or it  would have an odd set of responsibilities.
> >>In the metadata reading case, the function would:
> >>
> >>seek backwards if first packet is not metadata
> >>parse metadata otherwise, seek to second packet before returning.
> >>create streams if metadata is complete and valid.
> >>
> >>In the packet reading case:
> >>
> >>skip past any non-audio/video packets
> >>skip past any st->discard designated packets
> >>return information required by the rest of flv_read_packet(): body
> >>size, tag type, next tag position, tag pts, tag flags
> >>
> >>I think the problem here is that the largest amount of shared
> >>function between the two usages is that both flv_read_header() and
> >>flv_read_packet() need to know a minimal amount of data about the
> >>next tag in the stream before deciding what to do with them. This is
> >>what flv_read_tag_header() does. After this, flv_read_header() either
> >>processes the tag or seeks back to the beginning the tag.
> >>flv_read_packet(), however, skips tags until it finds one that's
> >>interesting. I'm at a loss as to how we could encapsulate these two
> >>kinds of operations into one sane-looking function.
> >
> >hmm, what about leaving the metadata reading in flv_read_packet() and
> >clearing AVFMTCTX_NOHEADER as soon as all needed data is available?
> 
> This would cause flv_read_header() to seek to the end of the stream  
> if !url_is_streamed() when it tries to determine duration, which  
> undesirable in my case, where I want to begin decoding of a partially- 
> downloaded FLV before the entire file is available. In this case, the  
> FLV isn't "streamed" in the non-seekable sense, but a seek and read  
> >from the end would block until that portion of the file is available.

see latest svn, i think ive solved this problem


[...]
> IMHO, this functionality should be the subject of a different series  
> of patches that also address these issues:
> 1) the FLV muxer does not currently write this metadata.
> 2) the FLV muxer is currently broken such that -vcodec copy produces  
> bad output for non-H263 video codecs (video tags are unconditionally  
> marked as H263).

yes fixes to the flv muxer should be in seperate patch(es) ...
still what good does most of the metadata do if not for stream copy?

[...]
> +
> +static int amf_parse_object(AVFormatContext *s, const char *key) {
> +    FLVDemuxContext *context;
> +    ByteIOContext *ioc;
> +    AMFDataType amf_type;
> +    char str_val[256];
> +    double num_val;
> +
> +    num_val = 0;
> +    context = s->priv_data;
> +    ioc = &s->pb;
> +
> +    amf_type = get_byte(ioc);
> +
> +    switch(amf_type) {
> +        case AMF_DATA_TYPE_NUMBER:
> +            num_val = av_int2dbl(get_be64(ioc)); break;
> +        case AMF_DATA_TYPE_BOOL:
> +            num_val = get_byte(ioc); break;
> +        case AMF_DATA_TYPE_STRING:
> +            if(amf_get_string(ioc, str_val, sizeof(str_val)) < 0)
> +                return -1;
> +            break;
> +        case AMF_DATA_TYPE_OBJECT: {
> +            unsigned int keylen;
> +
> +            while((keylen = get_be16(ioc))) {
> +                url_fskip(ioc, keylen); //skip key string
> +                if(amf_parse_object(s, NULL) < 0)
> +                    return -1; //if we couldn't skip, bomb out.
> +            }
> +            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_MIXEDARRAY:
> +            url_fskip(ioc, 4); //skip 32-bit max array index
> +            while(amf_get_string(ioc, str_val, sizeof(str_val)) > 0) {
> +                if(amf_parse_object(s, NULL) < 0)
> +                    return -1;
> +            }
> +            if(get_byte(ioc) != AMF_END_OF_OBJECT)
> +                return -1;
> +            break;
> +        case AMF_DATA_TYPE_ARRAY: {
> +            unsigned int arraylen, i;
> +
> +            arraylen = get_be32(ioc);
> +            for(i = 0; i < arraylen; i++) {
> +                if(amf_parse_object(s, NULL) < 0)
> +                    return -1; //if we couldn't skip, bomb out.
> +            }
> +        }
> +            break;
> +        case AMF_DATA_TYPE_DATE:
> +            url_fskip(ioc, 8 + 2); //timestamp (double) and UTC offset (int16)
> +            break;
> +        default: //unsupported type, we couldn't skip
> +            return -1;
> +    }
> +
> +    if(key) { //only look for values for the context when key != NULL
> +        if(amf_type == AMF_DATA_TYPE_BOOL) {
> +            if(!strcmp(key, "stereo"))               context->is_stereo    = num_val;
> +        } else if(amf_type == AMF_DATA_TYPE_NUMBER) {
> +            if(!strcmp(key, "duration"))             s->duration           = num_val * AV_TIME_BASE;
> +            else if(!strcmp(key, "width"))           context->width        = num_val;
> +            else if(!strcmp(key, "height"))          context->height       = num_val;
> +            else if(!strcmp(key, "audiocodecid"))    context->audiocodecid = num_val;
> +            else if(!strcmp(key, "videocodecid"))    context->videocodecid = num_val;
> +            else if(!strcmp(key, "audiosamplerate")) context->samplerate   = num_val;
> +            else if(!strcmp(key, "audiosamplesize")) context->samplesize   = num_val;

IMHO these should be set directly in AVCodecContext without the intermediate
FLVDemuxContext layer


> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static int flv_read_tag_header(ByteIOContext *ioc, FLVTagInfo *info) {
> +    info->pos = url_ftell(ioc);
> +    info->prev_tag_size = get_be32(ioc);
> +    info->type = get_byte(ioc);
> +    info->body_size = get_be24(ioc);
> +    info->pts = get_be24(ioc);
> +//    av_log(s, AV_LOG_DEBUG, "type:%d, size:%d, pts:%d\n", info->type, info->body_size, info->pts);
> +
> +    if(url_feof(ioc))
> +        return AVERROR_IO;
> +
> +    url_fskip(ioc, 4); /* reserved */
> +
> +    info->next_pos = info->body_size + url_ftell(ioc);
> +
> +    return 0;
> +}

i hope this function finally became unneeded after my latest changes in svn?


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

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus




More information about the ffmpeg-devel mailing list