Re: [FFmpeg-devel] [PATCH 4/4] avcodec/apedec: use ff_clz() instead of while loop
On Thu, Oct 08, 2020 at 09:33:35PM +0200, Michael Niedermayer wrote: > On Wed, Oct 07, 2020 at 05:12:47PM +0200, Paul B Mahol wrote: > > On Wed, Oct 07, 2020 at 04:45:56PM +0200, Michael Niedermayer wrote: > > > On Tue, Oct 06, 2020 at 12:08:27PM +0200, Anton Khirnov wrote: > > > > Quoting Paul B Mahol (2020-10-06 10:19:13) > > > > > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > > > > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > > > > > Signed-off-by: Paul B Mahol > > > > > > > --- > > > > > > > libavcodec/apedec.c | 10 +++--- > > > > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > > > > > index 8fe7b5ee86..ea36247eb9 100644 > > > > > > > --- a/libavcodec/apedec.c > > > > > > > +++ b/libavcodec/apedec.c > > > > > > > @@ -575,14 +575,10 @@ static inline int > > > > > > > ape_decode_value_3990(APEContext *ctx, APERice *rice) > > > > > > > base = range_decode_culfreq(ctx, pivot); > > > > > > > range_decode_update(ctx, 1, base); > > > > > > > } else { > > > > > > > -int base_hi = pivot, base_lo; > > > > > > > -int bbits = 0; > > > > > > > +int base_hi, base_lo; > > > > > > > +int bbits = 16 - ff_clz(pivot); > > > > > > > > > > > > This assumes unsigned is always 32bit, no? > > > > > > > > > > ksum is 32 bit, from which pivot is derived. > > > > > > > > > > Should I use explicitly uint32_t type instead? > > > > > Where is unsigned not 32bit? > > > > > > > > I don't think uint32_t would help, as ff_clz() can expand to a compiler > > > > builtin. > > > > > > > > What I'm concerned about is the unstated assumption about > > > > sizeof(int/unsigned) == 4 spreading through the codebase. It's already > > > > present in plenty of places, so I certainly don't intend to block your > > > > patch over this. But we should consider explitly documenting this, and > > > > maybe testing in configure. > > > > > > At least in code i wrote and write i consider it a bug if it would > > > assume sizeof(int/unsigned) == 4 > > > > > > We also could add a ILP64 fate target, that way we would better understand > > > how much really assumes 32bit int > > > > So you basically saying we should not use ff_clz() at all because of this. > > Then we should remove it from our code. > > i guess thats the conclusion yes or > the hardcoded 32 that is used by code calling it would need to be changed Will apply this ape patchset soon, without this last controversial patch. > > thx > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Rewriting code that is poorly written but fully understood is good. > Rewriting code that one doesnt understand is a sign that one is less smart > then the original author, trying to rewrite it will not make it better. > ___ > 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 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 4/4] avcodec/apedec: use ff_clz() instead of while loop
On Wed, Oct 07, 2020 at 05:12:47PM +0200, Paul B Mahol wrote: > On Wed, Oct 07, 2020 at 04:45:56PM +0200, Michael Niedermayer wrote: > > On Tue, Oct 06, 2020 at 12:08:27PM +0200, Anton Khirnov wrote: > > > Quoting Paul B Mahol (2020-10-06 10:19:13) > > > > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > > > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > > > > Signed-off-by: Paul B Mahol > > > > > > --- > > > > > > libavcodec/apedec.c | 10 +++--- > > > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > > > > index 8fe7b5ee86..ea36247eb9 100644 > > > > > > --- a/libavcodec/apedec.c > > > > > > +++ b/libavcodec/apedec.c > > > > > > @@ -575,14 +575,10 @@ static inline int > > > > > > ape_decode_value_3990(APEContext *ctx, APERice *rice) > > > > > > base = range_decode_culfreq(ctx, pivot); > > > > > > range_decode_update(ctx, 1, base); > > > > > > } else { > > > > > > -int base_hi = pivot, base_lo; > > > > > > -int bbits = 0; > > > > > > +int base_hi, base_lo; > > > > > > +int bbits = 16 - ff_clz(pivot); > > > > > > > > > > This assumes unsigned is always 32bit, no? > > > > > > > > ksum is 32 bit, from which pivot is derived. > > > > > > > > Should I use explicitly uint32_t type instead? > > > > Where is unsigned not 32bit? > > > > > > I don't think uint32_t would help, as ff_clz() can expand to a compiler > > > builtin. > > > > > > What I'm concerned about is the unstated assumption about > > > sizeof(int/unsigned) == 4 spreading through the codebase. It's already > > > present in plenty of places, so I certainly don't intend to block your > > > patch over this. But we should consider explitly documenting this, and > > > maybe testing in configure. > > > > At least in code i wrote and write i consider it a bug if it would > > assume sizeof(int/unsigned) == 4 > > > > We also could add a ILP64 fate target, that way we would better understand > > how much really assumes 32bit int > > So you basically saying we should not use ff_clz() at all because of this. > Then we should remove it from our code. i guess thats the conclusion yes or the hardcoded 32 that is used by code calling it would need to be changed thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better. 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 4/4] avcodec/apedec: use ff_clz() instead of while loop
Quoting Michael Niedermayer (2020-10-07 16:45:56) > On Tue, Oct 06, 2020 at 12:08:27PM +0200, Anton Khirnov wrote: > > Quoting Paul B Mahol (2020-10-06 10:19:13) > > > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > > > Signed-off-by: Paul B Mahol > > > > > --- > > > > > libavcodec/apedec.c | 10 +++--- > > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > > > index 8fe7b5ee86..ea36247eb9 100644 > > > > > --- a/libavcodec/apedec.c > > > > > +++ b/libavcodec/apedec.c > > > > > @@ -575,14 +575,10 @@ static inline int > > > > > ape_decode_value_3990(APEContext *ctx, APERice *rice) > > > > > base = range_decode_culfreq(ctx, pivot); > > > > > range_decode_update(ctx, 1, base); > > > > > } else { > > > > > -int base_hi = pivot, base_lo; > > > > > -int bbits = 0; > > > > > +int base_hi, base_lo; > > > > > +int bbits = 16 - ff_clz(pivot); > > > > > > > > This assumes unsigned is always 32bit, no? > > > > > > ksum is 32 bit, from which pivot is derived. > > > > > > Should I use explicitly uint32_t type instead? > > > Where is unsigned not 32bit? > > > > I don't think uint32_t would help, as ff_clz() can expand to a compiler > > builtin. > > > > What I'm concerned about is the unstated assumption about > > sizeof(int/unsigned) == 4 spreading through the codebase. It's already > > present in plenty of places, so I certainly don't intend to block your > > patch over this. But we should consider explitly documenting this, and > > maybe testing in configure. > > At least in code i wrote and write i consider it a bug if it would > assume sizeof(int/unsigned) == 4 It is my impression (not sure how justified) that there are many overflow checks that cast a multiplication of two ints to int64, thus assuming that int64 is strictly larger. E.g. 6b7bcd437e5 > > We also could add a ILP64 fate target, that way we would better understand > how much really assumes 32bit int I suppose that would be interesting. -- Anton Khirnov ___ 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 4/4] avcodec/apedec: use ff_clz() instead of while loop
On Wed, Oct 07, 2020 at 04:45:56PM +0200, Michael Niedermayer wrote: > On Tue, Oct 06, 2020 at 12:08:27PM +0200, Anton Khirnov wrote: > > Quoting Paul B Mahol (2020-10-06 10:19:13) > > > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > > > Signed-off-by: Paul B Mahol > > > > > --- > > > > > libavcodec/apedec.c | 10 +++--- > > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > > > index 8fe7b5ee86..ea36247eb9 100644 > > > > > --- a/libavcodec/apedec.c > > > > > +++ b/libavcodec/apedec.c > > > > > @@ -575,14 +575,10 @@ static inline int > > > > > ape_decode_value_3990(APEContext *ctx, APERice *rice) > > > > > base = range_decode_culfreq(ctx, pivot); > > > > > range_decode_update(ctx, 1, base); > > > > > } else { > > > > > -int base_hi = pivot, base_lo; > > > > > -int bbits = 0; > > > > > +int base_hi, base_lo; > > > > > +int bbits = 16 - ff_clz(pivot); > > > > > > > > This assumes unsigned is always 32bit, no? > > > > > > ksum is 32 bit, from which pivot is derived. > > > > > > Should I use explicitly uint32_t type instead? > > > Where is unsigned not 32bit? > > > > I don't think uint32_t would help, as ff_clz() can expand to a compiler > > builtin. > > > > What I'm concerned about is the unstated assumption about > > sizeof(int/unsigned) == 4 spreading through the codebase. It's already > > present in plenty of places, so I certainly don't intend to block your > > patch over this. But we should consider explitly documenting this, and > > maybe testing in configure. > > At least in code i wrote and write i consider it a bug if it would > assume sizeof(int/unsigned) == 4 > > We also could add a ILP64 fate target, that way we would better understand > how much really assumes 32bit int So you basically saying we should not use ff_clz() at all because of this. Then we should remove it from our code. > > thx > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > In a rich man's house there is no place to spit but his face. > -- Diogenes of Sinope > ___ > 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 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 4/4] avcodec/apedec: use ff_clz() instead of while loop
On Tue, Oct 06, 2020 at 12:08:27PM +0200, Anton Khirnov wrote: > Quoting Paul B Mahol (2020-10-06 10:19:13) > > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > > Signed-off-by: Paul B Mahol > > > > --- > > > > libavcodec/apedec.c | 10 +++--- > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > > index 8fe7b5ee86..ea36247eb9 100644 > > > > --- a/libavcodec/apedec.c > > > > +++ b/libavcodec/apedec.c > > > > @@ -575,14 +575,10 @@ static inline int > > > > ape_decode_value_3990(APEContext *ctx, APERice *rice) > > > > base = range_decode_culfreq(ctx, pivot); > > > > range_decode_update(ctx, 1, base); > > > > } else { > > > > -int base_hi = pivot, base_lo; > > > > -int bbits = 0; > > > > +int base_hi, base_lo; > > > > +int bbits = 16 - ff_clz(pivot); > > > > > > This assumes unsigned is always 32bit, no? > > > > ksum is 32 bit, from which pivot is derived. > > > > Should I use explicitly uint32_t type instead? > > Where is unsigned not 32bit? > > I don't think uint32_t would help, as ff_clz() can expand to a compiler > builtin. > > What I'm concerned about is the unstated assumption about > sizeof(int/unsigned) == 4 spreading through the codebase. It's already > present in plenty of places, so I certainly don't intend to block your > patch over this. But we should consider explitly documenting this, and > maybe testing in configure. At least in code i wrote and write i consider it a bug if it would assume sizeof(int/unsigned) == 4 We also could add a ILP64 fate target, that way we would better understand how much really assumes 32bit int thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In a rich man's house there is no place to spit but his face. -- Diogenes of Sinope 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 4/4] avcodec/apedec: use ff_clz() instead of while loop
Quoting Paul B Mahol (2020-10-06 10:19:13) > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > Signed-off-by: Paul B Mahol > > > --- > > > libavcodec/apedec.c | 10 +++--- > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > index 8fe7b5ee86..ea36247eb9 100644 > > > --- a/libavcodec/apedec.c > > > +++ b/libavcodec/apedec.c > > > @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext > > > *ctx, APERice *rice) > > > base = range_decode_culfreq(ctx, pivot); > > > range_decode_update(ctx, 1, base); > > > } else { > > > -int base_hi = pivot, base_lo; > > > -int bbits = 0; > > > +int base_hi, base_lo; > > > +int bbits = 16 - ff_clz(pivot); > > > > This assumes unsigned is always 32bit, no? > > ksum is 32 bit, from which pivot is derived. > > Should I use explicitly uint32_t type instead? > Where is unsigned not 32bit? I don't think uint32_t would help, as ff_clz() can expand to a compiler builtin. What I'm concerned about is the unstated assumption about sizeof(int/unsigned) == 4 spreading through the codebase. It's already present in plenty of places, so I certainly don't intend to block your patch over this. But we should consider explitly documenting this, and maybe testing in configure. -- Anton Khirnov ___ 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 4/4] avcodec/apedec: use ff_clz() instead of while loop
On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > Quoting Paul B Mahol (2020-10-06 02:17:14) > > Signed-off-by: Paul B Mahol > > --- > > libavcodec/apedec.c | 10 +++--- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > index 8fe7b5ee86..ea36247eb9 100644 > > --- a/libavcodec/apedec.c > > +++ b/libavcodec/apedec.c > > @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext > > *ctx, APERice *rice) > > base = range_decode_culfreq(ctx, pivot); > > range_decode_update(ctx, 1, base); > > } else { > > -int base_hi = pivot, base_lo; > > -int bbits = 0; > > +int base_hi, base_lo; > > +int bbits = 16 - ff_clz(pivot); > > This assumes unsigned is always 32bit, no? ksum is 32 bit, from which pivot is derived. Should I use explicitly uint32_t type instead? Where is unsigned not 32bit? > > -- > Anton Khirnov > ___ > 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 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 4/4] avcodec/apedec: use ff_clz() instead of while loop
Quoting Paul B Mahol (2020-10-06 02:17:14) > Signed-off-by: Paul B Mahol > --- > libavcodec/apedec.c | 10 +++--- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > index 8fe7b5ee86..ea36247eb9 100644 > --- a/libavcodec/apedec.c > +++ b/libavcodec/apedec.c > @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext > *ctx, APERice *rice) > base = range_decode_culfreq(ctx, pivot); > range_decode_update(ctx, 1, base); > } else { > -int base_hi = pivot, base_lo; > -int bbits = 0; > +int base_hi, base_lo; > +int bbits = 16 - ff_clz(pivot); This assumes unsigned is always 32bit, no? -- Anton Khirnov ___ 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 4/4] avcodec/apedec: use ff_clz() instead of while loop
Signed-off-by: Paul B Mahol --- libavcodec/apedec.c | 10 +++--- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c index 8fe7b5ee86..ea36247eb9 100644 --- a/libavcodec/apedec.c +++ b/libavcodec/apedec.c @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext *ctx, APERice *rice) base = range_decode_culfreq(ctx, pivot); range_decode_update(ctx, 1, base); } else { -int base_hi = pivot, base_lo; -int bbits = 0; +int base_hi, base_lo; +int bbits = 16 - ff_clz(pivot); -while (base_hi & ~0x) { -base_hi >>= 1; -bbits++; -} -base_hi = range_decode_culfreq(ctx, base_hi + 1); +base_hi = range_decode_culfreq(ctx, (pivot >> bbits) + 1); range_decode_update(ctx, 1, base_hi); base_lo = range_decode_culfreq(ctx, 1 << bbits); range_decode_update(ctx, 1, base_lo); -- 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".