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

Ganesh Ajjanagadde gajjanag at mit.edu
Wed Oct 7 04:50:10 CEST 2015


On Sat, Oct 3, 2015 at 9:14 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Thu, Sep 17, 2015 at 7: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.
>
>
> How does it change? I don't think that's necessarily a terrible thing. If
> it's just cosmetic, we can live with it.

I clarified how it changes in the commit message for completeness - it
is just cosmetic. See updated patch.

>
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list