[FFmpeg-devel] [PATCH v1 1/3] avfilter/f_sidedata: try to fix warning: comparison of constant -1 with expression of type 'enum AVFrameSideDataType'

[Marton Balint]

On Thu, 19 Sep 2019, Limin Wang wrote:

> On Wed, Sep 18, 2019 at 08:23:46PM +0200, Michael Niedermayer wrote:
>> On Sun, Aug 25, 2019 at 12:17:58AM +0800, lance.lmwang at gmail.com wrote:
>> > From: Limin Wang <lance.lmwang at gmail.com>
>> > 
>> > Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
>> > ---
>> >  libavfilter/f_sidedata.c | 10 +++++-----
>> >  libavutil/frame.h        | 10 ++++++++++
>> >  2 files changed, 15 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/libavfilter/f_sidedata.c b/libavfilter/f_sidedata.c
>> > index 381da5a..3082aa6 100644
>> > --- a/libavfilter/f_sidedata.c
>> > +++ b/libavfilter/f_sidedata.c
>> > @@ -49,7 +49,7 @@ static const AVOption filt_name##_options[] = { \
>> >      { "mode", "set a mode of operation", OFFSET(mode),   AV_OPT_TYPE_INT,    {.i64 = 0 }, 0, SIDEDATA_NB-1, FLAGS, "mode" }, \
>> >      {   "select", "select frame",        0,              AV_OPT_TYPE_CONST,  {.i64 = SIDEDATA_SELECT }, 0, 0, FLAGS, "mode" }, \
>> >      {   "delete", "delete side data",    0,              AV_OPT_TYPE_CONST,  {.i64 = SIDEDATA_DELETE }, 0, 0, FLAGS, "mode" }, \
>> > -    { "type",   "set side data type",    OFFSET(type),   AV_OPT_TYPE_INT,    {.i64 = -1 }, -1, INT_MAX, FLAGS, "type" }, \
>> > +    { "type",   "set side data type",    OFFSET(type),   AV_OPT_TYPE_INT,    {.i64 = AV_FRAME_DATA_NB }, 0, AV_FRAME_DATA_NB, FLAGS, "type" }, \
>> >      {   "PANSCAN",                    "", 0,             AV_OPT_TYPE_CONST,  {.i64 = AV_FRAME_DATA_PANSCAN                    }, 0, 0, FLAGS, "type" }, \
>> >      {   "A53_CC",                     "", 0,             AV_OPT_TYPE_CONST,  {.i64 = AV_FRAME_DATA_A53_CC                     }, 0, 0, FLAGS, "type" }, \
>> >      {   "STEREO3D",                   "", 0,             AV_OPT_TYPE_CONST,  {.i64 = AV_FRAME_DATA_STEREO3D                   }, 0, 0, FLAGS, "type" }, \
>> > @@ -79,7 +79,7 @@ static const AVOption filt_name##_options[] = { \
>> >      { "mode", "set a mode of operation", OFFSET(mode),   AV_OPT_TYPE_INT,    {.i64 = 0 }, 0, SIDEDATA_NB-1, FLAGS, "mode" }, \
>> >      {   "select", "select frame",        0,              AV_OPT_TYPE_CONST,  {.i64 = SIDEDATA_SELECT }, 0, 0, FLAGS, "mode" }, \
>> >      {   "delete", "delete side data",    0,              AV_OPT_TYPE_CONST,  {.i64 = SIDEDATA_DELETE }, 0, 0, FLAGS, "mode" }, \
>> > -    { "type",   "set side data type",    OFFSET(type),   AV_OPT_TYPE_INT,    {.i64 = -1 }, -1, INT_MAX, FLAGS, "type" }, \
>> > +    { "type",   "set side data type",    OFFSET(type),   AV_OPT_TYPE_INT,    {.i64 = AV_FRAME_DATA_NB }, 0, AV_FRAME_DATA_NB, FLAGS, "type" }, \
>> >      {   "PANSCAN",                    "", 0,             AV_OPT_TYPE_CONST,  {.i64 = AV_FRAME_DATA_PANSCAN                    }, 0, 0, FLAGS, "type" }, \
>> >      {   "A53_CC",                     "", 0,             AV_OPT_TYPE_CONST,  {.i64 = AV_FRAME_DATA_A53_CC                     }, 0, 0, FLAGS, "type" }, \
>> >      {   "STEREO3D",                   "", 0,             AV_OPT_TYPE_CONST,  {.i64 = AV_FRAME_DATA_STEREO3D                   }, 0, 0, FLAGS, "type" }, \
>> > @@ -107,7 +107,7 @@ static av_cold int init(AVFilterContext *ctx)
>> >  {
>> >      SideDataContext *s = ctx->priv;
>> > 
>> > -    if (s->type == -1 && s->mode != SIDEDATA_DELETE) {
>> > +    if (s->type == AV_FRAME_DATA_NB && s->mode != SIDEDATA_DELETE) {
>> >          av_log(ctx, AV_LOG_ERROR, "Side data type must be set\n");
>> >          return AVERROR(EINVAL);
>> >      }
>> > @@ -122,7 +122,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
>> >      SideDataContext *s = ctx->priv;
>> >      AVFrameSideData *sd = NULL;
>> > 
>> > -    if (s->type != -1)
>> > +    if (s->type != AV_FRAME_DATA_NB)
>> >         sd = av_frame_get_side_data(frame, s->type);
>> > 
>> >      switch (s->mode) {
>> > @@ -132,7 +132,7 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
>> >          }
>> >          break;
>> >      case SIDEDATA_DELETE:
>> > -        if (s->type == -1) {
>> > +        if (s->type == AV_FRAME_DATA_NB) {
>> >              while (frame->nb_side_data)
>> >                  av_frame_remove_side_data(frame, frame->side_data[0]->type);
>> >          } else if (sd) {
>> > diff --git a/libavutil/frame.h b/libavutil/frame.h
>> > index 5d3231e..4b83125 100644
>> > --- a/libavutil/frame.h
>> > +++ b/libavutil/frame.h
>> > @@ -179,6 +179,16 @@ enum AVFrameSideDataType {
>> >       * array element is implied by AVFrameSideData.size / AVRegionOfInterest.self_size.
>> >       */
>> >      AV_FRAME_DATA_REGIONS_OF_INTEREST,
>> > +
>> > +    /**
>> > +     * The number of side data types.
>> > +     * This is not part of the public API/ABI in the sense that it may
>> > +     * change when new side data types are added.
>> > +     * This must stay the last enum value.
>> > +     * If its value becomes huge, some code using it
>> > +     * needs to be updated as it assumes it to be smaller than other limits.
>> > +     */
>> > +    AV_FRAME_DATA_NB
>> >  };
>> 
>> This looks not really correct
>> this defines a constant in libavutil that can change with libavutils
>> minor version and then use it outside in libavfilter
>> it could then mismatch what is linked at runtime ...
>
> Thanks for the comments, I'll update to bump the minor version of libavutils.

That won't help, as the issue is caused by using non-public API/ABI of 
libavutil in another library (libavfilter).

Also it is not very good idea to change the value of the unset entry from 
-1, as the user might use that to explicitly set the parameter to unset.

If you want to fix the warnings maybe it is better if you simply define 
something like this in f_sidedata.c:

#define UNSET ((enum AVFrameSideDataType)-1)

Regards,
Marton