Message ID | tencent_E4DAAB754E84708C2D539A3A40EE9A959F08@qq.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,v2] fftools/ffmpeg_mux: fix reporting muxer EOF as error | expand |
Context | Check | Description |
---|---|---|
yinshiyou/make_loongarch64 | success | Make finished |
yinshiyou/make_fate_loongarch64 | success | Make fate finished |
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
Quoting Zhao Zhili (2023-04-22 14:56:34) > From: Zhao Zhili <zhilizhao@tencent.com> > > Fix #10327. > > Signed-off-by: Zhao Zhili <zhilizhao@tencent.com> > --- > fftools/ffmpeg_mux.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c > index a2e8873ad2..0e1a5d7dc5 100644 > --- a/fftools/ffmpeg_mux.c > +++ b/fftools/ffmpeg_mux.c > @@ -214,9 +214,15 @@ static void *muxer_thread(void *arg) > ost = of->streams[stream_idx]; > ret = sync_queue_process(mux, ost, ret < 0 ? NULL : pkt, &stream_eof); > av_packet_unref(pkt); > - if (ret == AVERROR_EOF && stream_eof) > - tq_receive_finish(mux->tq, stream_idx); > - else if (ret < 0) { > + if (ret == AVERROR_EOF) { > + if (stream_eof) { > + tq_receive_finish(mux->tq, stream_idx); > + } else { > + av_log(mux, AV_LOG_VERBOSE, "Muxer %s\n", av_err2str(ret)); That seems unnecesarily convoluted, given that we _know_ the error to be EOF here. Also, please make it "Muxer returned EOF" to make it clear what exactly is the source of EOF. Otherwise ok, feel free to push with this change. Thanks.
On Sat, 22 Apr 2023, Anton Khirnov wrote: > Quoting Zhao Zhili (2023-04-22 14:56:34) >> From: Zhao Zhili <zhilizhao@tencent.com> >> >> Fix #10327. >> >> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com> >> --- >> fftools/ffmpeg_mux.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c >> index a2e8873ad2..0e1a5d7dc5 100644 >> --- a/fftools/ffmpeg_mux.c >> +++ b/fftools/ffmpeg_mux.c >> @@ -214,9 +214,15 @@ static void *muxer_thread(void *arg) >> ost = of->streams[stream_idx]; >> ret = sync_queue_process(mux, ost, ret < 0 ? NULL : pkt, &stream_eof); >> av_packet_unref(pkt); >> - if (ret == AVERROR_EOF && stream_eof) >> - tq_receive_finish(mux->tq, stream_idx); >> - else if (ret < 0) { >> + if (ret == AVERROR_EOF) { >> + if (stream_eof) { >> + tq_receive_finish(mux->tq, stream_idx); >> + } else { >> + av_log(mux, AV_LOG_VERBOSE, "Muxer %s\n", av_err2str(ret)); > > That seems unnecesarily convoluted, given that we _know_ the error to be > EOF here. Also, please make it "Muxer returned EOF" to make it clear > what exactly is the source of EOF. > > Otherwise ok, feel free to push with this change. This seems like yet another clash of AVERROR_EOF error codes coming from different places with different semantics. For av_interleaved_write_frame(), AVERROR_EOF is an error condition, so file encoding should fail, but after this patch it will not fail, just simply end, because you assume it is legitimately coming from the filesize check... This should be fixed somehow, e.g. use a special error code for the filesize check and only handle that as a legitimate EOF. Regards, Marton
Quoting Marton Balint (2023-04-23 11:12:38) > > > On Sat, 22 Apr 2023, Anton Khirnov wrote: > > > Quoting Zhao Zhili (2023-04-22 14:56:34) > >> From: Zhao Zhili <zhilizhao@tencent.com> > >> > >> Fix #10327. > >> > >> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com> > >> --- > >> fftools/ffmpeg_mux.c | 12 +++++++++--- > >> 1 file changed, 9 insertions(+), 3 deletions(-) > >> > >> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c > >> index a2e8873ad2..0e1a5d7dc5 100644 > >> --- a/fftools/ffmpeg_mux.c > >> +++ b/fftools/ffmpeg_mux.c > >> @@ -214,9 +214,15 @@ static void *muxer_thread(void *arg) > >> ost = of->streams[stream_idx]; > >> ret = sync_queue_process(mux, ost, ret < 0 ? NULL : pkt, &stream_eof); > >> av_packet_unref(pkt); > >> - if (ret == AVERROR_EOF && stream_eof) > >> - tq_receive_finish(mux->tq, stream_idx); > >> - else if (ret < 0) { > >> + if (ret == AVERROR_EOF) { > >> + if (stream_eof) { > >> + tq_receive_finish(mux->tq, stream_idx); > >> + } else { > >> + av_log(mux, AV_LOG_VERBOSE, "Muxer %s\n", av_err2str(ret)); > > > > That seems unnecesarily convoluted, given that we _know_ the error to be > > EOF here. Also, please make it "Muxer returned EOF" to make it clear > > what exactly is the source of EOF. > > > > Otherwise ok, feel free to push with this change. > > This seems like yet another clash of AVERROR_EOF error codes coming from > different places with different semantics. For > av_interleaved_write_frame(), AVERROR_EOF is an error condition, so > file encoding should fail, Why should it fail? I'd think a muxer returning EOF is the way to signal non-error muxer-side termination.
On Sun, 23 Apr 2023, Anton Khirnov wrote: > Quoting Marton Balint (2023-04-23 11:12:38) >> >> >> On Sat, 22 Apr 2023, Anton Khirnov wrote: >> >>> Quoting Zhao Zhili (2023-04-22 14:56:34) >>>> From: Zhao Zhili <zhilizhao@tencent.com> >>>> >>>> Fix #10327. >>>> >>>> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com> >>>> --- >>>> fftools/ffmpeg_mux.c | 12 +++++++++--- >>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c >>>> index a2e8873ad2..0e1a5d7dc5 100644 >>>> --- a/fftools/ffmpeg_mux.c >>>> +++ b/fftools/ffmpeg_mux.c >>>> @@ -214,9 +214,15 @@ static void *muxer_thread(void *arg) >>>> ost = of->streams[stream_idx]; >>>> ret = sync_queue_process(mux, ost, ret < 0 ? NULL : pkt, &stream_eof); >>>> av_packet_unref(pkt); >>>> - if (ret == AVERROR_EOF && stream_eof) >>>> - tq_receive_finish(mux->tq, stream_idx); >>>> - else if (ret < 0) { >>>> + if (ret == AVERROR_EOF) { >>>> + if (stream_eof) { >>>> + tq_receive_finish(mux->tq, stream_idx); >>>> + } else { >>>> + av_log(mux, AV_LOG_VERBOSE, "Muxer %s\n", av_err2str(ret)); >>> >>> That seems unnecesarily convoluted, given that we _know_ the error to be >>> EOF here. Also, please make it "Muxer returned EOF" to make it clear >>> what exactly is the source of EOF. >>> >>> Otherwise ok, feel free to push with this change. >> >> This seems like yet another clash of AVERROR_EOF error codes coming from >> different places with different semantics. For >> av_interleaved_write_frame(), AVERROR_EOF is an error condition, so >> file encoding should fail, > > Why should it fail? I'd think a muxer returning EOF is the way to signal > non-error muxer-side termination. That would be an API change. AVERROR_EOF is not special in any way from other error codes for av_interleaved_write_frame. A muxer cannot signal non-error muxer side termination with existing API. Regards, Marton
> On Apr 23, 2023, at 17:42, Marton Balint <cus@passwd.hu> wrote: > > On Sun, 23 Apr 2023, Anton Khirnov wrote: > >> Quoting Marton Balint (2023-04-23 11:12:38) >>> >>> >>> On Sat, 22 Apr 2023, Anton Khirnov wrote: >>> >>>> Quoting Zhao Zhili (2023-04-22 14:56:34) >>>>> From: Zhao Zhili <zhilizhao@tencent.com> >>>>> >>>>> Fix #10327. >>>>> >>>>> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com> >>>>> --- >>>>> fftools/ffmpeg_mux.c | 12 +++++++++--- >>>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c >>>>> index a2e8873ad2..0e1a5d7dc5 100644 >>>>> --- a/fftools/ffmpeg_mux.c >>>>> +++ b/fftools/ffmpeg_mux.c >>>>> @@ -214,9 +214,15 @@ static void *muxer_thread(void *arg) >>>>> ost = of->streams[stream_idx]; >>>>> ret = sync_queue_process(mux, ost, ret < 0 ? NULL : pkt, &stream_eof); >>>>> av_packet_unref(pkt); >>>>> - if (ret == AVERROR_EOF && stream_eof) >>>>> - tq_receive_finish(mux->tq, stream_idx); >>>>> - else if (ret < 0) { >>>>> + if (ret == AVERROR_EOF) { >>>>> + if (stream_eof) { >>>>> + tq_receive_finish(mux->tq, stream_idx); >>>>> + } else { >>>>> + av_log(mux, AV_LOG_VERBOSE, "Muxer %s\n", av_err2str(ret)); >>>> >>>> That seems unnecesarily convoluted, given that we _know_ the error to be >>>> EOF here. Also, please make it "Muxer returned EOF" to make it clear >>>> what exactly is the source of EOF. >>>> >>>> Otherwise ok, feel free to push with this change. >>> >>> This seems like yet another clash of AVERROR_EOF error codes coming from >>> different places with different semantics. For >>> av_interleaved_write_frame(), AVERROR_EOF is an error condition, so >>> file encoding should fail, >> >> Why should it fail? I'd think a muxer returning EOF is the way to signal >> non-error muxer-side termination. > > That would be an API change. AVERROR_EOF is not special in any way from other error codes for av_interleaved_write_frame. A muxer cannot signal non-error muxer side termination with existing API. Sorry I have pushed before seeing your comments. It can happen in theory, but I’m not sure in which case muxer can return EOF and should be treated as a real error condition. A simple grep shows: $ grep 'AVERROR_EOF' libavformat/*enc.c libavformat/hlsenc.c: if (ret >= 0 || ret == AVERROR_EOF) <--- It changed ret to 0 afterwards. libavformat/webmdashenc.c: return AVERROR_EOF; <—-- More like a bug on the caller side than a real error. > > Regards, > Marton > _______________________________________________ > 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".
Quoting Marton Balint (2023-04-23 11:42:48) > On Sun, 23 Apr 2023, Anton Khirnov wrote: > > Quoting Marton Balint (2023-04-23 11:12:38) > >> This seems like yet another clash of AVERROR_EOF error codes coming from > >> different places with different semantics. For > >> av_interleaved_write_frame(), AVERROR_EOF is an error condition, so > >> file encoding should fail, > > > > Why should it fail? I'd think a muxer returning EOF is the way to signal > > non-error muxer-side termination. > > That would be an API change. AVERROR_EOF is not special in any way from > other error codes for av_interleaved_write_frame. A muxer cannot signal > non-error muxer side termination with existing API. All error codes (should) have a specific meaning. I cannot think of a good reason for a muxer to return AVERROR_EOF to signal an error. Can you?
On Sun, 23 Apr 2023, Anton Khirnov wrote: > Quoting Marton Balint (2023-04-23 11:42:48) >> On Sun, 23 Apr 2023, Anton Khirnov wrote: >>> Quoting Marton Balint (2023-04-23 11:12:38) >>>> This seems like yet another clash of AVERROR_EOF error codes coming from >>>> different places with different semantics. For >>>> av_interleaved_write_frame(), AVERROR_EOF is an error condition, so >>>> file encoding should fail, >>> >>> Why should it fail? I'd think a muxer returning EOF is the way to signal >>> non-error muxer-side termination. >> >> That would be an API change. AVERROR_EOF is not special in any way from >> other error codes for av_interleaved_write_frame. A muxer cannot signal >> non-error muxer side termination with existing API. > > All error codes (should) have a specific meaning. I cannot think of a > good reason for a muxer to return AVERROR_EOF to signal an error. > Can you? Previously, we expeced users to treat any negative error code as error for av_interleaved_write_frame(). This is what is documented. ffmpeg.c followed this approach. Don't you see the slightest problem if we suddenly change this? Here is a real case of av_interlaved_write_frame() returning EOF: https://ffmpeg.org/pipermail/ffmpeg-devel/2023-February/306247.html Regards, Marton
Marton Balint (12023-04-23):
> https://ffmpeg.org/pipermail/ffmpeg-devel/2023-February/306247.html
Looks like something that sould return AVERORR(EPIPE).
Regards,
Quoting Marton Balint (2023-04-23 12:05:51) > > > On Sun, 23 Apr 2023, Anton Khirnov wrote: > > > Quoting Marton Balint (2023-04-23 11:42:48) > >> On Sun, 23 Apr 2023, Anton Khirnov wrote: > >>> Quoting Marton Balint (2023-04-23 11:12:38) > >>>> This seems like yet another clash of AVERROR_EOF error codes coming from > >>>> different places with different semantics. For > >>>> av_interleaved_write_frame(), AVERROR_EOF is an error condition, so > >>>> file encoding should fail, > >>> > >>> Why should it fail? I'd think a muxer returning EOF is the way to signal > >>> non-error muxer-side termination. > >> > >> That would be an API change. AVERROR_EOF is not special in any way from > >> other error codes for av_interleaved_write_frame. A muxer cannot signal > >> non-error muxer side termination with existing API. > > > > All error codes (should) have a specific meaning. I cannot think of a > > good reason for a muxer to return AVERROR_EOF to signal an error. > > Can you? > > Previously, we expeced users to treat any negative error code as error for > av_interleaved_write_frame(). I don't think we expect the users to do anything in particular in responce to av_interleaved_write_frame() return codes. The doxy says that it returns a negative error code on error, but the caller can freely decide what to do with that information - this includes ignoring it. > This is what is documented. ffmpeg.c followed this approach. Don't you > see the slightest problem if we suddenly change this? Seems to me you're mixing ffmpeg CLI and lavf behavior. My claim is entirely from the point of view of the CLI, and is this: if the muxer returns AVERROR_EOF, then it should be treated as normal termination. This is similar to how other components behave - e.g. a (bitstream) filter can at any time decide to return EOF to its downstream, terminating a stream even though more input is available. You could argue that muxers should never return AVERROR_EOF and any muxer that currently does so is buggy, but that is still compatible with the above.
On Sun, 23 Apr 2023, Anton Khirnov wrote: > Quoting Marton Balint (2023-04-23 12:05:51) >> >> >> On Sun, 23 Apr 2023, Anton Khirnov wrote: >> >>> Quoting Marton Balint (2023-04-23 11:42:48) >>>> On Sun, 23 Apr 2023, Anton Khirnov wrote: >>>>> Quoting Marton Balint (2023-04-23 11:12:38) >>>>>> This seems like yet another clash of AVERROR_EOF error codes coming from >>>>>> different places with different semantics. For >>>>>> av_interleaved_write_frame(), AVERROR_EOF is an error condition, so >>>>>> file encoding should fail, >>>>> >>>>> Why should it fail? I'd think a muxer returning EOF is the way to signal >>>>> non-error muxer-side termination. >>>> >>>> That would be an API change. AVERROR_EOF is not special in any way from >>>> other error codes for av_interleaved_write_frame. A muxer cannot signal >>>> non-error muxer side termination with existing API. >>> >>> All error codes (should) have a specific meaning. I cannot think of a >>> good reason for a muxer to return AVERROR_EOF to signal an error. >>> Can you? >> >> Previously, we expeced users to treat any negative error code as error for >> av_interleaved_write_frame(). > > I don't think we expect the users to do anything in particular in > responce to av_interleaved_write_frame() return codes. The doxy says > that it returns a negative error code on error, but the caller can > freely decide what to do with that information - this includes ignoring > it. I don't understand. A good program propagates back error conditions to the user, and not hides them silently. > >> This is what is documented. ffmpeg.c followed this approach. Don't you >> see the slightest problem if we suddenly change this? > > Seems to me you're mixing ffmpeg CLI and lavf behavior. My claim is > entirely from the point of view of the CLI, and is this: if the muxer > returns AVERROR_EOF, then it should be treated as normal termination. I disagree. If ffmpeg.c ignores a specific muxer error code for whatever reason, that is a bug. At least it should fail if -xerror is given. > This is similar to how other components behave - e.g. a (bitstream) > filter can at any time decide to return EOF to its downstream, > terminating a stream even though more input is available. And for bitstream filters it is properly documented, and callers were always expected to act accordingly. Regards, Marton
Quoting Marton Balint (2023-04-23 20:15:13) > > > On Sun, 23 Apr 2023, Anton Khirnov wrote: > > > Quoting Marton Balint (2023-04-23 12:05:51) > >> > >> > >> On Sun, 23 Apr 2023, Anton Khirnov wrote: > >> > >>> Quoting Marton Balint (2023-04-23 11:42:48) > >>>> On Sun, 23 Apr 2023, Anton Khirnov wrote: > >>>>> Quoting Marton Balint (2023-04-23 11:12:38) > >>>>>> This seems like yet another clash of AVERROR_EOF error codes coming from > >>>>>> different places with different semantics. For > >>>>>> av_interleaved_write_frame(), AVERROR_EOF is an error condition, so > >>>>>> file encoding should fail, > >>>>> > >>>>> Why should it fail? I'd think a muxer returning EOF is the way to signal > >>>>> non-error muxer-side termination. > >>>> > >>>> That would be an API change. AVERROR_EOF is not special in any way from > >>>> other error codes for av_interleaved_write_frame. A muxer cannot signal > >>>> non-error muxer side termination with existing API. > >>> > >>> All error codes (should) have a specific meaning. I cannot think of a > >>> good reason for a muxer to return AVERROR_EOF to signal an error. > >>> Can you? > >> > >> Previously, we expeced users to treat any negative error code as error for > >> av_interleaved_write_frame(). > > > > I don't think we expect the users to do anything in particular in > > responce to av_interleaved_write_frame() return codes. The doxy says > > that it returns a negative error code on error, but the caller can > > freely decide what to do with that information - this includes ignoring > > it. > > I don't understand. A good program propagates back error conditions to the > user, and not hides them silently. I do not think blanket claims such as this are a good idea. What is or is not considered "an error condition" depends on the context. As I said before - I don't see why a muxer should ever return AVERROR_EOF to signal a legitimate muxing error.
On Sun, 23 Apr 2023, Anton Khirnov wrote: > Quoting Marton Balint (2023-04-23 20:15:13) >> >> >> On Sun, 23 Apr 2023, Anton Khirnov wrote: >> >>> Quoting Marton Balint (2023-04-23 12:05:51) >>>> >>>> >>>> On Sun, 23 Apr 2023, Anton Khirnov wrote: >>>> >>>>> Quoting Marton Balint (2023-04-23 11:42:48) >>>>>> On Sun, 23 Apr 2023, Anton Khirnov wrote: >>>>>>> Quoting Marton Balint (2023-04-23 11:12:38) >>>>>>>> This seems like yet another clash of AVERROR_EOF error codes coming from >>>>>>>> different places with different semantics. For >>>>>>>> av_interleaved_write_frame(), AVERROR_EOF is an error condition, so >>>>>>>> file encoding should fail, >>>>>>> >>>>>>> Why should it fail? I'd think a muxer returning EOF is the way to signal >>>>>>> non-error muxer-side termination. >>>>>> >>>>>> That would be an API change. AVERROR_EOF is not special in any way from >>>>>> other error codes for av_interleaved_write_frame. A muxer cannot signal >>>>>> non-error muxer side termination with existing API. >>>>> >>>>> All error codes (should) have a specific meaning. I cannot think of a >>>>> good reason for a muxer to return AVERROR_EOF to signal an error. >>>>> Can you? >>>> >>>> Previously, we expeced users to treat any negative error code as error for >>>> av_interleaved_write_frame(). >>> >>> I don't think we expect the users to do anything in particular in >>> responce to av_interleaved_write_frame() return codes. The doxy says >>> that it returns a negative error code on error, but the caller can >>> freely decide what to do with that information - this includes ignoring >>> it. >> >> I don't understand. A good program propagates back error conditions to the >> user, and not hides them silently. > > I do not think blanket claims such as this are a good idea. What is or > is not considered "an error condition" depends on the context. > > As I said before - I don't see why a muxer should ever return > AVERROR_EOF to signal a legitimate muxing error. The real risk is that they unintentionally do that, when the error code is coming from some underlying operation for example. So previsouly a muxer could return any error code to signal error condition and reasonably assume that ffmpeg.c will report it back to the user as an error. The change in ffmpeg.c "forces" muxers to check if they ever get AVERROR_EOF for some real error condition and map them to e.g. AVERROR(EIO). And that is an API change. Regards, Marton
Marton Balint (12023-04-24): > The change in ffmpeg.c "forces" muxers to check if they ever get AVERROR_EOF > for some real error condition and map them to e.g. AVERROR(EIO). And that is > an API change. Indeed. And the documentation agrees: * @return < 0 on error, = 0 if OK, 1 if flushed and there is no more data to flush * * @see av_interleaved_write_frame() */ int av_write_frame(AVFormatContext *s, AVPacket *pkt); * @return 0 on success, a negative AVERROR on error. * * @see av_write_frame(), AVFormatContext.max_interleave_delta */ int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt); No mention of EOF at all. Contrast with: * @return 0 if OK, < 0 on error or end of file. On error, pkt will be blank * (as if it came from av_packet_alloc()). … int av_read_frame(AVFormatContext *s, AVPacket *pkt); When EOF is intended, it written in the documentation. As it is, we should av_assert0(ret != AVERROR_EOF) in av_interleaved_write_frame() and fix the possible failures. Regards,
Quoting Marton Balint (2023-04-24 11:09:44) > The real risk is that they unintentionally do that, when the error code is > coming from some underlying operation for example. > > So previsouly a muxer could return any error code to signal error > condition and reasonably assume that ffmpeg.c will report it back to the > user as an error. > > The change in ffmpeg.c "forces" muxers to check if they ever get > AVERROR_EOF for some real error condition and map them to > e.g. AVERROR(EIO). And that is an API change. I don't follow, how is fixing bugs in muxers in any way an API change?
Quoting Marton Balint (2023-04-24 11:09:44) > > > On Sun, 23 Apr 2023, Anton Khirnov wrote: > > > Quoting Marton Balint (2023-04-23 20:15:13) > >> > >> > >> On Sun, 23 Apr 2023, Anton Khirnov wrote: > >> I don't understand. A good program propagates back error conditions to the > >> user, and not hides them silently. > > > > I do not think blanket claims such as this are a good idea. What is or > > is not considered "an error condition" depends on the context. > > > > As I said before - I don't see why a muxer should ever return > > AVERROR_EOF to signal a legitimate muxing error. > > The real risk is that they unintentionally do that, when the error code is > coming from some underlying operation for example. > > So previsouly a muxer could return any error code to signal error > condition and reasonably assume that ffmpeg.c will report it back to the > user as an error. Also, muxers absolutely must not make any assumptions about ffmpeg CLI behavior, because other callers exist and they must all be treated equally.
On Mon, 24 Apr 2023, Anton Khirnov wrote: > Quoting Marton Balint (2023-04-24 11:09:44) >> The real risk is that they unintentionally do that, when the error code is >> coming from some underlying operation for example. >> >> So previsouly a muxer could return any error code to signal error >> condition and reasonably assume that ffmpeg.c will report it back to the >> user as an error. >> >> The change in ffmpeg.c "forces" muxers to check if they ever get >> AVERROR_EOF for some real error condition and map them to >> e.g. AVERROR(EIO). And that is an API change. > > I don't follow, how is fixing bugs in muxers in any way an API change? The API change is that muxers are no longer allowed to return AVERROR_EOF for an error condition. Regards, Marton
Quoting Marton Balint (2023-04-24 20:41:55) > > > On Mon, 24 Apr 2023, Anton Khirnov wrote: > > > Quoting Marton Balint (2023-04-24 11:09:44) > >> The real risk is that they unintentionally do that, when the error code is > >> coming from some underlying operation for example. > >> > >> So previsouly a muxer could return any error code to signal error > >> condition and reasonably assume that ffmpeg.c will report it back to the > >> user as an error. > >> > >> The change in ffmpeg.c "forces" muxers to check if they ever get > >> AVERROR_EOF for some real error condition and map them to > >> e.g. AVERROR(EIO). And that is an API change. > > > > I don't follow, how is fixing bugs in muxers in any way an API change? > > The API change is that muxers are no longer allowed to return AVERROR_EOF > for an error condition. I still don't follow - what is the API that is being changed? Besides, I don't think that was ever a valid thing to do anyway.
On Mon, 24 Apr 2023, Anton Khirnov wrote: > Quoting Marton Balint (2023-04-24 20:41:55) >> >> >> On Mon, 24 Apr 2023, Anton Khirnov wrote: >> >>> Quoting Marton Balint (2023-04-24 11:09:44) >>>> The real risk is that they unintentionally do that, when the error code is >>>> coming from some underlying operation for example. >>>> >>>> So previsouly a muxer could return any error code to signal error >>>> condition and reasonably assume that ffmpeg.c will report it back to the >>>> user as an error. >>>> >>>> The change in ffmpeg.c "forces" muxers to check if they ever get >>>> AVERROR_EOF for some real error condition and map them to >>>> e.g. AVERROR(EIO). And that is an API change. >>> >>> I don't follow, how is fixing bugs in muxers in any way an API change? >> >> The API change is that muxers are no longer allowed to return AVERROR_EOF >> for an error condition. > > I still don't follow - what is the API that is being changed? av_interleaved_write_frame(). Previously any negative return value was an error condition. This change assumes that AVERROR_EOF return value is a non-error condition. > > Besides, I don't think that was ever a valid thing to do anyway. The error codes a muxer can use is not explicitly documented, so one could reasonably assume that any negative error code is valid. And ffmpeg.c worked that way for a long time, doc/examples/mux.c still works that way. Regards, Marton
Marton Balint (12023-04-24): > The API change is that muxers are no longer allowed to return AVERROR_EOF > for an error condition. There is no API change because applications are not allowed to write muxers. At worst, it would be an internal API change. But it is not even an internal API change, because AVERROR_EOF was not allowed as a return code from a muxer: muxers return error codes, and AVERROR_EOF, despite being negative, is not an error. But in the end, you are right that the reasons given here make this patch wrong. Regards,
Quoting Marton Balint (2023-04-24 21:42:26) > > > On Mon, 24 Apr 2023, Anton Khirnov wrote: > > > Quoting Marton Balint (2023-04-24 20:41:55) > >> > >> > >> On Mon, 24 Apr 2023, Anton Khirnov wrote: > >> > >>> Quoting Marton Balint (2023-04-24 11:09:44) > >>>> The real risk is that they unintentionally do that, when the error code is > >>>> coming from some underlying operation for example. > >>>> > >>>> So previsouly a muxer could return any error code to signal error > >>>> condition and reasonably assume that ffmpeg.c will report it back to the > >>>> user as an error. > >>>> > >>>> The change in ffmpeg.c "forces" muxers to check if they ever get > >>>> AVERROR_EOF for some real error condition and map them to > >>>> e.g. AVERROR(EIO). And that is an API change. > >>> > >>> I don't follow, how is fixing bugs in muxers in any way an API change? > >> > >> The API change is that muxers are no longer allowed to return AVERROR_EOF > >> for an error condition. > > > > I still don't follow - what is the API that is being changed? > > av_interleaved_write_frame(). Previously any negative return value was an > error condition. This change assumes that AVERROR_EOF return value is a > non-error condition. I think the point on which we disagree is your notion of "error conditions" as being basically interchangeable. In my opinion error codes are semantically different and returning the wrong one in this case(*) is just as much of a bug as returning success. Every error code returned by a muxer must have a meaningful interpretation, even if that specific code is not explicitly documented for use with muxers. I do not see any meaningful interpetation for av_interleaved_write_frame() returning AVERROR_EOF other than "the muxer, for whatever reason, has decided to terminate muxing and is using this code to inform the caller of said fact". So now either * this is exactly what happened, then not screaming at the CLI user is the correct thing to do * this is not what happened, then the muxer should be fixed to return an actually meaningful error code In both of these cases the CLI code as it is now is correct. A similar situation exists for demuxing, where we also avoid printing an error message for AVERROR_EOF. > > Besides, I don't think that was ever a valid thing to do anyway. > > The error codes a muxer can use is not explicitly documented, so one > could reasonably assume that any negative error code is valid. I'd say one could reasonably assume the muxer will return meaningful error codes and interpret them according to their meaning. > And ffmpeg.c worked that way for a long time, doc/examples/mux.c still > works that way. So what? The caller is free to decide what to do with an error code, the muxing API does not care if a message is printed or not. (*) I'm saying "in this case" because in some other APIs success vs failure has defined observable effects, like "data allocated or not", "ownership transferred or not", etc. Then making a strict distinction between success and failure makes more sense. This is not the case for av_interleaved_write_frame() - there is no specified observable difference between success and failure other than the code itself.
Anton Khirnov (12023-04-24): > I think the point on which we disagree is your notion of "error > conditions" as being basically interchangeable. The way everything, FFmpeg and all other sane system work, is that the caller handles the very few errors it know how to handle (EAGAIN certainly, maybe a few other depending on the situation), and everything else is passed to the user in human-readable form, in the hope the user can deal with it. So in a sence, most errors are basically interchangeable for the application, and it is how it is supposed to be. For example EISDIR, ENOENT, ENOTDIR are almost always interchangeable for the application, and really mean to the user “you mistyped the file name somehow”. > In both of these cases the CLI code as it is now is correct. Absolutely not. The documentation states that muxers return errors and nothing else, there is no special case for EOF, unlike demuxers, and therefore this patch is a waste of code.
diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c index a2e8873ad2..0e1a5d7dc5 100644 --- a/fftools/ffmpeg_mux.c +++ b/fftools/ffmpeg_mux.c @@ -214,9 +214,15 @@ static void *muxer_thread(void *arg) ost = of->streams[stream_idx]; ret = sync_queue_process(mux, ost, ret < 0 ? NULL : pkt, &stream_eof); av_packet_unref(pkt); - if (ret == AVERROR_EOF && stream_eof) - tq_receive_finish(mux->tq, stream_idx); - else if (ret < 0) { + if (ret == AVERROR_EOF) { + if (stream_eof) { + tq_receive_finish(mux->tq, stream_idx); + } else { + av_log(mux, AV_LOG_VERBOSE, "Muxer %s\n", av_err2str(ret)); + ret = 0; + break; + } + } else if (ret < 0) { av_log(mux, AV_LOG_ERROR, "Error muxing a packet\n"); break; }