| Message ID | 20170216162042.3518-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
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>.)
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 ... [...]
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 >
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 [...]
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;
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(-)