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

[Michael Niedermayer]

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 at 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


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170217/e3d2f9ae/attachment.sig>