diff mbox series

[FFmpeg-devel,4/7] avcodec/h264dec: prefer to use variable instead of type for sizeof

Message ID 1590674250-32571-4-git-send-email-lance.lmwang@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,1/7] avcodec/h264dec: cosmetics | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Lance Wang May 28, 2020, 1:57 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/h264dec.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

Comments

Jun Zhao May 28, 2020, 2:18 p.m. UTC | #1 
On Thu, May 28, 2020 at 9:58 PM <lance.lmwang@gmail.com> wrote:
>
> From: Limin Wang <lance.lmwang@gmail.com>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavcodec/h264dec.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> index 974d324..d5b3df3 100644
> --- a/libavcodec/h264dec.c
> +++ b/libavcodec/h264dec.c
> @@ -185,17 +185,17 @@ int ff_h264_alloc_tables(H264Context *h)
>      const int slice_tsize = big_mb_num + h->mb_stride;
>      int x, y;
>
> -    if (!(h->intra4x4_pred_mode     = av_mallocz_array(row_mb_num * 8, sizeof(uint8_t)))                ||
> -        !(h->non_zero_count         = av_mallocz_array(big_mb_num * 48, sizeof(uint8_t)))               ||
> +    if (!(h->intra4x4_pred_mode     = av_mallocz_array(row_mb_num * 8, sizeof(*h->intra4x4_pred_mode))) ||
> +        !(h->non_zero_count         = av_mallocz_array(big_mb_num * 48, sizeof(*h->non_zero_count)))    ||
>          !(h->slice_table_base       = av_mallocz_array(slice_tsize, sizeof(*h->slice_table_base)))      ||
> -        !(h->cbp_table              = av_mallocz_array(big_mb_num, sizeof(uint16_t)))                   ||
> -        !(h->chroma_pred_mode_table = av_mallocz_array(big_mb_num, sizeof(uint8_t)))                    ||
> -        !(h->mvd_table[0]           = av_mallocz_array(row_mb_num * 8, sizeof(uint8_t)))                ||
> -        !(h->mvd_table[1]           = av_mallocz_array(row_mb_num * 8, sizeof(uint8_t)))                ||
> -        !(h->direct_table           = av_mallocz_array(big_mb_num * 4, sizeof(uint8_t)))                ||
> -        !(h->list_counts            = av_mallocz_array(big_mb_num, sizeof(uint8_t)))                    ||
> -        !(h->mb2b_xy                = av_mallocz_array(big_mb_num, sizeof(uint32_t)))                   ||
> -        !(h->mb2br_xy               = av_mallocz_array(big_mb_num, sizeof(uint32_t))))
> +        !(h->cbp_table              = av_mallocz_array(big_mb_num, sizeof(*h->cbp_table)))              ||
> +        !(h->chroma_pred_mode_table = av_mallocz_array(big_mb_num, sizeof(*h->chroma_pred_mode_table))) ||
> +        !(h->mvd_table[0]           = av_mallocz_array(row_mb_num * 8, sizeof(*h->mvd_table[0])))       ||
> +        !(h->mvd_table[1]           = av_mallocz_array(row_mb_num * 8, sizeof(*h->mvd_table[1])))       ||
> +        !(h->direct_table           = av_mallocz_array(big_mb_num * 4, sizeof(*h->direct_table)))       ||
> +        !(h->list_counts            = av_mallocz_array(big_mb_num, sizeof(*h->list_counts)))            ||
> +        !(h->mb2b_xy                = av_mallocz_array(big_mb_num, sizeof(*h->mb2b_xy)))                ||
> +        !(h->mb2br_xy               = av_mallocz_array(big_mb_num, sizeof(*h->mb2br_xy))))
>          return AVERROR(ENOMEM);
>      h->slice_ctx[0].intra4x4_pred_mode = h->intra4x4_pred_mode;
>      h->slice_ctx[0].mvd_table[0] = h->mvd_table[0];
> @@ -252,10 +252,10 @@ int ff_h264_slice_context_init(H264Context *h, H264SliceContext *sl)
>          er->b8_stride   = h->mb_width * 2 + 1;
>
>          // error resilience code looks cleaner with this
> -        if (!(er->mb_index2xy        = av_mallocz_array(h->mb_num + 1, sizeof(int)))                   ||
> -            !(er->error_status_table = av_mallocz_array(mb_array_size, sizeof(uint8_t)))               ||
> -            !(er->er_temp_buffer     = av_mallocz_array(h->mb_height * h->mb_stride + 1, sizeof(int))) ||
> -            !(sl->dc_val_base        = av_mallocz_array(yc_size, sizeof(int16_t))))
> +        if (!(er->mb_index2xy        = av_mallocz_array(h->mb_num + 1, sizeof(*er->mb_index2xy)))                      ||
> +            !(er->error_status_table = av_mallocz_array(mb_array_size, sizeof(*er->error_status_table)))               ||
> +            !(er->er_temp_buffer     = av_mallocz_array(h->mb_height * h->mb_stride + 1, sizeof(*er->er_temp_buffer))) ||
> +            !(sl->dc_val_base        = av_mallocz_array(yc_size, sizeof(*sl->dc_val_base))))
>              return AVERROR(ENOMEM); // ff_h264_free_tables will clean up for us
>
>          for (y = 0; y < h->mb_height; y++)
> --
Do you have any special reason to use 2 patches (patch 3-4) and don't
combine patch 3-4 as one patch?   And I don't think splitting this
step as 2 patches will help code-review

