diff mbox series

[FFmpeg-devel] lavu/video_enc_params: make sure blocks are properly aligned

Message ID 20210408091734.7797-1-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel] lavu/video_enc_params: make sure blocks are properly aligned
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Anton Khirnov April 8, 2021, 9:17 a.m. UTC
---
 libavutil/video_enc_params.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Andreas Rheinhardt April 8, 2021, 9:41 a.m. UTC | #1
Anton Khirnov:
> ---
>  libavutil/video_enc_params.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
> index 635176ab91..1779a8799a 100644
> --- a/libavutil/video_enc_params.c
> +++ b/libavutil/video_enc_params.c
> @@ -30,9 +30,9 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
>                                              unsigned int nb_blocks, size_t *out_size)
>  {
>      AVVideoEncParams *par;
> -    size_t size;
> +    size_t blocks_offset = FFALIGN(sizeof(*par), 8);
> +    size_t size          = blocks_offset;
>  
> -    size = sizeof(*par);
>      if (nb_blocks > (SIZE_MAX - size) / sizeof(AVVideoBlockParams))
>          return NULL;
>      size += sizeof(AVVideoBlockParams) * nb_blocks;
> @@ -44,7 +44,7 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
>      par->type          = type;
>      par->nb_blocks     = nb_blocks;
>      par->block_size    = sizeof(AVVideoBlockParams);
> -    par->blocks_offset = sizeof(*par);
> +    par->blocks_offset = blocks_offset;
>  
>      if (out_size)
>          *out_size = size;
> 
Wouldn't it be safer to just define a struct { AVVideoEncParams header;
AVVideoBlockParams blocks[1]; } and use the offset of blocks in that?

- Andreas
Anton Khirnov April 8, 2021, 10:11 a.m. UTC | #2
Quoting Andreas Rheinhardt (2021-04-08 11:41:41)
> Anton Khirnov:
> > ---
> >  libavutil/video_enc_params.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
> > index 635176ab91..1779a8799a 100644
> > --- a/libavutil/video_enc_params.c
> > +++ b/libavutil/video_enc_params.c
> > @@ -30,9 +30,9 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
> >                                              unsigned int nb_blocks, size_t *out_size)
> >  {
> >      AVVideoEncParams *par;
> > -    size_t size;
> > +    size_t blocks_offset = FFALIGN(sizeof(*par), 8);
> > +    size_t size          = blocks_offset;
> >  
> > -    size = sizeof(*par);
> >      if (nb_blocks > (SIZE_MAX - size) / sizeof(AVVideoBlockParams))
> >          return NULL;
> >      size += sizeof(AVVideoBlockParams) * nb_blocks;
> > @@ -44,7 +44,7 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
> >      par->type          = type;
> >      par->nb_blocks     = nb_blocks;
> >      par->block_size    = sizeof(AVVideoBlockParams);
> > -    par->blocks_offset = sizeof(*par);
> > +    par->blocks_offset = blocks_offset;
> >  
> >      if (out_size)
> >          *out_size = size;
> > 
> Wouldn't it be safer to just define a struct { AVVideoEncParams header;
> AVVideoBlockParams blocks[1]; } and use the offset of blocks in that?

I don't see how it would be safer and it prevents you from adding new
fields to AVVideoEncParams
Andreas Rheinhardt April 8, 2021, 10:27 a.m. UTC | #3
Anton Khirnov:
> Quoting Andreas Rheinhardt (2021-04-08 11:41:41)
>> Anton Khirnov:
>>> ---
>>>  libavutil/video_enc_params.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
>>> index 635176ab91..1779a8799a 100644
>>> --- a/libavutil/video_enc_params.c
>>> +++ b/libavutil/video_enc_params.c
>>> @@ -30,9 +30,9 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
>>>                                              unsigned int nb_blocks, size_t *out_size)
>>>  {
>>>      AVVideoEncParams *par;
>>> -    size_t size;
>>> +    size_t blocks_offset = FFALIGN(sizeof(*par), 8);
>>> +    size_t size          = blocks_offset;
>>>  
>>> -    size = sizeof(*par);
>>>      if (nb_blocks > (SIZE_MAX - size) / sizeof(AVVideoBlockParams))
>>>          return NULL;
>>>      size += sizeof(AVVideoBlockParams) * nb_blocks;
>>> @@ -44,7 +44,7 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
>>>      par->type          = type;
>>>      par->nb_blocks     = nb_blocks;
>>>      par->block_size    = sizeof(AVVideoBlockParams);
>>> -    par->blocks_offset = sizeof(*par);
>>> +    par->blocks_offset = blocks_offset;
>>>  
>>>      if (out_size)
>>>          *out_size = size;
>>>
>> Wouldn't it be safer to just define a struct { AVVideoEncParams header;
>> AVVideoBlockParams blocks[1]; } and use the offset of blocks in that?
> 
> I don't see how it would be safer and it prevents you from adding new
> fields to AVVideoEncParams
> 
Safer: You are making assumptions about the alignment of
AVVideoBlockParams; these assumptions may or may not be fulfilled in
practice.
And I really don't get your point about this preventing the addition of
new fields to AVVideoEncParams. If something is added to
AVVideoEncParams, sizeof(header) and offsetof(blocks) will grow
automatically as required.

