[FFmpeg-devel] fate/api-h264-slice-test: use cleaner error handling

Submitted by joshdk@ob-encoder.com on Oct. 29, 2018, 1:57 p.m.

Details

Message ID 20181029135757.15077-1-joshdk@obe.tv
State New
Headers show

Commit Message

joshdk@ob-encoder.com Oct. 29, 2018, 1:57 p.m.
From: Josh de Kock <joshdk@obe.tv>

---
 tests/api/api-h264-slice-test.c | 74 +++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 31 deletions(-)

Comments

James Almer Oct. 29, 2018, 2:16 p.m.
On 10/29/2018 10:57 AM, joshdk@ob-encoder.com wrote:
> From: Josh de Kock <joshdk@obe.tv>
> 
> ---
>  tests/api/api-h264-slice-test.c | 74 +++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 31 deletions(-)
> 
> diff --git a/tests/api/api-h264-slice-test.c b/tests/api/api-h264-slice-test.c
> index 57e7dc79c3..08d5d57941 100644
> --- a/tests/api/api-h264-slice-test.c
> +++ b/tests/api/api-h264-slice-test.c
> @@ -48,7 +48,7 @@
>  
>  static int header = 0;
>  
> -static void decode(AVCodecContext *dec_ctx, AVFrame *frame,
> +static int decode(AVCodecContext *dec_ctx, AVFrame *frame,
>             AVPacket *pkt)
>  {
>      static uint64_t frame_cnt = 0;
> @@ -57,20 +57,20 @@ static void decode(AVCodecContext *dec_ctx, AVFrame *frame,
>      ret = avcodec_send_packet(dec_ctx, pkt);
>      if (ret < 0) {
>          fprintf(stderr, "Error sending a packet for decoding: %s\n", av_err2str(ret));
> -        exit(1);
> +        return ret;
>      }
>  
>      while (ret >= 0) {
>          const AVPixFmtDescriptor *desc;
> -        char *sum;
> +        char sum[AV_HASH_MAX_SIZE * 2 + 1];
>          struct AVHashContext *hash;
>  
>          ret = avcodec_receive_frame(dec_ctx, frame);
>          if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF) {
> -            return;
> +            return 0;
>          } else if (ret < 0) {
>              fprintf(stderr, "Error during decoding: %s\n", av_err2str(ret));
> -            exit(1);
> +            return ret;
>          }
>  
>          if (!header) {
> @@ -87,9 +87,10 @@ static void decode(AVCodecContext *dec_ctx, AVFrame *frame,
>              header = 1;
>          }
>          desc = av_pix_fmt_desc_get(dec_ctx->pix_fmt);
> -        av_hash_alloc(&hash, "md5");
> +        if ((ret = av_hash_alloc(&hash, "md5")) < 0) {
> +            return ret;
> +        }
>          av_hash_init(hash);
> -        sum = av_mallocz(av_hash_get_size(hash) * 2 + 1);
>  
>          for (int i = 0; i < frame->height; i++)
>              av_hash_update(hash, &frame->data[0][i * frame->linesize[0]], frame->width);
> @@ -104,8 +105,8 @@ static void decode(AVCodecContext *dec_ctx, AVFrame *frame,
>              (frame->width * frame->height + 2 * (frame->height >> desc->log2_chroma_h) * (frame->width >> desc->log2_chroma_w)), sum);
>          frame_cnt += 1;
>          av_hash_freep(&hash);
> -        av_free(sum);
>      }
> +    return 0;
>  }
>  
>  int main(int argc, char **argv)
> @@ -117,12 +118,12 @@ int main(int argc, char **argv)
>      AVPacket *pkt;
>      FILE *fd;
>      char nal[MAX_SLICES * UINT16_MAX + AV_INPUT_BUFFER_PADDING_SIZE];
> -    int nals = 0;
> +    int nals = 0, ret = 0;
>      char *p = nal;
>  
>      if (argc < 4) {
>          fprintf(stderr, "Usage: %s <threads> <input file> <output file>\n", argv[0]);
> -        exit(1);
> +        return -1;
>      }
>  
>      if (!(threads = strtoul(argv[1], NULL, 0)))
> @@ -134,17 +135,19 @@ int main(int argc, char **argv)
>      setmode(fileno(stdout), O_BINARY);
>  #endif
>  
> -    if (!(pkt = av_packet_alloc()))
> -        exit(1);
> +    if (!(pkt = av_packet_alloc())) {
> +        return -1;
> +    }
>  
>      if (!(codec = avcodec_find_decoder(AV_CODEC_ID_H264))) {
>          fprintf(stderr, "Codec not found\n");
> -        exit(1);
> +        return -1;

You're leaking the AVPacket if you return here.

>      }
>  
>      if (!(c = avcodec_alloc_context3(codec))) {
>          fprintf(stderr, "Could not allocate video codec context\n");
> -        exit(1);
> +        ret = -1;
> +        goto err_avctx;
>      }
>  
>      c->width  = 352;
> @@ -154,15 +157,16 @@ int main(int argc, char **argv)
>      c->thread_type = FF_THREAD_SLICE;
>      c->thread_count = threads;
>  
> -    if (avcodec_open2(c, codec, NULL) < 0) {
> +    if ((ret = avcodec_open2(c, codec, NULL)) < 0) {
>          fprintf(stderr, "Could not open codec\n");
> -        exit(1);
> +        goto err_frame;
>      }
>  
>  #if HAVE_THREADS
>      if (c->active_thread_type != FF_THREAD_SLICE) {
>          fprintf(stderr, "Couldn't activate slice threading: %d\n", c->active_thread_type);
> -        exit(1);
> +        ret = -1;
> +        goto err_frame;
>      }
>  #else
>      fprintf(stderr, "WARN: not using threads, only checking decoding slice NALUs\n");
> @@ -170,34 +174,36 @@ int main(int argc, char **argv)
>  
>      if (!(frame = av_frame_alloc())) {
>          fprintf(stderr, "Could not allocate video frame\n");
> -        exit(1);
> +        ret = -1;
> +        goto err_frame;
>      }
>  
>      if (!(fd = fopen(argv[2], "rb"))) {
>          fprintf(stderr, "Couldn't open NALU file: %s\n", argv[2]);
> -        exit(1);
> +        ret = -1;
> +        goto err_fopen;
>      }
>  
>      while(1) {
>          uint16_t size = 0;
> -        ssize_t ret = fread(&size, 1, sizeof(uint16_t), fd);
> -        if (ret < 0) {
> -            perror("Couldn't read size");
> -            exit(1);
> -        } else if (ret != sizeof(uint16_t))
> +        size_t ret = fread(&size, 1, sizeof(uint16_t), fd);
> +        if (ret != sizeof(uint16_t))
>              break;
>          size = ntohs(size);
>          ret = fread(p, 1, size, fd);
> -        if (ret < 0 || ret != size) {
> +        if (ret != size) {
>              perror("Couldn't read data");
> -            exit(1);
> +            goto err;
>          }
>          p += ret;
>  
>          if (++nals >= threads) {
> +            int decret = 0;
>              pkt->data = nal;
>              pkt->size = p - nal;
> -            decode(c, frame, pkt);
> +            if ((decret = decode(c, frame, pkt)) < 0) {
> +                goto err;
> +            }
>              memset(nal, 0, MAX_SLICES * UINT16_MAX + AV_INPUT_BUFFER_PADDING_SIZE);
>              nals = 0;
>              p = nal;
> @@ -207,15 +213,21 @@ int main(int argc, char **argv)
>      if (nals) {
>          pkt->data = nal;
>          pkt->size = p - nal;
> -        decode(c, frame, pkt);
> +        if ((ret = decode(c, frame, pkt)) < 0) {
> +            goto err;
> +        }
>      }
>  
> -    decode(c, frame, NULL);
> +    ret = decode(c, frame, NULL);
>  
> +err:
>      fclose(fd);
> -    avcodec_free_context(&c);
> +err_fopen:
>      av_frame_free(&frame);
> +err_frame:
> +    avcodec_free_context(&c);
> +err_avctx:
>      av_packet_free(&pkt);

There's no need for multiple labels. All these functions can be called
with a NULL argument. So simply initialize the respective pointers to
NULL, and failure to allocate or not, calling these will be safe.

The only exception is fclose, where a simple check to make sure fd is
not NULL is needed.

>  
> -    return 0;
> +    return ret;
>  }
>

Patch hide | download patch | download mbox

diff --git a/tests/api/api-h264-slice-test.c b/tests/api/api-h264-slice-test.c
index 57e7dc79c3..08d5d57941 100644
--- a/tests/api/api-h264-slice-test.c
+++ b/tests/api/api-h264-slice-test.c
@@ -48,7 +48,7 @@ 
 
 static int header = 0;
 
-static void decode(AVCodecContext *dec_ctx, AVFrame *frame,
+static int decode(AVCodecContext *dec_ctx, AVFrame *frame,
            AVPacket *pkt)
 {
     static uint64_t frame_cnt = 0;
@@ -57,20 +57,20 @@  static void decode(AVCodecContext *dec_ctx, AVFrame *frame,
     ret = avcodec_send_packet(dec_ctx, pkt);
     if (ret < 0) {
         fprintf(stderr, "Error sending a packet for decoding: %s\n", av_err2str(ret));
-        exit(1);
+        return ret;
     }
 
     while (ret >= 0) {
         const AVPixFmtDescriptor *desc;
-        char *sum;
+        char sum[AV_HASH_MAX_SIZE * 2 + 1];
         struct AVHashContext *hash;
 
         ret = avcodec_receive_frame(dec_ctx, frame);
         if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF) {
-            return;
+            return 0;
         } else if (ret < 0) {
             fprintf(stderr, "Error during decoding: %s\n", av_err2str(ret));
-            exit(1);
+            return ret;
         }
 
         if (!header) {
@@ -87,9 +87,10 @@  static void decode(AVCodecContext *dec_ctx, AVFrame *frame,
             header = 1;
         }
         desc = av_pix_fmt_desc_get(dec_ctx->pix_fmt);
-        av_hash_alloc(&hash, "md5");
+        if ((ret = av_hash_alloc(&hash, "md5")) < 0) {
+            return ret;
+        }
         av_hash_init(hash);
-        sum = av_mallocz(av_hash_get_size(hash) * 2 + 1);
 
         for (int i = 0; i < frame->height; i++)
             av_hash_update(hash, &frame->data[0][i * frame->linesize[0]], frame->width);
@@ -104,8 +105,8 @@  static void decode(AVCodecContext *dec_ctx, AVFrame *frame,
             (frame->width * frame->height + 2 * (frame->height >> desc->log2_chroma_h) * (frame->width >> desc->log2_chroma_w)), sum);
         frame_cnt += 1;
         av_hash_freep(&hash);
-        av_free(sum);
     }
+    return 0;
 }
 
 int main(int argc, char **argv)
@@ -117,12 +118,12 @@  int main(int argc, char **argv)
     AVPacket *pkt;
     FILE *fd;
     char nal[MAX_SLICES * UINT16_MAX + AV_INPUT_BUFFER_PADDING_SIZE];
-    int nals = 0;
+    int nals = 0, ret = 0;
     char *p = nal;
 
     if (argc < 4) {
         fprintf(stderr, "Usage: %s <threads> <input file> <output file>\n", argv[0]);
-        exit(1);
+        return -1;
     }
 
     if (!(threads = strtoul(argv[1], NULL, 0)))
@@ -134,17 +135,19 @@  int main(int argc, char **argv)
     setmode(fileno(stdout), O_BINARY);
 #endif
 
-    if (!(pkt = av_packet_alloc()))
-        exit(1);
+    if (!(pkt = av_packet_alloc())) {
+        return -1;
+    }
 
     if (!(codec = avcodec_find_decoder(AV_CODEC_ID_H264))) {
         fprintf(stderr, "Codec not found\n");
-        exit(1);
+        return -1;
     }
 
     if (!(c = avcodec_alloc_context3(codec))) {
         fprintf(stderr, "Could not allocate video codec context\n");
-        exit(1);
+        ret = -1;
+        goto err_avctx;
     }
 
     c->width  = 352;
@@ -154,15 +157,16 @@  int main(int argc, char **argv)
     c->thread_type = FF_THREAD_SLICE;
     c->thread_count = threads;
 
-    if (avcodec_open2(c, codec, NULL) < 0) {
+    if ((ret = avcodec_open2(c, codec, NULL)) < 0) {
         fprintf(stderr, "Could not open codec\n");
-        exit(1);
+        goto err_frame;
     }
 
 #if HAVE_THREADS
     if (c->active_thread_type != FF_THREAD_SLICE) {
         fprintf(stderr, "Couldn't activate slice threading: %d\n", c->active_thread_type);
-        exit(1);
+        ret = -1;
+        goto err_frame;
     }
 #else
     fprintf(stderr, "WARN: not using threads, only checking decoding slice NALUs\n");
@@ -170,34 +174,36 @@  int main(int argc, char **argv)
 
     if (!(frame = av_frame_alloc())) {
         fprintf(stderr, "Could not allocate video frame\n");
-        exit(1);
+        ret = -1;
+        goto err_frame;
     }
 
     if (!(fd = fopen(argv[2], "rb"))) {
         fprintf(stderr, "Couldn't open NALU file: %s\n", argv[2]);
-        exit(1);
+        ret = -1;
+        goto err_fopen;
     }
 
     while(1) {
         uint16_t size = 0;
-        ssize_t ret = fread(&size, 1, sizeof(uint16_t), fd);
-        if (ret < 0) {
-            perror("Couldn't read size");
-            exit(1);
-        } else if (ret != sizeof(uint16_t))
+        size_t ret = fread(&size, 1, sizeof(uint16_t), fd);
+        if (ret != sizeof(uint16_t))
             break;
         size = ntohs(size);
         ret = fread(p, 1, size, fd);
-        if (ret < 0 || ret != size) {
+        if (ret != size) {
             perror("Couldn't read data");
-            exit(1);
+            goto err;
         }
         p += ret;
 
         if (++nals >= threads) {
+            int decret = 0;
             pkt->data = nal;
             pkt->size = p - nal;
-            decode(c, frame, pkt);
+            if ((decret = decode(c, frame, pkt)) < 0) {
+                goto err;
+            }
             memset(nal, 0, MAX_SLICES * UINT16_MAX + AV_INPUT_BUFFER_PADDING_SIZE);
             nals = 0;
             p = nal;
@@ -207,15 +213,21 @@  int main(int argc, char **argv)
     if (nals) {
         pkt->data = nal;
         pkt->size = p - nal;
-        decode(c, frame, pkt);
+        if ((ret = decode(c, frame, pkt)) < 0) {
+            goto err;
+        }
     }
 
-    decode(c, frame, NULL);
+    ret = decode(c, frame, NULL);
 
+err:
     fclose(fd);
-    avcodec_free_context(&c);
+err_fopen:
     av_frame_free(&frame);
+err_frame:
+    avcodec_free_context(&c);
+err_avctx:
     av_packet_free(&pkt);
 
-    return 0;
+    return ret;
 }