From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH] avformat/dvdvideodec: remove `if ((ret = ...) < 0)` pattern
Date: Wed, 27 Mar 2024 12:32:11 +0100
Message-ID: <GV1P250MB07373103D6BCCF08B0A059758F342@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20240326201749.1543553-1-marth64@proxyid.net>
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)
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
next prev parent reply other threads:[~2024-03-27 11:32 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-26 20:17 Marth64
2024-03-27 11:27 ` Stefano Sabatini
2024-03-27 12:21 ` Anton Khirnov
2024-03-28 15:52 ` Marth64
2024-03-27 11:32 ` Andreas Rheinhardt [this message]
2024-03-27 12:08 ` Stefano Sabatini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=GV1P250MB07373103D6BCCF08B0A059758F342@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM \
--to=andreas.rheinhardt@outlook.com \
--cc=ffmpeg-devel@ffmpeg.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git