| Message ID | 201702121648.57782.cehoyos@ag.or.at |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, 12 Feb 2017 16:48:57 +0100 Carl Eugen Hoyos <cehoyos@ag.or.at> wrote: > Hi! > > Attached patch written yesterday silences warnings when compiling > ffmpeg.c, don't know how useful it is, only fate-tested. > > Please comment, Carl Eugen Conflicts with my merge patches.
Le quartidi 24 pluviôse, an CCXXV, Carl Eugen Hoyos a écrit : > Hi! > > Attached patch written yesterday silences warnings when compiling > ffmpeg.c, don't know how useful it is, only fate-tested. > > Please comment, Carl Eugen > From 525aff909aec50d4e1a49f289ff9069300a4b51f Mon Sep 17 00:00:00 2001 > From: Carl Eugen Hoyos <cehoyos@ag.or.at> > Date: Sun, 12 Feb 2017 16:41:21 +0100 > Subject: [PATCH] ffmpeg: Check the return value of two functions declared > "warn_unused_result". > > --- > ffmpeg.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/ffmpeg.c b/ffmpeg.c > index 06570c0..9952da6 100644 > --- a/ffmpeg.c > +++ b/ffmpeg.c > @@ -215,14 +215,18 @@ static void sub2video_copy_rect(uint8_t *dst, int dst_linesize, int w, int h, > static void sub2video_push_ref(InputStream *ist, int64_t pts) > { > AVFrame *frame = ist->sub2video.frame; > - int i; > + int i, ret; > > av_assert1(frame->data[0]); > ist->sub2video.last_pts = frame->pts = pts; > - for (i = 0; i < ist->nb_filters; i++) > - av_buffersrc_add_frame_flags(ist->filters[i]->filter, frame, > + for (i = 0; i < ist->nb_filters; i++) { > + ret = av_buffersrc_add_frame_flags(ist->filters[i]->filter, frame, > AV_BUFFERSRC_FLAG_KEEP_REF | > AV_BUFFERSRC_FLAG_PUSH); When the changes are that small, I think it is better to fix the indentation at the same time rather than leave it like that or fix it in a separate commit. > + if (ret < 0) > + av_log(ist->filters[i]->filter, AV_LOG_ERROR, > + "Failed to add a frame to the buffer source.\n"); Please mention it is a "subtitle video frame" and include the error message itself (av_err2str(ret)). > + } > } > > static void sub2video_update(InputStream *ist, AVSubtitle *sub) > @@ -290,12 +294,16 @@ static void sub2video_heartbeat(InputStream *ist, int64_t pts) > > static void sub2video_flush(InputStream *ist) > { > - int i; > + int i, ret; > > if (ist->sub2video.end_pts < INT64_MAX) > sub2video_update(ist, NULL); > - for (i = 0; i < ist->nb_filters; i++) > - av_buffersrc_add_frame(ist->filters[i]->filter, NULL); > + for (i = 0; i < ist->nb_filters; i++) { > + ret = av_buffersrc_add_frame(ist->filters[i]->filter, NULL); > + if (ret < 0) > + av_log(ist->filters[i]->filter, AV_LOG_ERROR, > + "Failed to add a frame to the buffer source.\n"); Same as above of course. > + } > } > > /* end of sub2video hack */ Apart from that, the patch looks good to me in principle. Regards,
On Sun, 12 Feb 2017, Carl Eugen Hoyos wrote: > Hi! > > Attached patch written yesterday silences warnings when compiling > ffmpeg.c, don't know how useful it is, only fate-tested. Writing a warning about the failed filter functions is good, but ffmpeg has a -xerror option as well, so I guess the proper fix would be to propagate the error through the functions and fail the encoding process as well if -xerror is set. Regards, Marton
Le quartidi 24 pluviôse, an CCXXV, Marton Balint a écrit : > Writing a warning about the failed filter functions is good, but ffmpeg has > a -xerror option as well, so I guess the proper fix would be to propagate > the error through the functions and fail the encoding process as well if > -xerror is set. I hesitated to mention it in my review. Ideally, taking it into account would be nice. But... First, our code is not completely consistent in its respect for -xerror, especially when it comes to filtering, unless I am mistaken. A re-read of the various code paths would probably be necessary to make all that consistent. Second, the patch fixes an issue (annoying warnings) even without that. So my opinion is that the patch can go in even without the -xerror implementation. A comment /* TODO -xerror */ would do no harm, though. And I see the implementation of -xerror elsewhere is really small; but a bit of factoring would make it more elegant and consistent. Regards,
2017-02-12 17:12 GMT+01:00 Marton Balint <cus@passwd.hu>: > > On Sun, 12 Feb 2017, Carl Eugen Hoyos wrote: > >> Attached patch written yesterday silences warnings when compiling >> ffmpeg.c, don't know how useful it is, only fate-tested. > > Writing a warning about the failed filter functions is good, but ffmpeg has > a -xerror option as well, so I guess the proper fix would be to propagate > the error through the functions and fail the encoding process as well if > -xerror is set. I don't necessarily disagree but will not work on this. Sorry, Carl Eugen
On Sun, 12 Feb 2017 16:52:21 +0100 wm4 <nfxjfg@googlemail.com> wrote: > On Sun, 12 Feb 2017 16:48:57 +0100 > Carl Eugen Hoyos <cehoyos@ag.or.at> wrote: > > > Hi! > > > > Attached patch written yesterday silences warnings when compiling > > ffmpeg.c, don't know how useful it is, only fate-tested. > > > > Please comment, Carl Eugen > > Conflicts with my merge patches. Actually it doesn't - I was thinking of something else. Sorry.
diff --git a/ffmpeg.c b/ffmpeg.c index 06570c0..9952da6 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -215,14 +215,18 @@ static void sub2video_copy_rect(uint8_t *dst, int dst_linesize, int w, int h, static void sub2video_push_ref(InputStream *ist, int64_t pts) { AVFrame *frame = ist->sub2video.frame; - int i; + int i, ret; av_assert1(frame->data[0]); ist->sub2video.last_pts = frame->pts = pts; - for (i = 0; i < ist->nb_filters; i++) - av_buffersrc_add_frame_flags(ist->filters[i]->filter, frame, + for (i = 0; i < ist->nb_filters; i++) { + ret = av_buffersrc_add_frame_flags(ist->filters[i]->filter, frame, AV_BUFFERSRC_FLAG_KEEP_REF | AV_BUFFERSRC_FLAG_PUSH); + if (ret < 0) + av_log(ist->filters[i]->filter, AV_LOG_ERROR, + "Failed to add a frame to the buffer source.\n"); + } } static void sub2video_update(InputStream *ist, AVSubtitle *sub) @@ -290,12 +294,16 @@ static void sub2video_heartbeat(InputStream *ist, int64_t pts) static void sub2video_flush(InputStream *ist) { - int i; + int i, ret; if (ist->sub2video.end_pts < INT64_MAX) sub2video_update(ist, NULL); - for (i = 0; i < ist->nb_filters; i++) - av_buffersrc_add_frame(ist->filters[i]->filter, NULL); + for (i = 0; i < ist->nb_filters; i++) { + ret = av_buffersrc_add_frame(ist->filters[i]->filter, NULL); + if (ret < 0) + av_log(ist->filters[i]->filter, AV_LOG_ERROR, + "Failed to add a frame to the buffer source.\n"); + } } /* end of sub2video hack */
Hi! Attached patch written yesterday silences warnings when compiling ffmpeg.c, don't know how useful it is, only fate-tested. Please comment, Carl Eugen From 525aff909aec50d4e1a49f289ff9069300a4b51f Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <cehoyos@ag.or.at> Date: Sun, 12 Feb 2017 16:41:21 +0100 Subject: [PATCH] ffmpeg: Check the return value of two functions declared "warn_unused_result". --- ffmpeg.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)