[FFmpeg-devel] avutil/frame: move wipe_side_data counter to its utilized scope

Originally in 77b2cd7b41d7ec8008b6fac753c04f77824c514c this
counter was separate in av_frame_unref, in which the same counter
was re-utilized multiple times over multiple loops.

This code was then refactored into wipe_side_data as-is in
5d839778b9f3edb682b7f71dde4f80f07c75b098 , keeping the location of
counter initialization. This was unnecessary, as the counter was
no longer utilized outside of the for loop's scope and thus could
reside within the loop itself.
---
 libavutil/frame.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Jan Ekström:
> Originally in 77b2cd7b41d7ec8008b6fac753c04f77824c514c this
> counter was separate in av_frame_unref, in which the same counter
> was re-utilized multiple times over multiple loops.
> 
> This code was then refactored into wipe_side_data as-is in
> 5d839778b9f3edb682b7f71dde4f80f07c75b098 , keeping the location of
> counter initialization. This was unnecessary, as the counter was
> no longer utilized outside of the for loop's scope and thus could
> reside within the loop itself.
> ---
>  libavutil/frame.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 9545477acc..d06a673779 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -74,9 +74,7 @@ static void free_side_data(AVFrameSideData **ptr_sd)
>  
>  static void wipe_side_data(AVFrame *frame)
>  {
> -    int i;
> -
> -    for (i = 0; i < frame->nb_side_data; i++) {
> +    for (int i = 0; i < frame->nb_side_data; i++) {
>          free_side_data(&frame->side_data[i]);
>      }
>      frame->nb_side_data = 0;

Don't create a patch for a single for loop; do this for all for loops in
this file where it is possible.

- Andreas

On Mon, Mar 13, 2023 at 12:10 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Jan Ekström:
> > Originally in 77b2cd7b41d7ec8008b6fac753c04f77824c514c this
> > counter was separate in av_frame_unref, in which the same counter
> > was re-utilized multiple times over multiple loops.
> >
> > This code was then refactored into wipe_side_data as-is in
> > 5d839778b9f3edb682b7f71dde4f80f07c75b098 , keeping the location of
> > counter initialization. This was unnecessary, as the counter was
> > no longer utilized outside of the for loop's scope and thus could
> > reside within the loop itself.
> > ---
> >  libavutil/frame.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/libavutil/frame.c b/libavutil/frame.c
> > index 9545477acc..d06a673779 100644
> > --- a/libavutil/frame.c
> > +++ b/libavutil/frame.c
> > @@ -74,9 +74,7 @@ static void free_side_data(AVFrameSideData **ptr_sd)
> >
> >  static void wipe_side_data(AVFrame *frame)
> >  {
> > -    int i;
> > -
> > -    for (i = 0; i < frame->nb_side_data; i++) {
> > +    for (int i = 0; i < frame->nb_side_data; i++) {
> >          free_side_data(&frame->side_data[i]);
> >      }
> >      frame->nb_side_data = 0;
>
> Don't create a patch for a single for loop; do this for all for loops in
> this file where it is possible.
>

Noted. I just noticed this specific case as I was working on the side
data to avctx stuff for passing through HDR metadata etc to encoders.

Will check the rest of the file for related stuff as I get off $dayjob .

Jan

On Mon, Mar 13, 2023 at 12:10 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Jan Ekström:
> > Originally in 77b2cd7b41d7ec8008b6fac753c04f77824c514c this
> > counter was separate in av_frame_unref, in which the same counter
> > was re-utilized multiple times over multiple loops.
> >
> > This code was then refactored into wipe_side_data as-is in
> > 5d839778b9f3edb682b7f71dde4f80f07c75b098 , keeping the location of
> > counter initialization. This was unnecessary, as the counter was
> > no longer utilized outside of the for loop's scope and thus could
> > reside within the loop itself.
> > ---
> >  libavutil/frame.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/libavutil/frame.c b/libavutil/frame.c
> > index 9545477acc..d06a673779 100644
> > --- a/libavutil/frame.c
> > +++ b/libavutil/frame.c
> > @@ -74,9 +74,7 @@ static void free_side_data(AVFrameSideData **ptr_sd)
> >
> >  static void wipe_side_data(AVFrame *frame)
> >  {
> > -    int i;
> > -
> > -    for (i = 0; i < frame->nb_side_data; i++) {
> > +    for (int i = 0; i < frame->nb_side_data; i++) {
> >          free_side_data(&frame->side_data[i]);
> >      }
> >      frame->nb_side_data = 0;
>
> Don't create a patch for a single for loop; do this for all for loops in
> this file where it is possible.

Alright, went through the loops in avutil/frame.c. Just to understand
your preferences I made two separate commits available in
https://github.com/jeeb/ffmpeg/commits/avutil_wipe_side_data_counter_fixup

First that moves the initialization in cases where there is just a
single loop utilizing that counter.
Second one that moves the initialization also in cases where the same
counter is reset and reused for multiple loops.

So yea, just note which of these I should post on ML and I'll do that :) .

Jan

Jan Ekström:
> On Mon, Mar 13, 2023 at 12:10 AM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
>>
>> Jan Ekström:
>>> Originally in 77b2cd7b41d7ec8008b6fac753c04f77824c514c this
>>> counter was separate in av_frame_unref, in which the same counter
>>> was re-utilized multiple times over multiple loops.
>>>
>>> This code was then refactored into wipe_side_data as-is in
>>> 5d839778b9f3edb682b7f71dde4f80f07c75b098 , keeping the location of
>>> counter initialization. This was unnecessary, as the counter was
>>> no longer utilized outside of the for loop's scope and thus could
>>> reside within the loop itself.
>>> ---
>>>  libavutil/frame.c | 4 +---
>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>>> index 9545477acc..d06a673779 100644
>>> --- a/libavutil/frame.c
>>> +++ b/libavutil/frame.c
>>> @@ -74,9 +74,7 @@ static void free_side_data(AVFrameSideData **ptr_sd)
>>>
>>>  static void wipe_side_data(AVFrame *frame)
>>>  {
>>> -    int i;
>>> -
>>> -    for (i = 0; i < frame->nb_side_data; i++) {
>>> +    for (int i = 0; i < frame->nb_side_data; i++) {
>>>          free_side_data(&frame->side_data[i]);
>>>      }
>>>      frame->nb_side_data = 0;
>>
>> Don't create a patch for a single for loop; do this for all for loops in
>> this file where it is possible.
> 
> Alright, went through the loops in avutil/frame.c. Just to understand
> your preferences I made two separate commits available in
> https://github.com/jeeb/ffmpeg/commits/avutil_wipe_side_data_counter_fixup
> 
> First that moves the initialization in cases where there is just a
> single loop utilizing that counter.
> Second one that moves the initialization also in cases where the same
> counter is reset and reused for multiple loops.
> 
> So yea, just note which of these I should post on ML and I'll do that :) .
> 

The second one (or rather: one that combines both).

- Andreas