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 <kharkovalexan...@gmail.com>
> > > >
> > > > 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.
// Martin
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
