[Ffmpeg-devel] [PATCH] FLV decoder metadata reading
Michael Niedermayer
michaelni
Sun Dec 10 10:48:50 CET 2006
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?
>
> [...]
> >>+static int flv_validate_context(AVFormatContext *s) {
> >>+ FLVDemuxContext *context = s->priv_data;
> >>+
> >>+ //if any values do not validate, assume metadata tool was
> >>brain dead and fail.
> >>+ if(s->duration <= 0)
> >>+ return -1;
> >>+
> >>+ if(context->has_audio) {
> >>+ switch(context->audiocodecid << FLV_AUDIO_CODECID_OFFSET) {
> >>+ case FLV_CODECID_PCM_BE:
> >>+ case FLV_CODECID_ADPCM:
> >>+ case FLV_CODECID_MP3:
> >>+ case FLV_CODECID_PCM_LE:
> >>+ case FLV_CODECID_NELLYMOSER_8HZ_MONO:
> >>+ case FLV_CODECID_NELLYMOSER:
> >>+ break;
> >>+ default:
> >>+ return -1;
> >
> >whats invalid if the codec id is not one of these?
> >same for the samplerate and video?
>
> I'm not sure what you're asking here? If the values in the metadata
> tag aren't valid values for what they describe, either the metadata
> tag is supplying bogus data (and we should ignore it and hope we can
> read the stream the old-fashioned way) or some new capabilities have
> been added to the FLV spec and our decoder needs to be updated:).
> I've added some logging lines on the case of failed validation.
well my reasoning is that a unknown value in the codec tag does not render
all other values invalid, the samplerate or width and height still likely
are correct and ffmpeg -vcodec copy should be able to just copy such a
stream between 2 flv files, for that of course the width/height/samplerate
must be available otherwise at least the metadata would dissapear ...
[...]
> +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_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;
this thing is missing the AMF_DATA_TYPE_MIXEDARRAY i think?
case AMF_DATA_TYPE_MIXEDARRAY:
arraylen = get_be32(ioc);
if(!strcmp(key, "onMetaData"))
arraylen= INT_MAX;
while(arraylen-- && url_ftell(ioc) < end - 3){ //or i=0; i<arraylen && ...; i++
if(amf_get_string(ioc, str_val, sizeof(str_val) < 0)
return -1;
if(amf_parse_object(s, str_val, end) < 0)
return -1;
}
[...]
> +static int flv_read_metabody(AVFormatContext *s, unsigned int next_pos) {
> + AMFDataType type;
> + ByteIOContext *ioc;
> + int keylen;
> + unsigned int itemcount;
> + char buffer[256];
> +
> + keylen = 0;
> + ioc = &s->pb;
> +
> + //first object needs to be "onMetaData" string
> + type = get_byte(ioc);
> + if(type != AMF_DATA_TYPE_STRING || amf_get_string(ioc, buffer, sizeof(buffer)) < 0 || strcmp(buffer, "onMetaData") != 0)
> + goto bail;
> +
> + //second object needs to be a mixedarray
> + type = get_byte(ioc);
> + if(type != AMF_DATA_TYPE_MIXEDARRAY)
> + goto bail;
> +
> + //this number has been known to misreport the number of items in the mixed array, so we don't use it.
> + itemcount = get_be32(ioc);
> +
> + while(url_ftell(ioc) < next_pos - 2 && (keylen = amf_get_string(ioc, buffer, sizeof(buffer))) > 0) {
> + if(amf_parse_object(s, buffer) < 0)
> + goto bail;
> + }
> +
> + if(keylen < 0 || get_byte(ioc) != AMF_END_OF_OBJECT)
> + goto bail;
> +
> + url_fseek(ioc, next_pos, SEEK_SET);
> + return 0;
> +
> +bail:
> + url_fseek(ioc, next_pos, SEEK_SET);
> + return -1;
> +}
putting url_fseek(ioc, next_pos, SEEK_SET); after both calls to
flv_read_metabody() would allow you to replace the goto bail by return -1
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The educated differ from the uneducated as much as the living from the
dead. -- Aristotle
More information about the ffmpeg-devel
mailing list