[Ffmpeg-devel] [PATCH] FLV decoder metadata reading
Allan Hsu
allan
Sun Dec 10 04:25:11 CET 2006
Hello
On Dec 9, 2006, at 10:33 AM, Michael Niedermayer wrote:
[...]
>>> 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? :(
That's correct. The metadata is contained in a "mixed array" AMF
object, which is an unordered collection of key/object pairs. I think
it's done this way because the data inside is intended to be handled
by actionscript inside of Flash applications, so the metadata tag is
just a bunch of serialized variable names and values.
> 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;
> }
I've implemented something like what you suggest, but without the
amf_get_string() call at the front (it interferes with using the same
function to skip nested objects).
[...]
>>> 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
Right. Fixed for real this time.
>
>>
>> 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.
[...]
>> +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.
>> + if(context->height == 0 || context->width == 0)
>> + return -1;
>
> should be <=0
[...]
Fixed. New patch attached.
-Allan
--
Allan Hsu <allan at counterpop dot net>
1E64 E20F 34D9 CBA7 1300 1457 AC37 CBBB 0E92 C779
-------------- next part --------------
A non-text attachment was scrubbed...
Name: flvdec_metadata.patch
Type: application/octet-stream
Size: 16953 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20061209/80c079ee/attachment.obj>
-------------- next part --------------
More information about the ffmpeg-devel
mailing list