[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