[FFmpeg-devel] [PATCH 1/2] avcodec/mpegvideo: prefer to use variable instead of type for sizeof
Marton Balint
cus at passwd.hu
Mon May 11 10:16:39 EEST 2020
On Mon, 11 May 2020, lance.lmwang at gmail.com wrote:
> On Mon, May 11, 2020 at 01:22:24AM +0200, Marton Balint wrote:
>>
>>
>> On Mon, 11 May 2020, lance.lmwang at gmail.com wrote:
>>
>> > On Sun, May 10, 2020 at 06:30:41PM +0200, Marton Balint wrote:
>> > >
>> > >
>> > > On Sun, 10 May 2020, lance.lmwang at gmail.com wrote:
>> > >
>> > > > From: Limin Wang <lance.lmwang at gmail.com>
>> > > > > Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
>> > > > ---
>> > > > libavcodec/mpegvideo.c | 48 ++++++++++++++++++++++++------------------------
>> > > > 1 file changed, 24 insertions(+), 24 deletions(-)
>> > >
>> > > If you find these cosmetics interesting, then I suggest you introduce a new
>> > > macro instead: FF_ALLOCZ_ARRAY_OR_GOTO().
>> > >
>> > > E.g.:
>> > >
>> > > FF_ALLOCZ_ARRAY_OR_GOTO(s->avctx, s->me.map, ME_MAP_SIZE, fail)
>> >
>> > Yeah, I have considered it so I change the type to use use variable first and
>> > submit one typical for review. If the change is OK, then I'll go ahead next.
>>
>> No need to do it in two steps, better touch the code once. E.g. patch 2
>> might not even be needed if you do this in a single patch.
>
> Sorry, now for array alloc, you had to input the one elemeay size and its count
> always. internal.h have defined it already:
> FF_ALLOCZ_ARRAY_OR_GOTO(ctx, p, nelem, elsize, label)
>
> You don't want to input elsize? isn't general usage?
I just found out that there is already an FF_ALLOCZ_ARRAY_OR_GOTO with
elsize. So you should add a new macro which uses FF_ALLOCZ_ARRAY_OR_GOTO
and calculates elszize automatically. I am not sure about the name:
FF_ALLOCZ_TYPED_ARRAY_OR_GOTO ?
Regards,
Marton
>
>>
>> Regards,
>> Marton
>>
>> >
>> > >
>> > > Regards,
>> > > Marton
>> > >
>> > > > > diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
>> > > > index 49fd1c9..561062f 100644
>> > > > --- a/libavcodec/mpegvideo.c
>> > > > +++ b/libavcodec/mpegvideo.c
>> > > > @@ -373,15 +373,15 @@ static int init_duplicate_context(MpegEncContext *s)
>> > > > > if (s->encoding) {
>> > > > FF_ALLOCZ_OR_GOTO(s->avctx, s->me.map,
>> > > > - ME_MAP_SIZE * sizeof(uint32_t), fail)
>> > > > + ME_MAP_SIZE * sizeof(*s->me.map), fail)
>> > > > FF_ALLOCZ_OR_GOTO(s->avctx, s->me.score_map,
>> > > > - ME_MAP_SIZE * sizeof(uint32_t), fail)
>> > > > + ME_MAP_SIZE * sizeof(*s->me.score_map), fail)
>> > > > if (s->noise_reduction) {
>> > > > FF_ALLOCZ_OR_GOTO(s->avctx, s->dct_error_sum,
>> > > > - 2 * 64 * sizeof(int), fail)
>> > > > + 2 * 64 * sizeof(*s->dct_error_sum), fail)
>> > > > }
>> > > > }
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->blocks, 64 * 12 * 2 * sizeof(int16_t), fail)
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->blocks, 64 * 12 * 2 * sizeof(*s->blocks), fail)
>> > > > s->block = s->blocks[0];
>> > > > > for (i = 0; i < 12; i++) {
>> > > > @@ -400,7 +400,7 @@ static int init_duplicate_context(MpegEncContext *s)
>> > > > if (s->out_format == FMT_H263) {
>> > > > /* ac values */
>> > > > FF_ALLOCZ_OR_GOTO(s->avctx, s->ac_val_base,
>> > > > - yc_size * sizeof(int16_t) * 16, fail);
>> > > > + yc_size * sizeof(*s->ac_val_base) * 16, fail);
>> > > > s->ac_val[0] = s->ac_val_base + s->b8_stride + 1;
>> > > > s->ac_val[1] = s->ac_val_base + y_size + s->mb_stride + 1;
>> > > > s->ac_val[2] = s->ac_val[1] + c_size;
>> > > > @@ -715,7 +715,7 @@ static int init_context_frame(MpegEncContext *s)
>> > > > if (s->mb_height & 1)
>> > > > yc_size += 2*s->b8_stride + 2*s->mb_stride;
>> > > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->mb_index2xy, (s->mb_num + 1)
>> > > * sizeof(int),
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->mb_index2xy, (s->mb_num + 1) * sizeof(*s->mb_index2xy),
>> > > > fail); // error resilience code looks cleaner with this
>> > > > for (y = 0; y < s->mb_height; y++)
>> > > > for (x = 0; x < s->mb_width; x++)
>> > > > @@ -725,12 +725,12 @@ static int init_context_frame(MpegEncContext *s)
>> > > > > if (s->encoding) {
>> > > > /* Allocate MV tables */
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->p_mv_table_base, mv_table_size * 2 * sizeof(int16_t), fail)
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->b_forw_mv_table_base, mv_table_size * 2 * sizeof(int16_t), fail)
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->b_back_mv_table_base, mv_table_size * 2 * sizeof(int16_t), fail)
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->b_bidir_forw_mv_table_base, mv_table_size * 2 * sizeof(int16_t), fail)
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->b_bidir_back_mv_table_base, mv_table_size * 2 * sizeof(int16_t), fail)
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->b_direct_mv_table_base, mv_table_size * 2 * sizeof(int16_t), fail)
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->p_mv_table_base, mv_table_size * 2 * sizeof(*s->p_mv_table_base), fail)
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->b_forw_mv_table_base, mv_table_size * 2 * sizeof(*s->b_forw_mv_table_base), fail)
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->b_back_mv_table_base, mv_table_size * 2 * sizeof(*s->b_back_mv_table_base), fail)
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->b_bidir_forw_mv_table_base, mv_table_size * 2 * sizeof(*s->b_bidir_forw_mv_table_base), fail)
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->b_bidir_back_mv_table_base, mv_table_size * 2 * sizeof(*s->b_bidir_back_mv_table_base), fail)
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->b_direct_mv_table_base, mv_table_size * 2 * sizeof(*s->b_direct_mv_table_base), fail)
>> > > > s->p_mv_table = s->p_mv_table_base + s->mb_stride + 1;
>> > > > s->b_forw_mv_table = s->b_forw_mv_table_base + s->mb_stride + 1;
>> > > > s->b_back_mv_table = s->b_back_mv_table_base + s->mb_stride + 1;
>> > > > @@ -739,14 +739,14 @@ static int init_context_frame(MpegEncContext *s)
>> > > > s->b_direct_mv_table = s->b_direct_mv_table_base + s->mb_stride + 1;
>> > > > > /* Allocate MB type table */
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->mb_type, mb_array_size * sizeof(uint16_t), fail) // needed for encoding
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->mb_type, mb_array_size * sizeof(*s->mb_type), fail) // needed for encoding
>> > > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->lambda_table,
>> > > mb_array_size * sizeof(int), fail)
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->lambda_table, mb_array_size * sizeof(*s->lambda_table), fail)
>> > > > > FF_ALLOC_OR_GOTO(s->avctx, s->cplx_tab,
>> > > > - mb_array_size * sizeof(float), fail);
>> > > > + mb_array_size * sizeof(*s->cplx_tab), fail);
>> > > > FF_ALLOC_OR_GOTO(s->avctx, s->bits_tab,
>> > > > - mb_array_size * sizeof(float), fail);
>> > > > + mb_array_size * sizeof(*s->bits_tab), fail);
>> > > > > }
>> > > > > @@ -759,16 +759,16 @@ static int
>> > > init_context_frame(MpegEncContext *s)
>> > > > for (k = 0; k < 2; k++) {
>> > > > FF_ALLOCZ_OR_GOTO(s->avctx,
>> > > > s->b_field_mv_table_base[i][j][k],
>> > > > - mv_table_size * 2 * sizeof(int16_t),
>> > > > + mv_table_size * 2 * sizeof(*s->b_field_mv_table_base[i][j][k]),
>> > > > fail);
>> > > > s->b_field_mv_table[i][j][k] = s->b_field_mv_table_base[i][j][k] +
>> > > > s->mb_stride + 1;
>> > > > }
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->b_field_select_table [i][j], mb_array_size * 2 * sizeof(uint8_t), fail)
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->p_field_mv_table_base[i][j], mv_table_size * 2 * sizeof(int16_t), fail)
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->b_field_select_table [i][j], mb_array_size * 2 * sizeof(*s->b_field_select_table [i][j]), fail)
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->p_field_mv_table_base[i][j], mv_table_size * 2 * sizeof(*s->p_field_mv_table_base[i][j]), fail)
>> > > > s->p_field_mv_table[i][j] = s->p_field_mv_table_base[i][j] + s->mb_stride + 1;
>> > > > }
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->p_field_select_table[i], mb_array_size * 2 * sizeof(uint8_t), fail)
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->p_field_select_table[i], mb_array_size * 2 * sizeof(*s->p_field_select_table[i]), fail)
>> > > > }
>> > > > }
>> > > > if (s->out_format == FMT_H263) {
>> > > > @@ -777,14 +777,14 @@ static int init_context_frame(MpegEncContext *s)
>> > > > s->coded_block = s->coded_block_base + s->b8_stride + 1;
>> > > > > /* cbp, ac_pred, pred_dir */
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->cbp_table , mb_array_size * sizeof(uint8_t), fail);
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->pred_dir_table, mb_array_size * sizeof(uint8_t), fail);
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->cbp_table , mb_array_size * sizeof(*s->cbp_table), fail);
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->pred_dir_table, mb_array_size * sizeof(*s->pred_dir_table), fail);
>> > > > }
>> > > > > if (s->h263_pred || s->h263_plus || !s->encoding) {
>> > > > /* dc values */
>> > > > // MN: we need these for error resilience of intra-frames
>> > > > - FF_ALLOCZ_OR_GOTO(s->avctx, s->dc_val_base, yc_size * sizeof(int16_t), fail);
>> > > > + FF_ALLOCZ_OR_GOTO(s->avctx, s->dc_val_base, yc_size * sizeof(*s->dc_val_base), fail);
>> > > > s->dc_val[0] = s->dc_val_base + s->b8_stride + 1;
>> > > > s->dc_val[1] = s->dc_val_base + y_size + s->mb_stride + 1;
>> > > > s->dc_val[2] = s->dc_val[1] + c_size;
>> > > > @@ -935,7 +935,7 @@ av_cold int ff_mpv_common_init(MpegEncContext *s)
>> > > > return ret;
>> > > > > FF_ALLOCZ_OR_GOTO(s->avctx, s->picture,
>> > > > - MAX_PICTURE_COUNT * sizeof(Picture), fail_nomem);
>> > > > + MAX_PICTURE_COUNT * sizeof(*s->picture), fail_nomem);
>> > > > for (i = 0; i < MAX_PICTURE_COUNT; i++) {
>> > > > s->picture[i].f = av_frame_alloc();
>> > > > if (!s->picture[i].f)
>> > > > -- > 1.8.3.1
>> > > > > _______________________________________________
>> > > > ffmpeg-devel mailing list
>> > > > ffmpeg-devel at ffmpeg.org
>> > > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > > > > To unsubscribe, visit link above, or email
>> > > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>> > > _______________________________________________
>> > > ffmpeg-devel mailing list
>> > > ffmpeg-devel at ffmpeg.org
>> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > >
>> > > To unsubscribe, visit link above, or email
>> > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>> >
>> > --
>> > Thanks,
>> > Limin Wang
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel at ffmpeg.org
>> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>> > To unsubscribe, visit link above, or email
>> > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
> --
> Thanks,
> Limin Wang
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list