[Ffmpeg-devel] [PATCH] FLV decoder metadata reading
Michael Niedermayer
michaelni
Sat Dec 9 19:33:38 CET 2006
Hi
On Fri, Dec 08, 2006 at 06:43:49PM -0800, Allan Hsu wrote:
[...]
> > > + 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?
>
> The AMF array types and object types are both used in onMetaData tags;
> these types contain an arbitrary number of nested AMF objects.
> Since there is no object index information, it isn't possible to find the
> next object without parsing these objects and any nested objects. We
> don't care about any of the complex types, but it is necessary to parse
> and skip in order to get to any of the simpler types we do care about.
so there is no restriction on their order? the simple standard metadata stuff
can be at the end of random other unknown metadata? :(
anyway what about the following? it seems simpler ...
int parse_metadata_object(AVFormatContext *s, ByteIOContext *ioc, char *parent_key){
char key[256];
char str[256];
double num;
amf_get_string(ioc, key, sizeof(key)); //should set key to "" if failure
type= get_byte(ioc);
switch(type){
case AMF_DATA_TYPE_NUMBER:
num= av_int2dbl(get_be64(ioc)); break;
case AMF_DATA_TYPE_BOOL :
num= get_byte(ioc) ; break;
case AMF_DATA_TYPE_STRING:
if(amf_get_string(ioc, str, sizeof(str)) < 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(parse_metadata_object(s, ioc, key) < 0) //skip the following object
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++)
parse_metadata_object(s, ioc, key);
}
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;
}
if(!strcmp(key, "foobar") && type == barfoo && num>X && num<Y) s->something= num;
...
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;
> > }
>
> More like:
>
> if(length >= buffsize - 1)
> ...
if length=1 then a buffer with buffersize=2 should be enough (the 1 byte
string + 1 zero byte)
but 1 >= 2-1 is true and failure would happen
>
> 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
> This revision also includes Aurelien's suggestions from earlier today.
good
[...]
> +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?
[...]
> + if(context->height == 0 || context->width == 0)
> + return -1;
should be <=0
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
More information about the ffmpeg-devel
mailing list