[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.

Shady? I don't understand how you can say that.


First of all: 

You broke ffprobe. 

It took me one and a half working days to trace this back from the
symptom that the user was experiencing to that commit of yours.

Now, you could fix that in ffprobe - but then it would be still 
broken for:

1. Applications which have copied the code from ffprobe
2. Users running (an unchanged) ffprobe with a newer libavcodec
   version; (you said it would be so important that the libraries
   are exchangeable and multiple versions can be mixed)


Adjusting ffprobe would still break these two cases.

Thanks,
sw