The other thing is, I can't find this change for what special reasons
Lance Wang May 28, 2020, 3:06 p.m. UTC | #2 
On Thu, May 28, 2020 at 10:18:09PM +0800, mypopy@gmail.com wrote:
> On Thu, May 28, 2020 at 9:58 PM <lance.lmwang@gmail.com> wrote:
> >
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavcodec/h264dec.c | 28 ++++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> > index 974d324..d5b3df3 100644
> > --- a/libavcodec/h264dec.c
> > +++ b/libavcodec/h264dec.c
> > @@ -185,17 +185,17 @@ int ff_h264_alloc_tables(H264Context *h)
> >      const int slice_tsize = big_mb_num + h->mb_stride;
> >      int x, y;
> >
> > -    if (!(h->intra4x4_pred_mode     = av_mallocz_array(row_mb_num * 8, sizeof(uint8_t)))                ||
> > -        !(h->non_zero_count         = av_mallocz_array(big_mb_num * 48, sizeof(uint8_t)))               ||
> > +    if (!(h->intra4x4_pred_mode     = av_mallocz_array(row_mb_num * 8, sizeof(*h->intra4x4_pred_mode))) ||
> > +        !(h->non_zero_count         = av_mallocz_array(big_mb_num * 48, sizeof(*h->non_zero_count)))    ||
> >          !(h->slice_table_base       = av_mallocz_array(slice_tsize, sizeof(*h->slice_table_base)))      ||
> > -        !(h->cbp_table              = av_mallocz_array(big_mb_num, sizeof(uint16_t)))                   ||
> > -        !(h->chroma_pred_mode_table = av_mallocz_array(big_mb_num, sizeof(uint8_t)))                    ||
> > -        !(h->mvd_table[0]           = av_mallocz_array(row_mb_num * 8, sizeof(uint8_t)))                ||
> > -        !(h->mvd_table[1]           = av_mallocz_array(row_mb_num * 8, sizeof(uint8_t)))                ||
> > -        !(h->direct_table           = av_mallocz_array(big_mb_num * 4, sizeof(uint8_t)))                ||
> > -        !(h->list_counts            = av_mallocz_array(big_mb_num, sizeof(uint8_t)))                    ||
> > -        !(h->mb2b_xy                = av_mallocz_array(big_mb_num, sizeof(uint32_t)))                   ||
> > -        !(h->mb2br_xy               = av_mallocz_array(big_mb_num, sizeof(uint32_t))))
> > +        !(h->cbp_table              = av_mallocz_array(big_mb_num, sizeof(*h->cbp_table)))              ||
> > +        !(h->chroma_pred_mode_table = av_mallocz_array(big_mb_num, sizeof(*h->chroma_pred_mode_table))) ||
> > +        !(h->mvd_table[0]           = av_mallocz_array(row_mb_num * 8, sizeof(*h->mvd_table[0])))       ||
> > +        !(h->mvd_table[1]           = av_mallocz_array(row_mb_num * 8, sizeof(*h->mvd_table[1])))       ||
> > +        !(h->direct_table           = av_mallocz_array(big_mb_num * 4, sizeof(*h->direct_table)))       ||
> > +        !(h->list_counts            = av_mallocz_array(big_mb_num, sizeof(*h->list_counts)))            ||
> > +        !(h->mb2b_xy                = av_mallocz_array(big_mb_num, sizeof(*h->mb2b_xy)))                ||
> > +        !(h->mb2br_xy               = av_mallocz_array(big_mb_num, sizeof(*h->mb2br_xy))))
> >          return AVERROR(ENOMEM);
> >      h->slice_ctx[0].intra4x4_pred_mode = h->intra4x4_pred_mode;
> >      h->slice_ctx[0].mvd_table[0] = h->mvd_table[0];
> > @@ -252,10 +252,10 @@ int ff_h264_slice_context_init(H264Context *h, H264SliceContext *sl)
> >          er->b8_stride   = h->mb_width * 2 + 1;
> >
> >          // error resilience code looks cleaner with this
> > -        if (!(er->mb_index2xy        = av_mallocz_array(h->mb_num + 1, sizeof(int)))                   ||
> > -            !(er->error_status_table = av_mallocz_array(mb_array_size, sizeof(uint8_t)))               ||
> > -            !(er->er_temp_buffer     = av_mallocz_array(h->mb_height * h->mb_stride + 1, sizeof(int))) ||
> > -            !(sl->dc_val_base        = av_mallocz_array(yc_size, sizeof(int16_t))))
> > +        if (!(er->mb_index2xy        = av_mallocz_array(h->mb_num + 1, sizeof(*er->mb_index2xy)))                      ||
> > +            !(er->error_status_table = av_mallocz_array(mb_array_size, sizeof(*er->error_status_table)))               ||
> > +            !(er->er_temp_buffer     = av_mallocz_array(h->mb_height * h->mb_stride + 1, sizeof(*er->er_temp_buffer))) ||
> > +            !(sl->dc_val_base        = av_mallocz_array(yc_size, sizeof(*sl->dc_val_base))))
> >              return AVERROR(ENOMEM); // ff_h264_free_tables will clean up for us
> >
> >          for (y = 0; y < h->mb_height; y++)
> > --
> Do you have any special reason to use 2 patches (patch 3-4) and don't
> combine patch 3-4 as one patch?   And I don't think splitting this
> step as 2 patches will help code-review
At first, I change 3-4 in one patch, but it's difficult to say clearly in the commit message for two changes mixed,
so I split into 2 patch.

