[libav-devel] [PATCH] Updated error codes and error log messages in mpeg4videodec.c

Luca Barbato lu_zero at gentoo.org
Thu Aug 6 09:27:39 CEST 2015


On 06/08/15 08:23, Jake Sebastian-Jones wrote:
> ---
>  libavcodec/mpeg4videodec.c | 209 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 138 insertions(+), 71 deletions(-)
> 
> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
> index b09aeb2..e7b9bf6 100644
> --- a/libavcodec/mpeg4videodec.c
> +++ b/libavcodec/mpeg4videodec.c
> @@ -377,20 +377,26 @@ int
> ff_mpeg4_decode_video_packet_header(Mpeg4DecContext *ctx)
>  {
>      MpegEncContext *s = &ctx->m;
> 
> -    int mb_num_bits      = av_log2(s->mb_num - 1) + 1;
> -    int header_extension = 0, mb_num, len;
> +    int mb_num_bits = av_log2(s->mb_num - 1) + 1;
> +    int header_extension  = 0, mb_num, len;

This is a stray change, drop from your patch (e.g. with `git reset -p`).


> -    /* is there enough space left for a video packet + header */
> -    if (get_bits_count(&s->gb) > s->gb.size_in_bits - 20)
> -        return -1;
> +    if (get_bits_count(&s->gb) > s->gb.size_in_bits - 20) {
> +        av_log(s->avctx, AV_LOG_ERROR,
> +               "buffer of size %d does not enough space for a video packet "
                                             ^
                                         have
> +               "of size %d + a header of size 20\n",
> +               s->gb.size_in_bits, get_bits_count(&s->gb));
> +        return AVERROR_INVALIDDATA;
> +    }
> 

>                  if (cbpc == 20)
>                      goto try_again;
> @@ -719,12 +730,20 @@ try_again:
>                          ff_h263_pred_motion(s, 0, 0, &pred_x, &pred_y);
>                          if (!s->mcsel) {
>                              mx = ff_h263_decode_motion(s, pred_x, s->f_code);

This function returns 0xffff when the get_vlc returns an invalid value
you can just skip this part and print the message once there (bonus
points for returning there the proper error and forward it everywhere.

> -                            if (mx >= 0xffff)
> -                                return -1;
> +                            if (mx >= 0xffff) {
> +                                av_log(s->avctx, AV_LOG_ERROR,
> +                                       "16x16 motion decode failed with mx of "
> +                                       "%#010x not < 0xffff\n", mx);
> +                                return AVERROR_INVALIDDATA;
> +                            }
> 
>                              my = ff_h263_decode_motion(s, pred_y, s->f_code);
> -                            if (my >= 0xffff)
> -                                return -1;
> +                            if (my >= 0xffff) {
> +                                av_log(s->avctx, AV_LOG_ERROR,
> +                                       "16x16 motion decode failed with my of "
> +                                       "%#010x not < 0xffff\n", my);
> +                                return AVERROR_INVALIDDATA;
> +                            }
>                              s->current_picture.mb_type[xy] = MB_TYPE_16x16 |
>                                                               MB_TYPE_L0;
>                          } else {
> @@ -750,12 +769,21 @@ try_again:
>                          for (i = 0; i < 4; i++) {
>                              int16_t *mot_val = ff_h263_pred_motion(s,
> i, 0, &pred_x, &pred_y);
>                              mx = ff_h263_decode_motion(s, pred_x, s->f_code);
> -                            if (mx >= 0xffff)
> -                                return -1;
> +                            if (mx >= 0xffff) {
> +                                av_log(s->avctx, AV_LOG_ERROR,
> +                                       "motion decode failed with mx of "
> +                                       "%#010x not < 0xffff\n", mx);
> +                                return AVERROR_INVALIDDATA;
> +                            }
> +
> 
>                              my = ff_h263_decode_motion(s, pred_y, s->f_code);
> -                            if (my >= 0xffff)
> -                                return -1;
> +                            if (my >= 0xffff) {
> +                                av_log(s->avctx, AV_LOG_ERROR,
> +                                       "motion decode failed with my of "
> +                                       "%#010x not < 0xffff\n", my);
> +                                return AVERROR_INVALIDDATA;
> +                            }
>                              mot_val[0] = mx;
>                              mot_val[1] = my;
>                          }

Same thing here below.

>                  for (i = 0; i < 2; i++) {
>                      mx = ff_h263_decode_motion(s, pred_x, s->f_code);
> -                    if (mx >= 0xffff)
> -                        return -1;
> +                    if (mx >= 0xffff) {
> +                        av_log(s->avctx, AV_LOG_ERROR,
> +                               "16x8 motion decode failed with mx of "
> +                               "%#010x not < 0xffff\n", mx);
> +                        return AVERROR_INVALIDDATA;
> +                    }
> 
>                      my = ff_h263_decode_motion(s, pred_y / 2, s->f_code);
> -                    if (my >= 0xffff)
> -                        return -1;
> +                    if (my >= 0xffff) {
> +                        av_log(s->avctx, AV_LOG_ERROR,
> +                               "16x8 motion decode failed with my of "
> +                               "%#010x not < 0xffff\n", my);
> +                        return AVERROR_INVALIDDATA;
> +                    }
> 
>                      s->mv[0][i][0] = mx;
>                      s->mv[0][i][1] = my;
> @@ -1371,13 +1413,21 @@ static int mpeg4_decode_mb(MpegEncContext *s,
> int16_t block[6][64])
>                  ff_h263_pred_motion(s, 0, 0, &pred_x, &pred_y);
>                  mx = ff_h263_decode_motion(s, pred_x, s->f_code);
> 
> -                if (mx >= 0xffff)
> -                    return -1;
> +                if (mx >= 0xffff) {
> +                    av_log(s->avctx, AV_LOG_ERROR,
> +                           "16x16 motion decode failed with mx of "
> +                           "%#010x not < 0xffff\n", mx);
> +                    return AVERROR_INVALIDDATA;
> +                }
> 
>                  my = ff_h263_decode_motion(s, pred_y, s->f_code);
> 
> -                if (my >= 0xffff)
> -                    return -1;
> +                if (my >= 0xffff) {
> +                    av_log(s->avctx, AV_LOG_ERROR,
> +                           "16x8 motion decode failed with my of "
> +                           "%#010x not < 0xffff\n", my);
> +                    return AVERROR_INVALIDDATA;
> +                }
>                  s->mv[0][0][0] = mx;
>                  s->mv[0][0][1] = my;
>              }
> @@ -1387,12 +1437,20 @@ static int mpeg4_decode_mb(MpegEncContext *s,
> int16_t block[6][64])
>              for (i = 0; i < 4; i++) {
>                  mot_val = ff_h263_pred_motion(s, i, 0, &pred_x, &pred_y);
>                  mx      = ff_h263_decode_motion(s, pred_x, s->f_code);
> -                if (mx >= 0xffff)
> -                    return -1;
> +                if (mx >= 0xffff) {
> +                    av_log(s->avctx, AV_LOG_ERROR,
> +                           "8x8 motion decode failed with mx of "
> +                           "%#010x not < 0xffff\n", mx);
> +                    return AVERROR_INVALIDDATA;
> +                }
> 
>                  my = ff_h263_decode_motion(s, pred_y, s->f_code);
> -                if (my >= 0xffff)
> -                    return -1;
> +                if (my >= 0xffff) {
> +                    av_log(s->avctx, AV_LOG_ERROR,
> +                           "8x8 motion decode failed with my of "
> +                           "%#010x not < 0xffff\n", my);
> +                    return AVERROR_INVALIDDATA;
> +                }

You should forward the error from `mpeg4_decode_block`. (see below)

>          for (i = 0; i < 6; i++) {
>              if (mpeg4_decode_block(ctx, block[i], i, cbp & 32, 1, 0) < 0)
> -                return -1;
> +                return AVERROR_INVALIDDATA;
>              cbp += cbp;
>          }
>          goto end;
> @@ -1602,7 +1661,7 @@ intra:
>      /* decode each block */
>      for (i = 0; i < 6; i++) {
>          if (mpeg4_decode_block(ctx, block[i], i, cbp & 32, 0, 0) < 0)
> -            return -1;
> +            return AVERROR_INVALIDDATA;
>          cbp += cbp;
>      }




> -            } else
> -                return -1;  // end of stream

> +            } else {
> +                av_log(s->avctx, AV_LOG_ERROR,
> +                       "encountered eos decoding picture header\n");
> +                return AVERROR_EOF;  // end of stream
> +            }

This would make the decoding stop, I'm not sure the comment is correct.

> +            if (decode_vol_header(ctx, gb) < 0) {
> +                av_log(s->avctx, AV_LOG_ERROR,
> +                       "failed to decode volume header\n");
> +                return AVERROR_INVALIDDATA;
> +            }

You should forward the error from `decode_vol_header`.

Larger patches end up requiring some more work, you might split it in
two so once a portion of it is ok can be merged already.

lu


More information about the libav-devel mailing list