[FFmpeg-devel] avcodec/vaapi_encode_vp8: Use av_clip_uintp2()

Submitted by Michael Niedermayer on Feb. 16, 2017, 4:20 p.m.

Details

Message ID 20170216162042.3518-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Feb. 16, 2017, 4:20 p.m.
Its used elsewhere for 2^p-1 cliping

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/vaapi_encode_vp8.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Mark Thompson Feb. 16, 2017, 5:11 p.m.
On 16/02/17 16:20, Michael Niedermayer wrote:
> Its used elsewhere for 2^p-1 cliping
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/vaapi_encode_vp8.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode_vp8.c b/libavcodec/vaapi_encode_vp8.c
> index 4a1c85e66c..3d3831c46d 100644
> --- a/libavcodec/vaapi_encode_vp8.c
> +++ b/libavcodec/vaapi_encode_vp8.c
> @@ -161,12 +161,12 @@ static av_cold int vaapi_encode_vp8_configure(AVCodecContext *avctx)
>      VAAPIEncodeContext     *ctx = avctx->priv_data;
>      VAAPIEncodeVP8Context *priv = ctx->priv_data;
>  
> -    priv->q_index_p = av_clip(avctx->global_quality, 0, 127);
> +    priv->q_index_p = av_clip_uintp2(avctx->global_quality, 7);
>      if (avctx->i_quant_factor > 0.0)
> -        priv->q_index_i = av_clip((avctx->global_quality *
> +        priv->q_index_i = av_clip_uintp2((avctx->global_quality *
>                                     avctx->i_quant_factor +
>                                     avctx->i_quant_offset) + 0.5,
> -                                  0, 127);
> +                                  7);
>      else
>          priv->q_index_i = priv->q_index_p;

IMO this makes the code less readable, not more.  It doesn't really matter to anything, though, so commit it if you really want to.

(If this is mainly objecting to the magic number being visible there then please do introduce a constant to hide it rather than making the constant smaller - VP8_QINDEX_RANGE, say, to match <https://tools.ietf.org/html/rfc6386#section-14.1>.)
Michael Niedermayer Feb. 16, 2017, 8:15 p.m.
On Thu, Feb 16, 2017 at 05:11:54PM +0000, Mark Thompson wrote:
> On 16/02/17 16:20, Michael Niedermayer wrote:
> > Its used elsewhere for 2^p-1 cliping
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/vaapi_encode_vp8.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavcodec/vaapi_encode_vp8.c b/libavcodec/vaapi_encode_vp8.c
> > index 4a1c85e66c..3d3831c46d 100644
> > --- a/libavcodec/vaapi_encode_vp8.c
> > +++ b/libavcodec/vaapi_encode_vp8.c
> > @@ -161,12 +161,12 @@ static av_cold int vaapi_encode_vp8_configure(AVCodecContext *avctx)
> >      VAAPIEncodeContext     *ctx = avctx->priv_data;
> >      VAAPIEncodeVP8Context *priv = ctx->priv_data;
> >  
> > -    priv->q_index_p = av_clip(avctx->global_quality, 0, 127);
> > +    priv->q_index_p = av_clip_uintp2(avctx->global_quality, 7);
> >      if (avctx->i_quant_factor > 0.0)
> > -        priv->q_index_i = av_clip((avctx->global_quality *
> > +        priv->q_index_i = av_clip_uintp2((avctx->global_quality *
> >                                     avctx->i_quant_factor +
> >                                     avctx->i_quant_offset) + 0.5,
> > -                                  0, 127);
> > +                                  7);
> >      else
> >          priv->q_index_i = priv->q_index_p;
> 
> IMO this makes the code less readable, not more.  It doesn't really matter to anything, though, so commit it if you really want to.
> 
> (If this is mainly objecting to the magic number being visible there then please do introduce a constant to hide it rather than making the constant smaller - VP8_QINDEX_RANGE, say, to match <https://tools.ietf.org/html/rfc6386#section-14.1>.)

I just suggested it because its the only case we have in the codebase
that matches this:
git grep -E 'av_clip *\(.*, *0 *, *(3|7|15|31|63|127|255|511|1023|2047|4095|8191|16383|32767|65535|131071|262143|524287|1048575|2097151|4194303|8388607|16777215|33554431|67108863|134217727|268435455|536870911|1073741823) *\)'

If we dont use the more optimized code everywhere then finding
cases where it makes a difference and isnt used is harder and not
something i would attempt.

i can also move this into fate-source
(there it might even be possible to keep track of exceptions)
but previous additions to fate-source had opposition, so iam not
doing that unless theres some positive feedback in that direction
first ...

[...]
James Almer Feb. 16, 2017, 8:50 p.m.
On 2/16/2017 5:15 PM, Michael Niedermayer wrote:
> On Thu, Feb 16, 2017 at 05:11:54PM +0000, Mark Thompson wrote:
>> On 16/02/17 16:20, Michael Niedermayer wrote:
>>> Its used elsewhere for 2^p-1 cliping
>>>
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/vaapi_encode_vp8.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libavcodec/vaapi_encode_vp8.c b/libavcodec/vaapi_encode_vp8.c
>>> index 4a1c85e66c..3d3831c46d 100644
>>> --- a/libavcodec/vaapi_encode_vp8.c
>>> +++ b/libavcodec/vaapi_encode_vp8.c
>>> @@ -161,12 +161,12 @@ static av_cold int vaapi_encode_vp8_configure(AVCodecContext *avctx)
>>>      VAAPIEncodeContext     *ctx = avctx->priv_data;
>>>      VAAPIEncodeVP8Context *priv = ctx->priv_data;
>>>  
>>> -    priv->q_index_p = av_clip(avctx->global_quality, 0, 127);
>>> +    priv->q_index_p = av_clip_uintp2(avctx->global_quality, 7);
>>>      if (avctx->i_quant_factor > 0.0)
>>> -        priv->q_index_i = av_clip((avctx->global_quality *
>>> +        priv->q_index_i = av_clip_uintp2((avctx->global_quality *
>>>                                     avctx->i_quant_factor +
>>>                                     avctx->i_quant_offset) + 0.5,
>>> -                                  0, 127);
>>> +                                  7);
>>>      else
>>>          priv->q_index_i = priv->q_index_p;
>>
>> IMO this makes the code less readable, not more.  It doesn't really matter to anything, though, so commit it if you really want to.
>>
>> (If this is mainly objecting to the magic number being visible there then please do introduce a constant to hide it rather than making the constant smaller - VP8_QINDEX_RANGE, say, to match <https://tools.ietf.org/html/rfc6386#section-14.1>.)
> 
> I just suggested it because its the only case we have in the codebase
> that matches this:
> git grep -E 'av_clip *\(.*, *0 *, *(3|7|15|31|63|127|255|511|1023|2047|4095|8191|16383|32767|65535|131071|262143|524287|1048575|2097151|4194303|8388607|16777215|33554431|67108863|134217727|268435455|536870911|1073741823) *\)'
> 
> If we dont use the more optimized code everywhere then finding
> cases where it makes a difference and isnt used is harder and not
> something i would attempt.

These specialized integer av_clip* functions are optimized only for
arm.
On x86 i don't think anyone ever did benchmarks to see if av_clip(),
when the compiler properly uses cmov, isn't faster than all the
arithmetical instructions from the alternatives. Especially intp2.

> 
> i can also move this into fate-source
> (there it might even be possible to keep track of exceptions)
> but previous additions to fate-source had opposition, so iam not
> doing that unless theres some positive feedback in that direction
> first ...
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer Feb. 16, 2017, 11:45 p.m.
On Thu, Feb 16, 2017 at 05:50:58PM -0300, James Almer wrote:
> On 2/16/2017 5:15 PM, Michael Niedermayer wrote:
> > On Thu, Feb 16, 2017 at 05:11:54PM +0000, Mark Thompson wrote:
> >> On 16/02/17 16:20, Michael Niedermayer wrote:
> >>> Its used elsewhere for 2^p-1 cliping
> >>>
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavcodec/vaapi_encode_vp8.c | 6 +++---
> >>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/libavcodec/vaapi_encode_vp8.c b/libavcodec/vaapi_encode_vp8.c
> >>> index 4a1c85e66c..3d3831c46d 100644
> >>> --- a/libavcodec/vaapi_encode_vp8.c
> >>> +++ b/libavcodec/vaapi_encode_vp8.c
> >>> @@ -161,12 +161,12 @@ static av_cold int vaapi_encode_vp8_configure(AVCodecContext *avctx)
> >>>      VAAPIEncodeContext     *ctx = avctx->priv_data;
> >>>      VAAPIEncodeVP8Context *priv = ctx->priv_data;
> >>>  
> >>> -    priv->q_index_p = av_clip(avctx->global_quality, 0, 127);
> >>> +    priv->q_index_p = av_clip_uintp2(avctx->global_quality, 7);
> >>>      if (avctx->i_quant_factor > 0.0)
> >>> -        priv->q_index_i = av_clip((avctx->global_quality *
> >>> +        priv->q_index_i = av_clip_uintp2((avctx->global_quality *
> >>>                                     avctx->i_quant_factor +
> >>>                                     avctx->i_quant_offset) + 0.5,
> >>> -                                  0, 127);
> >>> +                                  7);
> >>>      else
> >>>          priv->q_index_i = priv->q_index_p;
> >>
> >> IMO this makes the code less readable, not more.  It doesn't really matter to anything, though, so commit it if you really want to.
> >>
> >> (If this is mainly objecting to the magic number being visible there then please do introduce a constant to hide it rather than making the constant smaller - VP8_QINDEX_RANGE, say, to match <https://tools.ietf.org/html/rfc6386#section-14.1>.)
> > 
> > I just suggested it because its the only case we have in the codebase
> > that matches this:
> > git grep -E 'av_clip *\(.*, *0 *, *(3|7|15|31|63|127|255|511|1023|2047|4095|8191|16383|32767|65535|131071|262143|524287|1048575|2097151|4194303|8388607|16777215|33554431|67108863|134217727|268435455|536870911|1073741823) *\)'
> > 
> > If we dont use the more optimized code everywhere then finding
> > cases where it makes a difference and isnt used is harder and not
> > something i would attempt.
> 
> These specialized integer av_clip* functions are optimized only for
> arm.

i am aware of that


> On x86 i don't think anyone ever did benchmarks to see if av_clip(),
> when the compiler properly uses cmov, isn't faster than all the
> arithmetical instructions from the alternatives. Especially intp2.

intp2 handles a subset of what av_clip handles, it should not be
slower, if it is slower it can directly be implemented with
av_clip(), i assume here that operations on constants are optimized
out. And i belive all? uses of (u)intp2 are with constants
Also you raise an important point, av_clip* should be benchmarked,
that might be tricky though as it would depend on the surrounding code


[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/vaapi_encode_vp8.c b/libavcodec/vaapi_encode_vp8.c
index 4a1c85e66c..3d3831c46d 100644
--- a/libavcodec/vaapi_encode_vp8.c
+++ b/libavcodec/vaapi_encode_vp8.c
@@ -161,12 +161,12 @@  static av_cold int vaapi_encode_vp8_configure(AVCodecContext *avctx)
     VAAPIEncodeContext     *ctx = avctx->priv_data;
     VAAPIEncodeVP8Context *priv = ctx->priv_data;
 
-    priv->q_index_p = av_clip(avctx->global_quality, 0, 127);
+    priv->q_index_p = av_clip_uintp2(avctx->global_quality, 7);
     if (avctx->i_quant_factor > 0.0)
-        priv->q_index_i = av_clip((avctx->global_quality *
+        priv->q_index_i = av_clip_uintp2((avctx->global_quality *
                                    avctx->i_quant_factor +
                                    avctx->i_quant_offset) + 0.5,
-                                  0, 127);
+                                  7);
     else
         priv->q_index_i = priv->q_index_p;