> 
> The other thing is, I can't find this change for what special reasons

refine the code for better readiablity, remove the unneeded gotos, remove the
FF_ALLOC{Z}{_ARRAY}_OR_GOTO macros.
diff mbox series

Patch

diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
index 974d324..d5b3df3 100644
--- a/libavcodec/h264dec.c
+++ b/libavcodec/h264dec.c
@@ -185,17 +185,17 @@  int ff_h264_alloc_tables(H264Context *h)
     const int slice_tsize = big_mb_num + h->mb_stride;
     int x, y;
 
-    if (!(h->intra4x4_pred_mode     = av_mallocz_array(row_mb_num * 8, sizeof(uint8_t)))                ||
-        !(h->non_zero_count         = av_mallocz_array(big_mb_num * 48, sizeof(uint8_t)))               ||
+    if (!(h->intra4x4_pred_mode     = av_mallocz_array(row_mb_num * 8, sizeof(*h->intra4x4_pred_mode))) ||
+        !(h->non_zero_count         = av_mallocz_array(big_mb_num * 48, sizeof(*h->non_zero_count)))    ||
         !(h->slice_table_base       = av_mallocz_array(slice_tsize, sizeof(*h->slice_table_base)))      ||
-        !(h->cbp_table              = av_mallocz_array(big_mb_num, sizeof(uint16_t)))                   ||
-        !(h->chroma_pred_mode_table = av_mallocz_array(big_mb_num, sizeof(uint8_t)))                    ||
-        !(h->mvd_table[0]           = av_mallocz_array(row_mb_num * 8, sizeof(uint8_t)))                ||
-        !(h->mvd_table[1]           = av_mallocz_array(row_mb_num * 8, sizeof(uint8_t)))                ||
-        !(h->direct_table           = av_mallocz_array(big_mb_num * 4, sizeof(uint8_t)))                ||
-        !(h->list_counts            = av_mallocz_array(big_mb_num, sizeof(uint8_t)))                    ||
-        !(h->mb2b_xy                = av_mallocz_array(big_mb_num, sizeof(uint32_t)))                   ||
-        !(h->mb2br_xy               = av_mallocz_array(big_mb_num, sizeof(uint32_t))))
+        !(h->cbp_table              = av_mallocz_array(big_mb_num, sizeof(*h->cbp_table)))              ||
+        !(h->chroma_pred_mode_table = av_mallocz_array(big_mb_num, sizeof(*h->chroma_pred_mode_table))) ||
+        !(h->mvd_table[0]           = av_mallocz_array(row_mb_num * 8, sizeof(*h->mvd_table[0])))       ||
+        !(h->mvd_table[1]           = av_mallocz_array(row_mb_num * 8, sizeof(*h->mvd_table[1])))       ||
+        !(h->direct_table           = av_mallocz_array(big_mb_num * 4, sizeof(*h->direct_table)))       ||
+        !(h->list_counts            = av_mallocz_array(big_mb_num, sizeof(*h->list_counts)))            ||
+        !(h->mb2b_xy                = av_mallocz_array(big_mb_num, sizeof(*h->mb2b_xy)))                ||
+        !(h->mb2br_xy               = av_mallocz_array(big_mb_num, sizeof(*h->mb2br_xy))))
         return AVERROR(ENOMEM);
     h->slice_ctx[0].intra4x4_pred_mode = h->intra4x4_pred_mode;
     h->slice_ctx[0].mvd_table[0] = h->mvd_table[0];
