* [FFmpeg-devel] [PATCH] avformat/dvdvideodec: remove `if ((ret = ...) < 0)` pattern
@ 2024-03-26 20:17 Marth64
2024-03-27 11:27 ` Stefano Sabatini
2024-03-27 11:32 ` Andreas Rheinhardt
0 siblings, 2 replies; 6+ messages in thread
From: Marth64 @ 2024-03-26 20:17 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: 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;
if (!c->opt_pgc && !c->opt_preindex)
--
2.34.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [FFmpeg-devel] [PATCH] avformat/dvdvideodec: remove `if ((ret = ...) < 0)` pattern
2024-03-26 20:17 [FFmpeg-devel] [PATCH] avformat/dvdvideodec: remove `if ((ret = ...) < 0)` pattern Marth64
@ 2024-03-27 11:27 ` Stefano Sabatini
2024-03-27 12:21 ` Anton Khirnov
2024-03-27 11:32 ` Andreas Rheinhardt
1 sibling, 1 reply; 6+ messages in thread
From: Stefano Sabatini @ 2024-03-27 11:27 UTC (permalink / raw)
To: FFmpeg development discussions and patches; +Cc: Marth64
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.
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/dvdvideodec: remove `if ((ret = ...) < 0)` pattern
2024-03-27 11:27 ` Stefano Sabatini
@ 2024-03-27 12:21 ` Anton Khirnov
2024-03-28 15:52 ` Marth64
0 siblings, 1 reply; 6+ messages in thread
From: Anton Khirnov @ 2024-03-27 12:21 UTC (permalink / raw)
To: FFmpeg development discussions and patches, Marth64
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.
--
Anton Khirnov
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/dvdvideodec: remove `if ((ret = ...) < 0)` pattern
2024-03-27 12:21 ` Anton Khirnov
@ 2024-03-28 15:52 ` Marth64
0 siblings, 0 replies; 6+ messages in thread
From: Marth64 @ 2024-03-28 15:52 UTC (permalink / raw)
To: FFmpeg development discussions and patches, Marth64
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] avformat/dvdvideodec: remove `if ((ret = ...) < 0)` pattern
2024-03-26 20:17 [FFmpeg-devel] [PATCH] avformat/dvdvideodec: remove `if ((ret = ...) < 0)` pattern Marth64
2024-03-27 11:27 ` Stefano Sabatini
@ 2024-03-27 11:32 ` Andreas Rheinhardt
2024-03-27 12:08 ` Stefano Sabatini
1 sibling, 1 reply; 6+ messages in thread
From: Andreas Rheinhardt @ 2024-03-27 11:32 UTC (permalink / raw)
To: ffmpeg-devel
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".
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [FFmpeg-devel] [PATCH] avformat/dvdvideodec: remove `if ((ret = ...) < 0)` pattern
2024-03-27 11:32 ` Andreas Rheinhardt
@ 2024-03-27 12:08 ` Stefano Sabatini
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Sabatini @ 2024-03-27 12:08 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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.
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-03-28 15:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-26 20:17 [FFmpeg-devel] [PATCH] avformat/dvdvideodec: remove `if ((ret = ...) < 0)` pattern 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
2024-03-27 12:08 ` Stefano Sabatini
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