[FFmpeg-devel] avformat/dvdvideodec: remove `if ((ret = ...) < 0)` pattern

Recent advice plus my own experience agree that this pattern
is error-prone. Instead, set `ret` in its own line and do
the error validation after. Also, explicitly return 0 on success
in dvdvideo_chapters_setup_preindex()

Signed-off-by: Marth64 <marth64@proxyid.net>
---
 libavformat/dvdvideodec.c | 132 +++++++++++++++++++++++++-------------
 1 file changed, 86 insertions(+), 46 deletions(-)

On date Tuesday 2024-03-26 15:17:49 -0500, Marth64 wrote:
> Recent advice plus my own experience agree that this pattern
> is error-prone. Instead, set `ret` in its own line and do
> the error validation after. Also, explicitly return 0 on success
> in dvdvideo_chapters_setup_preindex()

Not super convinced this is really useful (it's increasing the line
count and therefore decreasing readability), but will apply.

Marth64:
> Recent advice plus my own experience agree that this pattern
> is error-prone. Instead, set `ret` in its own line and do
> the error validation after. Also, explicitly return 0 on success
> in dvdvideo_chapters_setup_preindex()
> 
> Signed-off-by: Marth64 <marth64@proxyid.net>
> ---
>  libavformat/dvdvideodec.c | 132 +++++++++++++++++++++++++-------------
>  1 file changed, 86 insertions(+), 46 deletions(-)
> 
> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
> index c94e7f7fe6..000f9c5c9b 100644
> --- a/libavformat/dvdvideodec.c
> +++ b/libavformat/dvdvideodec.c
> @@ -900,7 +900,7 @@ static int dvdvideo_chapters_setup_preindex(AVFormatContext *s)
>  {
>      DVDVideoDemuxContext *c = s->priv_data;
>  
> -    int ret = 0, interrupt = 0;
> +    int ret, interrupt = 0;
>      int nb_chapters = 0, last_ptt = c->opt_chapter_start;
>      uint64_t cur_chapter_offset = 0, cur_chapter_duration = 0;
>      DVDVideoPlaybackState state = {0};
> @@ -909,9 +909,10 @@ static int dvdvideo_chapters_setup_preindex(AVFormatContext *s)
>      int nav_event;
>  
>      if (c->opt_chapter_start == c->opt_chapter_end)
> -        return ret;
> +        return 0;
>  
> -    if ((ret = dvdvideo_play_open(s, &state)) < 0)
> +    ret = dvdvideo_play_open(s, &state);
> +    if (ret < 0)
>          return ret;
>  
>      if (state.pgc->nr_of_programs == 1)
> @@ -1058,7 +1059,7 @@ static int dvdvideo_video_stream_setup(AVFormatContext *s)
>  {
>      DVDVideoDemuxContext *c = s->priv_data;
>  
> -    int ret = 0;
> +    int ret;
>      DVDVideoVTSVideoStreamEntry entry = {0};
>      video_attr_t video_attr;
>  
> @@ -1068,14 +1069,11 @@ static int dvdvideo_video_stream_setup(AVFormatContext *s)
>      else
>          video_attr = c->vts_ifo->vtsi_mat->vts_video_attr;
>  
> -    if ((ret = dvdvideo_video_stream_analyze(s, video_attr, &entry)) < 0 ||
> -        (ret = dvdvideo_video_stream_add(s, &entry, AVSTREAM_PARSE_HEADERS)) < 0) {
> -
> -        av_log(s, AV_LOG_ERROR, "Unable to add video stream\n");
> +    ret = dvdvideo_video_stream_analyze(s, video_attr, &entry);
> +    if (ret < 0)
>          return ret;
> -    }
>  
> -    return 0;
> +    return dvdvideo_video_stream_add(s, &entry, AVSTREAM_PARSE_HEADERS);
>  }
>  
>  static int dvdvideo_audio_stream_analyze(AVFormatContext *s, audio_attr_t audio_attr,
> @@ -1219,7 +1217,7 @@ static int dvdvideo_audio_stream_add_all(AVFormatContext *s)
>  {
>      DVDVideoDemuxContext *c = s->priv_data;
>  
> -    int ret = 0;
> +    int ret;
>      int nb_streams;
>  
>      if (c->opt_menu)
> @@ -1241,8 +1239,9 @@ static int dvdvideo_audio_stream_add_all(AVFormatContext *s)
>          if (!(c->play_state.pgc->audio_control[i] & 0x8000))
>              continue;
>  
> -        if ((ret = dvdvideo_audio_stream_analyze(s, audio_attr, c->play_state.pgc->audio_control[i],
> -                                                 &entry)) < 0)
> +        ret = dvdvideo_audio_stream_analyze(s, audio_attr, c->play_state.pgc->audio_control[i],
> +                                            &entry);
> +        if (ret < 0)
>              goto break_error;
>  
>          /* IFO structures can declare duplicate entries for the same startcode */
> @@ -1250,7 +1249,8 @@ static int dvdvideo_audio_stream_add_all(AVFormatContext *s)
>              if (s->streams[j]->id == entry.startcode)
>                  continue;
>  
> -        if ((ret = dvdvideo_audio_stream_add(s, &entry, AVSTREAM_PARSE_HEADERS)) < 0)
> +        ret = dvdvideo_audio_stream_add(s, &entry, AVSTREAM_PARSE_HEADERS);
> +        if (ret < 0)
>              goto break_error;
>  
>          continue;
> @@ -1302,7 +1302,8 @@ static int dvdvideo_subp_stream_add(AVFormatContext *s, DVDVideoPGCSubtitleStrea
>      st->codecpar->codec_type = AVMEDIA_TYPE_SUBTITLE;
>      st->codecpar->codec_id = AV_CODEC_ID_DVD_SUBTITLE;
>  
> -    if ((ret = ff_dvdclut_palette_extradata_cat(entry->clut, FF_DVDCLUT_CLUT_SIZE, st->codecpar)) < 0)
> +    ret = ff_dvdclut_palette_extradata_cat(entry->clut, FF_DVDCLUT_CLUT_SIZE, st->codecpar);
> +    if (ret < 0)
>          return ret;
>  
>      if (entry->lang_iso)
> @@ -1326,12 +1327,13 @@ static int dvdvideo_subp_stream_add_internal(AVFormatContext *s, uint32_t offset
>                                               subp_attr_t subp_attr,
>                                               enum DVDVideoSubpictureViewport viewport)
>  {
> -    int ret = 0;
> +    int ret;
>      DVDVideoPGCSubtitleStreamEntry entry = {0};
>  
>      entry.viewport = viewport;
>  
> -    if ((ret = dvdvideo_subp_stream_analyze(s, offset, subp_attr, &entry)) < 0)
> +    ret = dvdvideo_subp_stream_analyze(s, offset, subp_attr, &entry);
> +    if (ret < 0)
>          goto end_error;
>  
>      /* IFO structures can declare duplicate entries for the same startcode */
> @@ -1339,7 +1341,8 @@ static int dvdvideo_subp_stream_add_internal(AVFormatContext *s, uint32_t offset
>          if (s->streams[i]->id == entry.startcode)
>              return 0;
>  
> -    if ((ret = dvdvideo_subp_stream_add(s, &entry, AVSTREAM_PARSE_HEADERS)) < 0)
> +    ret = dvdvideo_subp_stream_add(s, &entry, AVSTREAM_PARSE_HEADERS);
> +    if (ret < 0)
>          goto end_error;
>  
>      return 0;
> @@ -1363,7 +1366,7 @@ static int dvdvideo_subp_stream_add_all(AVFormatContext *s)
>  
>  
>      for (int i = 0; i < nb_streams; i++) {
> -        int ret = 0;
> +        int ret;
>          uint32_t subp_control;
>          subp_attr_t subp_attr;
>          video_attr_t video_attr;
> @@ -1387,29 +1390,35 @@ static int dvdvideo_subp_stream_add_all(AVFormatContext *s)
>  
>          /* 4:3 */
>          if (!video_attr.display_aspect_ratio) {
> -            if ((ret = dvdvideo_subp_stream_add_internal(s, subp_control >> 24, subp_attr,
> -                                                         DVDVIDEO_SUBP_VIEWPORT_FULLSCREEN)) < 0)
> +            ret = dvdvideo_subp_stream_add_internal(s, subp_control >> 24, subp_attr,
> +                                                    DVDVIDEO_SUBP_VIEWPORT_FULLSCREEN);
> +            if (ret < 0)
>                  return ret;
>  
>              continue;
>          }
>  
>          /* 16:9 */
> -        if ((    ret = dvdvideo_subp_stream_add_internal(s, subp_control >> 16, subp_attr,
> -                                                         DVDVIDEO_SUBP_VIEWPORT_WIDESCREEN)) < 0)
> +        ret =     dvdvideo_subp_stream_add_internal(s, subp_control >> 16, subp_attr,
> +                                                    DVDVIDEO_SUBP_VIEWPORT_WIDESCREEN);
> +        if (ret < 0)
>              return ret;
>  
>          /* 16:9 letterbox */
> -        if (video_attr.permitted_df == 2 || video_attr.permitted_df == 0)
> -            if ((ret = dvdvideo_subp_stream_add_internal(s, subp_control >> 8, subp_attr,
> -                                                         DVDVIDEO_SUBP_VIEWPORT_LETTERBOX)) < 0)
> +        if (video_attr.permitted_df == 2 || video_attr.permitted_df == 0) {
> +            ret = dvdvideo_subp_stream_add_internal(s, subp_control >> 8, subp_attr,
> +                                                    DVDVIDEO_SUBP_VIEWPORT_LETTERBOX);
> +            if (ret < 0)
>                  return ret;
> +        }
>  
>          /* 16:9 pan-and-scan */
> -        if (video_attr.permitted_df == 1 || video_attr.permitted_df == 0)
> -            if ((ret = dvdvideo_subp_stream_add_internal(s, subp_control, subp_attr,
> -                                                         DVDVIDEO_SUBP_VIEWPORT_PANSCAN)) < 0)
> +        if (video_attr.permitted_df == 1 || video_attr.permitted_df == 0) {
> +            ret = dvdvideo_subp_stream_add_internal(s, subp_control, subp_attr,
> +                                                    DVDVIDEO_SUBP_VIEWPORT_PANSCAN);
> +            if (ret < 0)
>                  return ret;
> +        }
>      }
>  
>      return 0;
> @@ -1433,7 +1442,7 @@ static int dvdvideo_subdemux_read_data(void *opaque, uint8_t *buf, int buf_size)
>      AVFormatContext *s = opaque;
>      DVDVideoDemuxContext *c = s->priv_data;
>  
> -    int ret = 0;
> +    int ret;
>      int nav_event;
>  
>      if (c->play_end)
> @@ -1471,7 +1480,7 @@ static int dvdvideo_subdemux_open(AVFormatContext *s)
>  {
>      DVDVideoDemuxContext *c = s->priv_data;
>      extern const FFInputFormat ff_mpegps_demuxer;
> -    int ret = 0;
> +    int ret;
>  
>      if (!(c->mpeg_buf = av_mallocz(DVDVIDEO_BLOCK_SIZE)))
>          return AVERROR(ENOMEM);
> @@ -1483,7 +1492,8 @@ static int dvdvideo_subdemux_open(AVFormatContext *s)
>      if (!(c->mpeg_ctx = avformat_alloc_context()))
>          return AVERROR(ENOMEM);
>  
> -    if ((ret = ff_copy_whiteblacklists(c->mpeg_ctx, s)) < 0) {
> +    ret = ff_copy_whiteblacklists(c->mpeg_ctx, s);
> +    if (ret < 0) {
>          avformat_free_context(c->mpeg_ctx);
>          c->mpeg_ctx = NULL;
>  
> @@ -1506,7 +1516,7 @@ static int dvdvideo_read_header(AVFormatContext *s)
>  {
>      DVDVideoDemuxContext *c = s->priv_data;
>  
> -    int ret = 0;
> +    int ret;
>  
>      if (c->opt_menu) {
>          if (c->opt_region               ||
> @@ -1539,12 +1549,25 @@ static int dvdvideo_read_header(AVFormatContext *s)
>              c->opt_pg = 1;
>          }
>  
> -        if ((ret = dvdvideo_ifo_open(s)) < 0                    ||
> -            (ret = dvdvideo_menu_open(s, &c->play_state)) < 0   ||
> -            (ret = dvdvideo_subdemux_open(s)) < 0               ||
> -            (ret = dvdvideo_video_stream_setup(s)) < 0          ||
> -            (ret = dvdvideo_audio_stream_add_all(s)) < 0)
> -        return ret;
> +        ret = dvdvideo_ifo_open(s);
> +        if (ret < 0)
> +            return ret;
> +
> +        ret = dvdvideo_menu_open(s, &c->play_state);
> +        if (ret < 0)
> +            return ret;
> +
> +        ret = dvdvideo_subdemux_open(s);
> +        if (ret < 0)
> +            return ret;
> +
> +        ret = dvdvideo_video_stream_setup(s);
> +        if (ret < 0)
> +            return ret;
> +
> +        ret = dvdvideo_audio_stream_add_all(s);
> +        if (ret < 0)
> +            return ret;
>  
>          return 0;
>      }
> @@ -1568,17 +1591,34 @@ static int dvdvideo_read_header(AVFormatContext *s)
>          }
>      }
>  
> -    if ((ret = dvdvideo_ifo_open(s)) < 0)
> +    ret = dvdvideo_ifo_open(s);
> +    if (ret < 0)
>          return ret;
>  
> -    if (!c->opt_pgc && c->opt_preindex && (ret = dvdvideo_chapters_setup_preindex(s)) < 0)
> +    if (!c->opt_pgc && c->opt_preindex) {
> +        ret = dvdvideo_chapters_setup_preindex(s);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
> +    ret = dvdvideo_play_open(s, &c->play_state);
> +    if (ret < 0)
>          return ret;
>  
> -    if ((ret = dvdvideo_play_open(s, &c->play_state)) < 0   ||
> -        (ret = dvdvideo_subdemux_open(s)) < 0               ||
> -        (ret = dvdvideo_video_stream_setup(s)) < 0          ||
> -        (ret = dvdvideo_audio_stream_add_all(s)) < 0        ||
> -        (ret = dvdvideo_subp_stream_add_all(s)) < 0)
> +    ret = dvdvideo_subdemux_open(s);
> +    if (ret < 0)
> +        return ret;
> +
> +    ret = dvdvideo_video_stream_setup(s);
> +    if (ret < 0)
> +        return ret;
> +
> +    ret = dvdvideo_audio_stream_add_all(s);
> +    if (ret < 0)
> +        return ret;
> +
> +    ret = dvdvideo_subp_stream_add_all(s);
> +    if (ret < 0)
>          return ret;

When I argued against the "if ((ret = ...) < 0)" pattern, I meant single
checks. In chained cases like the above the compactness outweighs the
potential precedence problem.

>  
>      if (!c->opt_pgc && !c->opt_preindex)

On date Wednesday 2024-03-27 12:32:11 +0100, Andreas Rheinhardt wrote:
> Marth64:
> > Recent advice plus my own experience agree that this pattern
> > is error-prone. Instead, set `ret` in its own line and do
> > the error validation after. Also, explicitly return 0 on success
> > in dvdvideo_chapters_setup_preindex()
> > 
> > Signed-off-by: Marth64 <marth64@proxyid.net>
> > ---
> >  libavformat/dvdvideodec.c | 132 +++++++++++++++++++++++++-------------
> >  1 file changed, 86 insertions(+), 46 deletions(-)
[...] 
> When I argued against the "if ((ret = ...) < 0)" pattern, I meant single
> checks. In chained cases like the above the compactness outweighs the
> potential precedence problem.

Holding the patch for now, so it can be reworked to reduce the scope
of this change.

Quoting Stefano Sabatini (2024-03-27 12:27:24)
> it's increasing the line count and therefore decreasing readability

I don't think this holds in general.

Thanks all for the feedback. I think I took the idea too literally here,
and agree in retrospect it’s not necessary at this point. The good news is
while I made this change, I didn’t find any pre-existing bugs in this
demuxer due to using the pattern. (Because while I was writing it, I had
made the mistake a few times and was able to correct them :D)

I think we can skip this and if it becomes an issue down the line I’ll make
the changes surgically. Going forward for my own contributions, I’ll avoid
the pattern unless it truly offers readability gain.

Apologies for the inconvenience.

Best,
Marth64