[FFmpeg-devel] [PATCH 1/8] lavc/avcodec: simplify codec id/type validity checking

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Sunday, June 5, 2022 10:20 AM
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/8] lavc/avcodec: simplify codec id/type
> validity checking
> 
> Quoting Soft Works (2022-06-05 09:54:51)
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Anton
> > > Khirnov
> > > Sent: Sunday, June 5, 2022 9:01 AM
> > > To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH 1/8] lavc/avcodec: simplify codec
> id/type
> > > validity checking
> > >
> > > Quoting Soft Works (2022-06-05 07:23:18)
> > > > This is causing a regression in ffprobe.
> > > >
> > > > The commit removes the special-case check for AVMEDIA_TYPE_ATTACHMENT
> which
> > > > was required for ffprobe and had been added with
> > > e83c716e16c52fa56a78274408f7628e5dc719da.
> > > >
> > > > The demand from the commit message is not yet guaranteed to be
> fulfilled:
> > > >
> > > > > On entry to avcodec_open2(), the type and id either have to be
> > > > > UNKNOWN/NONE or have to match the codec to be used.
> > > >
> > > > I have one verified example (maybe a second will follow), which is an
> MKV
> > > with
> > > > an attachment "stream" of type "text".
> > > > The found codec will be textdec of type 'subtitle' even though the
> stream
> > > type
> > > > is attachment. Without the special condition for attachment streams,
> this
> > > > is now causing ffprobe to error out with non-zero exit code and
> incomplete
> > > > output.
> > > >
> > > >
> > > > -----------------------------------------------------------------------
> -
> > > > Example:
> > > >
> > > >   [...]
> > > >   Stream #0:9: Attachment: text
> > > >     Metadata:
> > > >       filename        : textfile.text
> > > >       mimetype        : text/plain
> > > > [text @ 000001AC32310340] Codec type or id mismatches
> > > > Could not open codec for input stream 9
> > > > -----------------------------------------------------------------------
> -
> > >
> > > This sounds very much like a bug in ffprobe. It makes no sense to call
> > > avcodec_open2() with the AVMEDIA_TYPE_ATTACHMENT type.
> >
> > You make a behavioral change to an API function that had this behavior
> > established and constant over more than 10 years, and when that change
> > breaks functionality, it's the callers' fault?
> > How does this go together with all that peanut counting of major, minor
> > and micro version numbers per library? What is this versioning good for,
> > when you can make breaking changes and declare the breakage as bugs?
> 
> We maintain compatibility for valid API usage. We do not maintain bug
> compatibility.
> I fail to see how calling avcodec_open2() with AVMEDIA_TYPE_ATTACHMENT
> is valid API usage. What do you expect it to do? There are no
> AVMEDIA_TYPE_ATTACHMENT decoders.
> 
> More generally, arguments along the line of "change <X> is needed to
> keep program <Y> working>" on their own sound very shady to me and
> suggest that perhaps program <Y> should not be doing whatever it is
> doing.


I might agree to that when:

- the function documentation would have been clear about it
- it wouldn't be ffprobe code getting invalidated by the change


When looking at all the APIs that you are so carefully protecting with
those version numbers, there is a small number of APIs, with good to
great documentation, but a large number where you can't get the
slightest clue about how it is supposed to be used and called and 
which conditions need to be met, which prerequisites are required 
before calling, how to interpret and what to do with the results, etc.
...without looking at the ffmpeg/ffprobe code for how these tools 
are using the APIs.
I mean - almost everybody does that. And when somebody is looking
at the code of ffprobe to understand how to use the ffmpeg API, 
then one needs to be able to rely on the code he sees there 
being correct. And even if it isn't from an ffmpeg internal perspective,
I would still consider it as an API break when a change would 
cause such code fail.

sw