| Message ID | 20231022215113.3469-5-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | a5259f326bcaf933a25df64aa49417c25990f7d3 |
| Headers | show |
| Series | [FFmpeg-devel,1/9] avcodec/vlc: merge lost 16bit end of array check | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
On 10/22/23 17:51, Michael Niedermayer wrote: > This makes the code more testable as uninitialized fields are 0 > and not random values from the last call > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/vlc.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/libavcodec/vlc.c b/libavcodec/vlc.c > index 9b7a42f79a3..4adec2da705 100644 > --- a/libavcodec/vlc.c > +++ b/libavcodec/vlc.c > @@ -356,7 +356,7 @@ static void add_level(VLC_MULTI_ELEM *table, const int is16bit, > uint32_t curcode, int curlen, > int curlimit, int curlevel, > const int minlen, const int max, > - unsigned* levelcnt, VLC_MULTI_ELEM *info) > + unsigned* levelcnt, VLC_MULTI_ELEM info) Is passing a struct by value advisable? Did you benchmark this? How does it compare to memset(info, 0, sizeof(*info))? - Leo Izen (Traneptora)
On Mon, Oct 23, 2023 at 02:10:35AM -0400, Leo Izen wrote: > On 10/22/23 17:51, Michael Niedermayer wrote: > > This makes the code more testable as uninitialized fields are 0 > > and not random values from the last call > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/vlc.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/libavcodec/vlc.c b/libavcodec/vlc.c > > index 9b7a42f79a3..4adec2da705 100644 > > --- a/libavcodec/vlc.c > > +++ b/libavcodec/vlc.c > > @@ -356,7 +356,7 @@ static void add_level(VLC_MULTI_ELEM *table, const int is16bit, > > uint32_t curcode, int curlen, > > int curlimit, int curlevel, > > const int minlen, const int max, > > - unsigned* levelcnt, VLC_MULTI_ELEM *info) > > + unsigned* levelcnt, VLC_MULTI_ELEM info) > > > Is passing a struct by value advisable? Did you benchmark this? How does it > compare to memset(info, 0, sizeof(*info))? The struct is 8 bytes, a pointer on 64bit arch is also 8byte I did not benchmark, I think this code doesnt run that many iterations (when its not buggy), I mean each iteration adds a entry to the table and the table will normally be designed to fit in cache and its only for initializing. do you still want me to bechmark this ? thx [...]
On 10/23/23 12:04, Michael Niedermayer wrote: > On Mon, Oct 23, 2023 at 02:10:35AM -0400, Leo Izen wrote: >> On 10/22/23 17:51, Michael Niedermayer wrote: >>> This makes the code more testable as uninitialized fields are 0 >>> and not random values from the last call >>> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/vlc.c | 14 +++++++------- >>> 1 file changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/libavcodec/vlc.c b/libavcodec/vlc.c >>> index 9b7a42f79a3..4adec2da705 100644 >>> --- a/libavcodec/vlc.c >>> +++ b/libavcodec/vlc.c >>> @@ -356,7 +356,7 @@ static void add_level(VLC_MULTI_ELEM *table, const int is16bit, >>> uint32_t curcode, int curlen, >>> int curlimit, int curlevel, >>> const int minlen, const int max, >>> - unsigned* levelcnt, VLC_MULTI_ELEM *info) >>> + unsigned* levelcnt, VLC_MULTI_ELEM info) >> >> >> Is passing a struct by value advisable? Did you benchmark this? How does it >> compare to memset(info, 0, sizeof(*info))? > > The struct is 8 bytes, a pointer on 64bit arch is also 8byte > > I did not benchmark, I think this code doesnt run that many iterations > (when its not buggy), I mean each iteration adds a entry to the table > and the table will normally be designed to fit in cache and its only > for initializing. > > do you still want me to bechmark this ? > > thx > If the struct is only 8 bytes it's probably not necessary. - Leo Izen (Traneptora)
On Tue, Oct 24, 2023 at 05:54:37AM -0400, Leo Izen wrote: > On 10/23/23 12:04, Michael Niedermayer wrote: > > On Mon, Oct 23, 2023 at 02:10:35AM -0400, Leo Izen wrote: > > > On 10/22/23 17:51, Michael Niedermayer wrote: > > > > This makes the code more testable as uninitialized fields are 0 > > > > and not random values from the last call > > > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavcodec/vlc.c | 14 +++++++------- > > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/libavcodec/vlc.c b/libavcodec/vlc.c > > > > index 9b7a42f79a3..4adec2da705 100644 > > > > --- a/libavcodec/vlc.c > > > > +++ b/libavcodec/vlc.c > > > > @@ -356,7 +356,7 @@ static void add_level(VLC_MULTI_ELEM *table, const int is16bit, > > > > uint32_t curcode, int curlen, > > > > int curlimit, int curlevel, > > > > const int minlen, const int max, > > > > - unsigned* levelcnt, VLC_MULTI_ELEM *info) > > > > + unsigned* levelcnt, VLC_MULTI_ELEM info) > > > > > > > > > Is passing a struct by value advisable? Did you benchmark this? How does it > > > compare to memset(info, 0, sizeof(*info))? > > > > The struct is 8 bytes, a pointer on 64bit arch is also 8byte > > > > I did not benchmark, I think this code doesnt run that many iterations > > (when its not buggy), I mean each iteration adds a entry to the table > > and the table will normally be designed to fit in cache and its only > > for initializing. > > > > do you still want me to bechmark this ? > > > > thx > > > > If the struct is only 8 bytes it's probably not necessary. will apply patches 3-5 thx [...]
diff --git a/libavcodec/vlc.c b/libavcodec/vlc.c index 9b7a42f79a3..4adec2da705 100644 --- a/libavcodec/vlc.c +++ b/libavcodec/vlc.c @@ -356,7 +356,7 @@ static void add_level(VLC_MULTI_ELEM *table, const int is16bit, uint32_t curcode, int curlen, int curlimit, int curlevel, const int minlen, const int max, - unsigned* levelcnt, VLC_MULTI_ELEM *info) + unsigned* levelcnt, VLC_MULTI_ELEM info) { int max_symbols = VLC_MULTI_MAX_SYMBOLS >> is16bit; for (int i = num-1; i >= max; i--) { @@ -372,16 +372,16 @@ static void add_level(VLC_MULTI_ELEM *table, const int is16bit, code = curcode + (buf[t].code >> curlen); newlimit = curlimit - l; l += curlen; - if (is16bit) AV_WN16(info->val+2*curlevel, sym); - else info->val[curlevel] = sym&0xFF; + if (is16bit) AV_WN16(info.val+2*curlevel, sym); + else info.val[curlevel] = sym&0xFF; if (curlevel) { // let's not add single entries uint32_t val = code >> (32 - numbits); uint32_t nb = val + (1U << (numbits - l)); - info->len = l; - info->num = curlevel+1; + info.len = l; + info.num = curlevel+1; for (; val < nb; val++) - AV_COPY64(table+val, info); + AV_COPY64(table+val, &info); levelcnt[curlevel-1]++; } @@ -435,7 +435,7 @@ static int vlc_multi_gen(VLC_MULTI_ELEM *table, const VLC *single, } add_level(table, is16bit, nb_codes, numbits, buf, - 0, 0, FFMIN(maxbits, numbits), 0, minbits, max, count, &info); + 0, 0, FFMIN(maxbits, numbits), 0, minbits, max, count, info); av_log(logctx, AV_LOG_DEBUG, "Joint: %d/%d/%d/%d/%d codes min=%ubits max=%u\n", count[0], count[1], count[2], count[3], count[4], minbits, max);
This makes the code more testable as uninitialized fields are 0 and not random values from the last call Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vlc.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)