[FFmpeg-devel,v2] fftools/ffmpeg_mux: fix reporting muxer EOF as error

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(-)

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.