---
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(-)
diff --git a/libavcodec/vaapi_encode_vp8.c b/libavcodec/vaapi_encode_vp8.c
index 4a1c85e66c..423f7483e8 100644
--- a/libavcodec/vaapi_encode_vp8.c
+++ b/libavcodec/vaapi_encode_vp8.c
@@ -28,6 +28,7 @@
#include "avcodec.h"
#include "internal.h"
#include "vaapi_encode.h"
+#include "vp8.h"
typedef struct VAAPIEncodeVP8Context {
@@ -161,12 +162,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(avctx->global_quality, 0, VP8_MAX_QUANT);
if (avctx->i_quant_factor > 0.0)
priv->q_index_i = av_clip((avctx->global_quality *
avctx->i_quant_factor +
avctx->i_quant_offset) + 0.5,
- 0, 127);
+ 0, VP8_MAX_QUANT);
else
priv->q_index_i = priv->q_index_p;
--
2.11.0
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
