[FFmpeg-devel] [PATCH 2/6] idcinvideo: if decoding fails return error

[Stefano Sabatini]

On date Wednesday 2012-10-17 21:12:45 +0000, Paul B Mahol encoded:
> On 10/17/12, Stefano Sabatini <stefasab at gmail.com> wrote:
> > On date Wednesday 2012-10-17 15:55:15 +0000, Paul B Mahol encoded:
> >> While here return meaningful error codes.
> >>
> >> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> >> ---
> >>  libavcodec/idcinvideo.c     | 14 +++++++++-----
> >>  tests/ref/fate/id-cin-video |  1 -
> >>  2 files changed, 9 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/libavcodec/idcinvideo.c b/libavcodec/idcinvideo.c
> >> index eedc4fc..9185f88 100644
> >> --- a/libavcodec/idcinvideo.c
> >> +++ b/libavcodec/idcinvideo.c
> >> @@ -173,7 +173,7 @@ static av_cold int idcin_decode_init(AVCodecContext
> >> *avctx)
> >>      return 0;
> >>  }
> >>
> >> -static void idcin_decode_vlcs(IdcinContext *s)
> >> +static int idcin_decode_vlcs(IdcinContext *s)
> >>  {
> >>      hnode *hnodes;
> >>      long x, y;
> >> @@ -192,7 +192,7 @@ static void idcin_decode_vlcs(IdcinContext *s)
> >>                  if(!bit_pos) {
> >>                      if(dat_pos >= s->size) {
> >>                          av_log(s->avctx, AV_LOG_ERROR, "Huffman decode
> >> error.\n");
> >> -                        return;
> >> +                        return -1;
> >
> > Uhm return AVERROR_INVALIDDATA?
> 
> Not much point, see bellow.
> >
> >>                      }
> >>                      bit_pos = 8;
> >>                      v = s->buf[dat_pos++];
> >> @@ -207,6 +207,8 @@ static void idcin_decode_vlcs(IdcinContext *s)
> >>              prev = node_num;
> >>          }
> >>      }
> >> +
> >> +    return 0;
> >>  }
> >>
> >>  static int idcin_decode_frame(AVCodecContext *avctx,
> >> @@ -217,6 +219,7 @@ static int idcin_decode_frame(AVCodecContext *avctx,
> >>      int buf_size = avpkt->size;
> >>      IdcinContext *s = avctx->priv_data;
> >>      const uint8_t *pal = av_packet_get_side_data(avpkt,
> >> AV_PKT_DATA_PALETTE, NULL);
> >> +    int ret;
> >>
> >>      s->buf = buf;
> >>      s->size = buf_size;
> >> @@ -224,12 +227,13 @@ static int idcin_decode_frame(AVCodecContext
> >> *avctx,
> >>      if (s->frame.data[0])
> >>          avctx->release_buffer(avctx, &s->frame);
> >>
> >> -    if (avctx->get_buffer(avctx, &s->frame)) {
> >> +    if ((ret = avctx->get_buffer(avctx, &s->frame))) {
> >>          av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> >> -        return -1;
> >> +        return ret;
> >>      }
> >>
> >> -    idcin_decode_vlcs(s);
> >> +    if (idcin_decode_vlcs(s))
> >> +        return AVERROR_INVALIDDATA;
> >
> > While at it you could propagate the error code.
> 
> no much point, it always fails with same error code.
> >
> >>
> >>      if (pal) {
> >>          s->frame.palette_has_changed = 1;
> >> diff --git a/tests/ref/fate/id-cin-video b/tests/ref/fate/id-cin-video
> >> index 241cae5..f55544f 100644
> >> --- a/tests/ref/fate/id-cin-video
> >> +++ b/tests/ref/fate/id-cin-video
> >> @@ -104,4 +104,3 @@
> >>  1,      78750,      78750,     1575,     6300, 0xe3bfa403
> >>  0,         51,         51,        1,   230400, 0x488de02d
> >>  1,      80325,      80325,     1575,     6300, 0x2c5bd9c9
> >> -0,         52,         52,        1,   230400, 0x488de02d
> >
> > Is this expected? Could you explain it in the commit log?
> 

> Last video frame is truncated, previously you would get error msg if decoding
> failed but it would be ingnored - aka no error detection/resilence.

Mention it in the log message, looks good otherwise, thanks.
-- 
FFmpeg = Friendly Fanciful Mastering Peaceless Elitist God