@@ -252,10 +252,10 @@  int ff_h264_slice_context_init(H264Context *h, H264SliceContext *sl)
         er->b8_stride   = h->mb_width * 2 + 1;
 
         // error resilience code looks cleaner with this
-        if (!(er->mb_index2xy        = av_mallocz_array(h->mb_num + 1, sizeof(int)))                   ||
-            !(er->error_status_table = av_mallocz_array(mb_array_size, sizeof(uint8_t)))               ||
-            !(er->er_temp_buffer     = av_mallocz_array(h->mb_height * h->mb_stride + 1, sizeof(int))) ||
-            !(sl->dc_val_base        = av_mallocz_array(yc_size, sizeof(int16_t))))
+        if (!(er->mb_index2xy        = av_mallocz_array(h->mb_num + 1, sizeof(*er->mb_index2xy)))                      ||
+            !(er->error_status_table = av_mallocz_array(mb_array_size, sizeof(*er->error_status_table)))               ||
+            !(er->er_temp_buffer     = av_mallocz_array(h->mb_height * h->mb_stride + 1, sizeof(*er->er_temp_buffer))) ||
+            !(sl->dc_val_base        = av_mallocz_array(yc_size, sizeof(*sl->dc_val_base))))
             return AVERROR(ENOMEM); // ff_h264_free_tables will clean up for us
 
         for (y = 0; y < h->mb_height; y++)