[FFmpeg-devel,1/4] avformat/flvenc: Forward errors from allocating keyframe index

Submitted by Andreas Rheinhardt on Oct. 25, 2019, 8:52 a.m.

Details

Message ID 20191025085222.4498-1-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt Oct. 25, 2019, 8:52 a.m.
While the function adding a new element to the keyframe index checked
the allocation, the caller didn't check the return value. This has been
changed. To do so, the return value has been changed to an ordinary ret
instead of pb->error. This doesn't pose a problem, as write_packet() in
mux.c already checks for write errors (since 9ad1e0c1).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/flvenc.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Comments

Michael Niedermayer Oct. 25, 2019, 9:12 p.m.
On Fri, Oct 25, 2019 at 10:52:19AM +0200, Andreas Rheinhardt wrote:
> While the function adding a new element to the keyframe index checked
> the allocation, the caller didn't check the return value. This has been
> changed. To do so, the return value has been changed to an ordinary ret
> instead of pb->error. This doesn't pose a problem, as write_packet() in
> mux.c already checks for write errors (since 9ad1e0c1).
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/flvenc.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
> index fb1dede7ae..e327545dcf 100644
> --- a/libavformat/flvenc.c
> +++ b/libavformat/flvenc.c
> @@ -887,7 +887,7 @@ static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
>      unsigned ts;
>      int size = pkt->size;
>      uint8_t *data = NULL;
> -    int flags = -1, flags_size, ret;
> +    int flags = -1, flags_size, ret = 0;
>      int64_t cur_offset = avio_tell(pb);
>  
>      if (par->codec_type == AVMEDIA_TYPE_AUDIO && !pkt->size) {
> @@ -1055,15 +1055,17 @@ static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
>              case AVMEDIA_TYPE_VIDEO:
>                  flv->videosize += (avio_tell(pb) - cur_offset);
>                  flv->lasttimestamp = flv->acurframeindex / flv->framerate;
> +                flv->acurframeindex++;
>                  if (pkt->flags & AV_PKT_FLAG_KEY) {
> -                    double ts = flv->acurframeindex / flv->framerate;
> +                    double ts = flv->lasttimestamp;
>                      int64_t pos = cur_offset;
>  
> -                    flv->lastkeyframetimestamp = flv->acurframeindex / flv->framerate;
> +                    flv->lastkeyframetimestamp = ts;
>                      flv->lastkeyframelocation = pos;
> -                    flv_append_keyframe_info(s, flv, ts, pos);
> +                    ret = flv_append_keyframe_info(s, flv, ts, pos);
> +                    if (ret < 0)
> +                        goto fail;
>                  }
> -                flv->acurframeindex++;
>                  break;

Some of these changes look like unrelated simplifications
that makes it a hair harder to see the actual change

[...]
Andreas Rheinhardt Oct. 26, 2019, 2:55 a.m.
On Fri, Oct 25, 2019 at 11:12 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Fri, Oct 25, 2019 at 10:52:19AM +0200, Andreas Rheinhardt wrote:
> > While the function adding a new element to the keyframe index checked
> > the allocation, the caller didn't check the return value. This has been
> > changed. To do so, the return value has been changed to an ordinary ret
> > instead of pb->error. This doesn't pose a problem, as write_packet() in
> > mux.c already checks for write errors (since 9ad1e0c1).
> >
> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> > ---
> >  libavformat/flvenc.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
> > index fb1dede7ae..e327545dcf 100644
> > --- a/libavformat/flvenc.c
> > +++ b/libavformat/flvenc.c
> > @@ -887,7 +887,7 @@ static int flv_write_packet(AVFormatContext *s,
> AVPacket *pkt)
> >      unsigned ts;
> >      int size = pkt->size;
> >      uint8_t *data = NULL;
> > -    int flags = -1, flags_size, ret;
> > +    int flags = -1, flags_size, ret = 0;
> >      int64_t cur_offset = avio_tell(pb);
> >
> >      if (par->codec_type == AVMEDIA_TYPE_AUDIO && !pkt->size) {
> > @@ -1055,15 +1055,17 @@ static int flv_write_packet(AVFormatContext *s,
> AVPacket *pkt)
> >              case AVMEDIA_TYPE_VIDEO:
> >                  flv->videosize += (avio_tell(pb) - cur_offset);
> >                  flv->lasttimestamp = flv->acurframeindex /
> flv->framerate;
> > +                flv->acurframeindex++;
> >                  if (pkt->flags & AV_PKT_FLAG_KEY) {
> > -                    double ts = flv->acurframeindex / flv->framerate;
> > +                    double ts = flv->lasttimestamp;
> >                      int64_t pos = cur_offset;
> >
> > -                    flv->lastkeyframetimestamp = flv->acurframeindex /
> flv->framerate;
> > +                    flv->lastkeyframetimestamp = ts;
> >                      flv->lastkeyframelocation = pos;
> > -                    flv_append_keyframe_info(s, flv, ts, pos);
> > +                    ret = flv_append_keyframe_info(s, flv, ts, pos);
> > +                    if (ret < 0)
> > +                        goto fail;
> >                  }
> > -                flv->acurframeindex++;
> >                  break;
>
> Some of these changes look like unrelated simplifications
> that makes it a hair harder to see the actual change
>

The earlier code incremented flv->acurframeindex even when the index
allocation failed.
I wanted to keep it that way and the easiest way to do so is to move the
code incrementing
it before flv_append_keyframe_info(). But this would change the values of
ts and
flv->lastkeyframetimestamp, so one has to change how they are derived, too.

- Andreas
Michael Niedermayer Oct. 26, 2019, 5:58 p.m.
On Sat, Oct 26, 2019 at 04:55:35AM +0200, Andreas Rheinhardt wrote:
> On Fri, Oct 25, 2019 at 11:12 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Fri, Oct 25, 2019 at 10:52:19AM +0200, Andreas Rheinhardt wrote:
> > > While the function adding a new element to the keyframe index checked
> > > the allocation, the caller didn't check the return value. This has been
> > > changed. To do so, the return value has been changed to an ordinary ret
> > > instead of pb->error. This doesn't pose a problem, as write_packet() in
> > > mux.c already checks for write errors (since 9ad1e0c1).
> > >
> > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> > > ---
> > >  libavformat/flvenc.c | 16 +++++++++-------
> > >  1 file changed, 9 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
> > > index fb1dede7ae..e327545dcf 100644
> > > --- a/libavformat/flvenc.c
> > > +++ b/libavformat/flvenc.c
> > > @@ -887,7 +887,7 @@ static int flv_write_packet(AVFormatContext *s,
> > AVPacket *pkt)
> > >      unsigned ts;
> > >      int size = pkt->size;
> > >      uint8_t *data = NULL;
> > > -    int flags = -1, flags_size, ret;
> > > +    int flags = -1, flags_size, ret = 0;
> > >      int64_t cur_offset = avio_tell(pb);
> > >
> > >      if (par->codec_type == AVMEDIA_TYPE_AUDIO && !pkt->size) {
> > > @@ -1055,15 +1055,17 @@ static int flv_write_packet(AVFormatContext *s,
> > AVPacket *pkt)
> > >              case AVMEDIA_TYPE_VIDEO:
> > >                  flv->videosize += (avio_tell(pb) - cur_offset);
> > >                  flv->lasttimestamp = flv->acurframeindex /
> > flv->framerate;
> > > +                flv->acurframeindex++;
> > >                  if (pkt->flags & AV_PKT_FLAG_KEY) {
> > > -                    double ts = flv->acurframeindex / flv->framerate;
> > > +                    double ts = flv->lasttimestamp;
> > >                      int64_t pos = cur_offset;
> > >
> > > -                    flv->lastkeyframetimestamp = flv->acurframeindex /
> > flv->framerate;
> > > +                    flv->lastkeyframetimestamp = ts;
> > >                      flv->lastkeyframelocation = pos;
> > > -                    flv_append_keyframe_info(s, flv, ts, pos);
> > > +                    ret = flv_append_keyframe_info(s, flv, ts, pos);
> > > +                    if (ret < 0)
> > > +                        goto fail;
> > >                  }
> > > -                flv->acurframeindex++;
> > >                  break;
> >
> > Some of these changes look like unrelated simplifications
> > that makes it a hair harder to see the actual change
> >
> 
> The earlier code incremented flv->acurframeindex even when the index
> allocation failed.
> I wanted to keep it that way and the easiest way to do so is to move the
> code incrementing
> it before flv_append_keyframe_info(). But this would change the values of
> ts and
> flv->lastkeyframetimestamp, so one has to change how they are derived, too.

i think
you could do these changes before the flv_append_keyframe_info ret change

that would make both clearer i think

thx
[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
index fb1dede7ae..e327545dcf 100644
--- a/libavformat/flvenc.c
+++ b/libavformat/flvenc.c
@@ -887,7 +887,7 @@  static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
     unsigned ts;
     int size = pkt->size;
     uint8_t *data = NULL;
-    int flags = -1, flags_size, ret;
+    int flags = -1, flags_size, ret = 0;
     int64_t cur_offset = avio_tell(pb);
 
     if (par->codec_type == AVMEDIA_TYPE_AUDIO && !pkt->size) {
@@ -1055,15 +1055,17 @@  static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
             case AVMEDIA_TYPE_VIDEO:
                 flv->videosize += (avio_tell(pb) - cur_offset);
                 flv->lasttimestamp = flv->acurframeindex / flv->framerate;
+                flv->acurframeindex++;
                 if (pkt->flags & AV_PKT_FLAG_KEY) {
-                    double ts = flv->acurframeindex / flv->framerate;
+                    double ts = flv->lasttimestamp;
                     int64_t pos = cur_offset;
 
-                    flv->lastkeyframetimestamp = flv->acurframeindex / flv->framerate;
+                    flv->lastkeyframetimestamp = ts;
                     flv->lastkeyframelocation = pos;
-                    flv_append_keyframe_info(s, flv, ts, pos);
+                    ret = flv_append_keyframe_info(s, flv, ts, pos);
+                    if (ret < 0)
+                        goto fail;
                 }
-                flv->acurframeindex++;
                 break;
 
             case AVMEDIA_TYPE_AUDIO:
@@ -1075,10 +1077,10 @@  static int flv_write_packet(AVFormatContext *s, AVPacket *pkt)
                 break;
         }
     }
-
+fail:
     av_free(data);
 
-    return pb->error;
+    return ret;
 }
 
 static int flv_check_bitstream(struct AVFormatContext *s, const AVPacket *pkt)