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

[Anton Khirnov]

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