On Tue, Apr 12, 2011 at 12:31:28PM +0300, Martin Storsjö wrote: > On Tue, 12 Apr 2011, Anton Khirnov wrote: > > > On Tue, Apr 12, 2011 at 11:58:44AM +0300, Martin Storsjö wrote: > > > On Tue, 12 Apr 2011, Anton Khirnov wrote: > > > > > > > On Tue, Apr 12, 2011 at 11:41:13AM +0300, Martin Storsjö wrote: > > > > > From: Kharkov Alexander <[email protected]> > > > > > > > > > > Current keyframes data parser unconditionally rewind metadata to > > > > > the end at the end of function. As result ALL metadata located > > > > > after keyframes index not parsed, and as metadata object can have > > > > > ANY placement inside metadata it can lead to unpredictable result > > > > > (bitrate can not be found, etc.). As result FLV movie will not > > > > > play at all in such situation. > > > > > --- > > > > > libavformat/flvdec.c | 3 ++- > > > > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > > > > > > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c > > > > > index f27b70c..5438ab0 100644 > > > > > --- a/libavformat/flvdec.c > > > > > +++ b/libavformat/flvdec.c > > > > > @@ -136,6 +136,7 @@ static int parse_keyframes_index(AVFormatContext > > > > > *s, AVIOContext *ioc, AVStream > > > > > int64_t *times = NULL; > > > > > int64_t *filepositions = NULL; > > > > > int ret = 0; > > > > > + int64_t initial_pos = url_ftell(ioc); > > > > > > > > avio_tell() > > > > > > Fixed locally > > > > > > > > > > > > > while (avio_tell(ioc) < max_pos - 2 && amf_get_string(ioc, > > > > > str_val, sizeof(str_val)) > 0) { > > > > > int64_t* current_array; > > > > > @@ -183,7 +184,7 @@ static int parse_keyframes_index(AVFormatContext > > > > > *s, AVIOContext *ioc, AVStream > > > > > finish: > > > > > av_freep(×); > > > > > av_freep(&filepositions); > > > > > - avio_seek(ioc, max_pos, SEEK_SET); > > > > > + avio_seek(ioc, initial_pos, SEEK_SET); > > > > > return ret; > > > > > } > > > > > > > > > > > > > This looks shady. Why should it rewind to the original position? > > > > > > Well, ideally it should probably seek to the end of the keyframes index > > > (but at least not to the end of all the metadata), but since the end of > > > the keyframes index isn't trivial to find (since this also is called when > > > exiting this function, if any of the allocations failed halfway through), > > > this at least seeks back to a good position, where the generic flv amr > > > parser can continue to iterate through it all. Or is it trivial to > > > calculate the end position of the keyframes index array? My amf knowledge > > > is a bit rusty... > > > > > > > Hmm, seems you're right. And here I was thinking that's a nice and not > > at all braindead format. > > > > > Another variant would be not to do any seek in either direction if > > > everything went well, at least, but it still requires seeking somewhere > > > if > > > something failed. > > > > > > > That sounds like a better idea. > > Actually, after re-reading the code, when returning from > parse_keyframes_index, amf_parse_object will continue to parse the same > object structure, so we can either do any of the following three: > > - Make amf_parse_object not iterate through the object if > parse_keyframes_index indicated that everything went well. This requires > that we make sure parse_keyframes_index exactly behaves as > amr_parse_object would do. Currently it doesn't, if there's some > additional unexpected data within the structure (the "else break;" part > within parse_keyframes_index currently). This might not be the only > issue, it's only the first thing that came to mind when reading the > code. > - Make parse_keyframes_index seek to the end, as it currently does, making > amf_parse_object not parse anything more at all, after keyframes have > been parsed. Robust, but loses all metadata after the keyframes. > - Make parse_keyframes_indx seek back to where it was before it was > called, as this patch does. Simple and robust. Has a slight > extra overhead of iterating over the keyframe index twice though.
The main problem I see with it is that it doesn't work if the input isn't seekable. But i guess it's ok for now and can be solved later if someone feels like it. -- Anton Khirnov
signature.asc
Description: Digital signature
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
