flvdec: Fix support for flvtool2 "keyframes based" generated index

Message ID 1302597673-92122-1-git-send-email-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö April 12, 2011, 8:41 a.m.
From: Kharkov Alexander <kharkovalexander@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(-)

Comments

Anton Khirnov April 12, 2011, 8:48 a.m. | #1
On Tue, Apr 12, 2011 at 11:41:13AM +0300, Martin Storsjö wrote:
> From: Kharkov Alexander <kharkovalexander@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()

>  
>      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(&times);
>      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?
Martin Storsjö April 12, 2011, 8:58 a.m. | #2
On Tue, 12 Apr 2011, Anton Khirnov wrote:

> On Tue, Apr 12, 2011 at 11:41:13AM +0300, Martin Storsjö wrote:
> > From: Kharkov Alexander <kharkovalexander@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(&times);
> >      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...

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.

// Martin
Anton Khirnov April 12, 2011, 9:11 a.m. | #3
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 <kharkovalexander@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(&times);
> > >      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.
Martin Storsjö April 12, 2011, 9:31 a.m. | #4
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 <kharkovalexander@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(&times);
> > > >      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
Anton Khirnov April 12, 2011, 9:37 a.m. | #5
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 <kharkovalexander@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(&times);
> > > > >      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.
Martin Storsjö April 12, 2011, 9:41 a.m. | #6
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 <kharkovalexander@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(&times);
> > > > > >      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
Vladimir Pantelic April 12, 2011, 9:47 a.m. | #7
Martin Storsjö wrote:

> 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?)

Yes, and what I asked him not to do :)

Patch

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);
 
     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(&times);
     av_freep(&filepositions);
-    avio_seek(ioc, max_pos, SEEK_SET);
+    avio_seek(ioc, initial_pos, SEEK_SET);
     return ret;
 }