Message ID | 20230312194230.175999-1-jeebjp@gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] avutil/frame: move wipe_side_data counter to its utilized scope | expand |
Context | Check | Description |
---|---|---|
yinshiyou/make_loongarch64 | success | Make finished |
yinshiyou/make_fate_loongarch64 | success | Make fate finished |
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
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
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;