[FFmpeg-devel] [PATCH] avcodec/libx264: silence -Waddress

[Ganesh Ajjanagadde]

On Thu, Sep 17, 2015 at 6:03 AM, Ganesh Ajjanagadde
<gajjanagadde at gmail.com> wrote:
> On Thu, Sep 17, 2015 at 6:15 AM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>> On Thu, Sep 17, 2015 at 12:19 AM, Ganesh Ajjanagadde
>> <gajjanagadde at gmail.com> wrote:
>>> This patch moves the pointer validity check outside the macro,
>>> and silences the -Waddress observed with GCC 5.2.
>>>
>>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>>> ---
>>>  libavcodec/libx264.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
>>> index 58fcfb0..c7c772e 100644
>>> --- a/libavcodec/libx264.c
>>> +++ b/libavcodec/libx264.c
>>> @@ -346,7 +346,7 @@ static av_cold int X264_close(AVCodecContext *avctx)
>>>  #define OPT_STR(opt, param)                                                   \
>>>      do {                                                                      \
>>>          int ret;                                                              \
>>> -        if (param && (ret = x264_param_parse(&x4->params, opt, param)) < 0) { \
>>> +        if ((ret = x264_param_parse(&x4->params, opt, param)) < 0) { \
>>>              if(ret == X264_PARAM_BAD_NAME)                                    \
>>>                  av_log(avctx, AV_LOG_ERROR,                                   \
>>>                          "bad option '%s': '%s'\n", opt, param);               \
>>> @@ -437,7 +437,8 @@ static av_cold int X264_init(AVCodecContext *avctx)
>>>      x4->params.i_log_level          = X264_LOG_DEBUG;
>>>      x4->params.i_csp                = convert_pix_fmt(avctx->pix_fmt);
>>>
>>> -    OPT_STR("weightp", x4->wpredp);
>>> +    if (x4->wpredp)
>>> +        OPT_STR("weightp", x4->wpredp);
>>>
>>>      if (avctx->bit_rate) {
>>>          x4->params.rc.i_bitrate   = avctx->bit_rate / 1000;
>>> @@ -467,7 +468,8 @@ static av_cold int X264_init(AVCodecContext *avctx)
>>>              (float)avctx->rc_initial_buffer_occupancy / avctx->rc_buffer_size;
>>>      }
>>>
>>> -    OPT_STR("level", x4->level);
>>> +    if (x4->level)
>>> +        OPT_STR("level", x4->level);
>>>
>>>      if (avctx->i_quant_factor > 0)
>>>          x4->params.rc.f_ip_factor         = 1 / fabs(avctx->i_quant_factor);
>>
>>
>> Instead of adding explicit checks here, why not make the file more
>> consistent and use PARSE_X264_OPT for the things from the x4 context
>> (like its already done for a bunch of other variables), and only use
>> OPT_STR for the two special cases further down (without the check
>> then)
>
> The behavior then won't be identical before and after the patch; e.g
> the log portion of PARSE_X264_OPT is different from that of OPT_STR.
> The current patch retains identical behavior. In particular, your
> change does change the "user-facing" output slightly. Unless you (or
> someone else) can confirm that it is irrelevant; I do not think your
> proposal is good.

Ping, can someone confirm either way so that this can move forward?

>
>>
>> - Hendrik