Re: [FFmpeg-devel] [PATCH 5/9] avcodec/vlc: Pass VLC_MULTI_ELEM directly not by pointer
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 > > > > --- > > > >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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Concerning the gods, I have no means of knowing whether they exist or not or of what sort they may be, because of the obscurity of the subject, and the brevity of human life -- Protagoras signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/9] avcodec/vlc: Pass VLC_MULTI_ELEM directly not by pointer
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 --- 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) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/9] avcodec/vlc: Pass VLC_MULTI_ELEM directly not by pointer
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 > > --- > > 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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you think the mosad wants you dead since a long time then you are either wrong or dead since a long time. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 5/9] avcodec/vlc: Pass VLC_MULTI_ELEM directly not by pointer
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 --- 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) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 5/9] avcodec/vlc: Pass VLC_MULTI_ELEM directly not by pointer
This makes the code more testable as uninitialized fields are 0 and not random values from the last call Signed-off-by: Michael Niedermayer --- 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) { 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); -- 2.17.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".