On Tue, 12 Apr 2011, Anton Khirnov wrote:
> 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.
Pushed.
An approach which doesn't require seekability would be to hook in this
parsing along the normal recursive amf parsing. That would distribute all
of the keyframe index parsing all over the place, though. (I didn't follow
this patchset closely in the beginning, but wasn't that what the author
did initially?)
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
