Re: [FFmpeg-devel] [PATCH] aacenc: avoid double in quantize_bands.

[Reimar Döffinger]

On Wed, Mar 02, 2016 at 07:33:31PM +, Rostislav Pehlivanov wrote:
> On 1 March 2016 at 21:55, Reimar Döffinger  wrote:
> > I cannot see any point whatsoever to use
> > double here instead of float.
> > Using float allows for use of SIMD.
> > ---
> >  libavcodec/aacenc_utils.h | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavcodec/aacenc_utils.h b/libavcodec/aacenc_utils.h
> > index cb5bc8d..571b1e6 100644
> > --- a/libavcodec/aacenc_utils.h
> > +++ b/libavcodec/aacenc_utils.h
> > @@ -66,10 +66,9 @@ static inline void quantize_bands(int *out, const float
> > *in, const float *scaled
> >const float rounding)
> >  {
> >  int i;
> > -double qc;
> >  for (i = 0; i < size; i++) {
> > -qc = scaled[i] * Q34;
> > -out[i] = (int)FFMIN(qc + rounding, (double)maxval);
> > +float qc = scaled[i] * Q34;
> > +out[i] = (int)FFMIN(qc + rounding, (float)maxval);
> >  if (is_signed && in[i] < 0.0f) {
> >  out[i] = -out[i];
> >  }
> > --
> >
> 
> You could just avoid the whole need for qc and just do "FFMIN((scaled[i] *
> Q34) + rounding, (float)maxval));". We have plenty of space and I think it
> would look neater.

I don't like it much because FFMIN is a macro and I don't trust the
compiler's subexpression elimination too much...

> But up to you to decide, either way it looks good to me, doesn't change
> anything. Feel free to push if you can.

Done.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] aacenc: avoid double in quantize_bands.

[Rostislav Pehlivanov]

On 1 March 2016 at 21:55, Reimar Döffinger  wrote:

> I cannot see any point whatsoever to use
> double here instead of float.
> Using float allows for use of SIMD.
> ---
>  libavcodec/aacenc_utils.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/aacenc_utils.h b/libavcodec/aacenc_utils.h
> index cb5bc8d..571b1e6 100644
> --- a/libavcodec/aacenc_utils.h
> +++ b/libavcodec/aacenc_utils.h
> @@ -66,10 +66,9 @@ static inline void quantize_bands(int *out, const float
> *in, const float *scaled
>const float rounding)
>  {
>  int i;
> -double qc;
>  for (i = 0; i < size; i++) {
> -qc = scaled[i] * Q34;
> -out[i] = (int)FFMIN(qc + rounding, (double)maxval);
> +float qc = scaled[i] * Q34;
> +out[i] = (int)FFMIN(qc + rounding, (float)maxval);
>  if (is_signed && in[i] < 0.0f) {
>  out[i] = -out[i];
>  }
> --
>

You could just avoid the whole need for qc and just do "FFMIN((scaled[i] *
Q34) + rounding, (float)maxval));". We have plenty of space and I think it
would look neater.
But up to you to decide, either way it looks good to me, doesn't change
anything. Feel free to push if you can.

Thanks
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] aacenc: avoid double in quantize_bands.

[Reimar Döffinger]

On 02.03.2016, at 04:48, Ganesh Ajjanagadde  wrote:

> On Tue, Mar 1, 2016 at 4:55 PM, Reimar Döffinger
>  wrote:
>> I cannot see any point whatsoever to use
>> double here instead of float.
> 
> There can be some negligible accuracy differences, but I do not see
> any harm myself; aac anyway sticks to floats in most places.

floating-point is difficult, but I really can't see any.
The only thing it changes at all is the addition of the rounding value.
However given that we convert to int after the range of relevant bits is very 
limited, and the rounding value is a fixed value != 0.5.
Thus I suspect that the two are actually exactly equivalent.

>> Using float allows for use of SIMD.
> 
> Not an accurate statement, there is SIMD for doubles as well. More
> precise variant is "allows for more effective/efficient use of SIMD".

Well... I don't know if I'll improve the statement, but the problem is that 
there are conditions in that code, and most SIMD does not have an cmov 
instruction equivalent.
Since SIMD registers generally fit only 2 doubles that basically does mean that 
with doubles using SIMD is in fact not possible (in a way that makes sense/does 
not make it slower).
At the very least the compiler won't do it on its own.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] aacenc: avoid double in quantize_bands.

[Ganesh Ajjanagadde]

On Tue, Mar 1, 2016 at 4:55 PM, Reimar Döffinger
 wrote:
> I cannot see any point whatsoever to use
> double here instead of float.

There can be some negligible accuracy differences, but I do not see
any harm myself; aac anyway sticks to floats in most places.

> Using float allows for use of SIMD.

Not an accurate statement, there is SIMD for doubles as well. More
precise variant is "allows for more effective/efficient use of SIMD".

Patch itself LGTM, but I am not an aac maintainer.

> ---
>  libavcodec/aacenc_utils.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/aacenc_utils.h b/libavcodec/aacenc_utils.h
> index cb5bc8d..571b1e6 100644
> --- a/libavcodec/aacenc_utils.h
> +++ b/libavcodec/aacenc_utils.h
> @@ -66,10 +66,9 @@ static inline void quantize_bands(int *out, const float 
> *in, const float *scaled
>const float rounding)
>  {
>  int i;
> -double qc;
>  for (i = 0; i < size; i++) {
> -qc = scaled[i] * Q34;
> -out[i] = (int)FFMIN(qc + rounding, (double)maxval);
> +float qc = scaled[i] * Q34;
> +out[i] = (int)FFMIN(qc + rounding, (float)maxval);
>  if (is_signed && in[i] < 0.0f) {
>  out[i] = -out[i];
>  }
> --
> 2.7.0
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] aacenc: avoid double in quantize_bands.

[Reimar Döffinger]

I cannot see any point whatsoever to use
double here instead of float.
Using float allows for use of SIMD.
---
 libavcodec/aacenc_utils.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/libavcodec/aacenc_utils.h b/libavcodec/aacenc_utils.h
index cb5bc8d..571b1e6 100644
--- a/libavcodec/aacenc_utils.h
+++ b/libavcodec/aacenc_utils.h
@@ -66,10 +66,9 @@ static inline void quantize_bands(int *out, const float *in, 
const float *scaled
   const float rounding)
 {
 int i;
-double qc;
 for (i = 0; i < size; i++) {
-qc = scaled[i] * Q34;
-out[i] = (int)FFMIN(qc + rounding, (double)maxval);
+float qc = scaled[i] * Q34;
+out[i] = (int)FFMIN(qc + rounding, (float)maxval);
 if (is_signed && in[i] < 0.0f) {
 out[i] = -out[i];
 }
-- 
2.7.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel