diff mbox

[FFmpeg-devel] vaapi_vp8: Use VP8_MAX_QUANT instead of magic number

Message ID b833879d-a408-2f32-470b-be0a8be8fd1e@jkqxz.net
State Accepted
Commit a1e83a2f904bb0e29c99ec0c3d57b56c2960f939
Headers show

Commit Message

Mark Thompson Feb. 16, 2017, 8:45 p.m. UTC 
---
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 <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 ...

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(-)

Comments

Michael Niedermayer Feb. 16, 2017, 11:46 p.m. UTC | #1 
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 <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 ...
> 
> 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

thx

[...]
Mark Thompson Feb. 17, 2017, 12:32 a.m. UTC | #2 
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 <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 ...
>>
>> 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
diff mbox

Patch

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;