On 16/02/17 23:46, Michael Niedermayer wrote: > On Thu, Feb 16, 2017 at 08:45:33PM +0000, Mark Thompson wrote: >> --- >> On 16/02/17 20:15, 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 <mich...@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 ... >> >> That does make a lot of sense; I would be in favour of having automated >> checks like that in fate-source. >> >> To remove the issue here, how about this patch instead? The constant >> already exists in the VP8 header, so we can just use it from there. >> >> >> libavcodec/vaapi_encode_vp8.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) > > patch LGTM
Ok, applied. Thanks, - Mark _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