- Andreas
Anton Khirnov April 8, 2021, 12:44 p.m. UTC | #4
Quoting Andreas Rheinhardt (2021-04-08 12:27:03)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2021-04-08 11:41:41)
> >> Anton Khirnov:
> >>> ---
> >>>  libavutil/video_enc_params.c | 6 +++---
> >>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
> >>> index 635176ab91..1779a8799a 100644
> >>> --- a/libavutil/video_enc_params.c
> >>> +++ b/libavutil/video_enc_params.c
> >>> @@ -30,9 +30,9 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
> >>>                                              unsigned int nb_blocks, size_t *out_size)
> >>>  {
> >>>      AVVideoEncParams *par;
> >>> -    size_t size;
> >>> +    size_t blocks_offset = FFALIGN(sizeof(*par), 8);
> >>> +    size_t size          = blocks_offset;
> >>>  
> >>> -    size = sizeof(*par);
> >>>      if (nb_blocks > (SIZE_MAX - size) / sizeof(AVVideoBlockParams))
> >>>          return NULL;
> >>>      size += sizeof(AVVideoBlockParams) * nb_blocks;
> >>> @@ -44,7 +44,7 @@ AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
> >>>      par->type          = type;
> >>>      par->nb_blocks     = nb_blocks;
> >>>      par->block_size    = sizeof(AVVideoBlockParams);
> >>> -    par->blocks_offset = sizeof(*par);
> >>> +    par->blocks_offset = blocks_offset;
> >>>  
> >>>      if (out_size)
> >>>          *out_size = size;
> >>>
> >> Wouldn't it be safer to just define a struct { AVVideoEncParams header;
> >> AVVideoBlockParams blocks[1]; } and use the offset of blocks in that?
> > 
> > I don't see how it would be safer and it prevents you from adding new
> > fields to AVVideoEncParams
> > 
> Safer: You are making assumptions about the alignment of
> AVVideoBlockParams; these assumptions may or may not be fulfilled in
> practice.
> And I really don't get your point about this preventing the addition of
> new fields to AVVideoEncParams. If something is added to
> AVVideoEncParams, sizeof(header) and offsetof(blocks) will grow
> automatically as required.

Nevermind, I misunderstood your suggestion. I suppose it works better
indeed.
diff mbox series

Patch

diff --git a/libavutil/video_enc_params.c b/libavutil/video_enc_params.c
index 635176ab91..1779a8799a 100644
--- a/libavutil/video_enc_params.c
+++ b/libavutil/video_enc_params.c
@@ -30,9 +30,9 @@  AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
                                             unsigned int nb_blocks, size_t *out_size)
 {
     AVVideoEncParams *par;
-    size_t size;
+    size_t blocks_offset = FFALIGN(sizeof(*par), 8);
+    size_t size          = blocks_offset;
 
-    size = sizeof(*par);
     if (nb_blocks > (SIZE_MAX - size) / sizeof(AVVideoBlockParams))
         return NULL;
     size += sizeof(AVVideoBlockParams) * nb_blocks;
@@ -44,7 +44,7 @@  AVVideoEncParams *av_video_enc_params_alloc(enum AVVideoEncParamsType type,
     par->type          = type;
     par->nb_blocks     = nb_blocks;
     par->block_size    = sizeof(AVVideoBlockParams);
-    par->blocks_offset = sizeof(*par);
+    par->blocks_offset = blocks_offset;
 
     if (out_size)
         *out_size = size;