| Message ID | 20220323155720.20017-1-anton@khirnov.net |
|---|---|
| State | Accepted |
| Commit | 7efa6418b83ae123770c6930f492edb87cd19274 |
| Headers | show |
| Series | [FFmpeg-devel,1/8] lavc/avcodec: simplify codec id/type validity checking | 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 |
pushed patches 1-3 + new patch dropping EncodeSimpleContext. Will push the rest soonish if nobody objects.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > Khirnov > Sent: Wednesday, March 23, 2022 4:57 PM > To: ffmpeg-devel@ffmpeg.org > Subject: [FFmpeg-devel] [PATCH 1/8] lavc/avcodec: simplify codec id/type > validity checking > > On entry to avcodec_open2(), the type and id either have to be > UNKNOWN/NONE or have to match the codec to be used. > --- > libavcodec/avcodec.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c > index fbe4a5e413..dbaa9f78a2 100644 > --- a/libavcodec/avcodec.c > +++ b/libavcodec/avcodec.c > @@ -158,17 +158,15 @@ int attribute_align_arg avcodec_open2(AVCodecContext > *avctx, const AVCodec *code > codec = avctx->codec; > codec2 = ffcodec(codec); > > - if ((avctx->codec_type == AVMEDIA_TYPE_UNKNOWN || avctx->codec_type == > codec->type) && > - avctx->codec_id == AV_CODEC_ID_NONE) { > - avctx->codec_type = codec->type; > - avctx->codec_id = codec->id; > - } > - if (avctx->codec_id != codec->id || (avctx->codec_type != codec->type && > - avctx->codec_type != > AVMEDIA_TYPE_ATTACHMENT)) { > + if ((avctx->codec_type != AVMEDIA_TYPE_UNKNOWN && avctx->codec_type != > codec->type) || > + (avctx->codec_id != AV_CODEC_ID_NONE && avctx->codec_id != > codec->id)) { > av_log(avctx, AV_LOG_ERROR, "Codec type or id mismatches\n"); > return AVERROR(EINVAL); > } > - avctx->codec = codec; > + > + avctx->codec_type = codec->type; > + avctx->codec_id = codec->id; > + avctx->codec = codec; > > if (avctx->extradata_size < 0 || avctx->extradata_size >= > FF_MAX_EXTRADATA_SIZE) > return AVERROR(EINVAL); > -- 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 ------------------------------------------------------------------------ Regards, softworkz
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.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > Khirnov > Sent: Sunday, June 5, 2022 9:01 AM > To: FFmpeg development discussions and patches <ffmpeg-devel@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? Though, I don't want to say that your change is wrong or shouldn't be made. Yet, the change requires ffprobe to be adjusted (I just wouldn't call it a "bug fix"..) Thanks, softworkz
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Soft Works > Sent: Sunday, June 5, 2022 9:55 AM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 1/8] lavc/avcodec: simplify codec id/type > validity checking > > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > > Khirnov > > Sent: Sunday, June 5, 2022 9:01 AM > > To: FFmpeg development discussions and patches <ffmpeg-devel@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? As per your logic - wouldn't this change require a major version bump? Or even an avcodec_open3() to ensure backward-compatibility?
Quoting Soft Works (2022-06-05 09:54:51) > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > > Khirnov > > Sent: Sunday, June 5, 2022 9:01 AM > > To: FFmpeg development discussions and patches <ffmpeg-devel@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.
On Sun, Jun 5, 2022 at 10:20 AM Anton Khirnov <anton@khirnov.net> wrote: > Quoting Soft Works (2022-06-05 09:54:51) > > > > > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Anton > > > Khirnov > > > Sent: Sunday, June 5, 2022 9:01 AM > > > To: FFmpeg development discussions and patches < > ffmpeg-devel@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. > You broke existing functionality, fix it ASAP or revert those changes! > -- > Anton Khirnov > _______________________________________________ > 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". >
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > Khirnov > Sent: Sunday, June 5, 2022 10:20 AM > To: FFmpeg development discussions and patches <ffmpeg-devel@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@ffmpeg.org> On Behalf Of Anton > > > Khirnov > > > Sent: Sunday, June 5, 2022 9:01 AM > > > To: FFmpeg development discussions and patches <ffmpeg-devel@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
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > Khirnov > Sent: Sunday, June 5, 2022 10:20 AM > To: FFmpeg development discussions and patches <ffmpeg-devel@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@ffmpeg.org> On Behalf Of Anton > > > Khirnov > > > Sent: Sunday, June 5, 2022 9:01 AM > > > To: FFmpeg development discussions and patches <ffmpeg-devel@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
So much text, but no actual answer. Again: > 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.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > Khirnov > Sent: Sunday, June 5, 2022 12:42 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 1/8] lavc/avcodec: simplify codec id/type > validity checking > > So much text, but no actual answer. Again: > > 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. Yes. That's perfectly right. But it doesn't matter at this point. And I used "so much text" to precisely explain why it doesn't matter.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > Khirnov > Sent: Sunday, June 5, 2022 12:42 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 1/8] lavc/avcodec: simplify codec id/type > validity checking > > So much text, but no actual answer. Again: > > 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. As you didn't mention anything about how you want to address it, does it mean that your intention is to leave it as is and declare all other code being wrong?
Quoting Soft Works (2022-06-05 13:10:49) > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > > Khirnov > > Sent: Sunday, June 5, 2022 12:42 PM > > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > > Subject: Re: [FFmpeg-devel] [PATCH 1/8] lavc/avcodec: simplify codec id/type > > validity checking > > > > So much text, but no actual answer. Again: > > > 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. > > As you didn't mention anything about how you want to address it, does it > mean that your intention is to leave it as is and declare all other code > being wrong? Frankly, your attitude of breathlessly repeating "ffprobe is BROKEN, and this is HORRIBLE" is unhelpful. I am open to various kinds of solutions, which include (temporarily?) reintroducing previous behavior, but first we must determine what the actual issue is. I.e. whether it is libavcodec or ffprobe that is broken. You seem uninterested in this question, which makes me not very interested in spending time on this.
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > Khirnov > Sent: Sunday, June 5, 2022 3:21 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 1/8] lavc/avcodec: simplify codec id/type > validity checking > > Quoting Soft Works (2022-06-05 13:10:49) > > > > > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Anton > > > Khirnov > > > Sent: Sunday, June 5, 2022 12:42 PM > > > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > > > Subject: Re: [FFmpeg-devel] [PATCH 1/8] lavc/avcodec: simplify codec > id/type > > > validity checking > > > > > > So much text, but no actual answer. Again: > > > > 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. > > > > As you didn't mention anything about how you want to address it, does it > > mean that your intention is to leave it as is and declare all other code > > being wrong? > > Frankly, your attitude of breathlessly repeating "ffprobe is BROKEN, and > this is HORRIBLE" is unhelpful. > I am open to various kinds of solutions, which include (temporarily?) > reintroducing previous behavior, but first we must determine what the > actual issue is. I.e. whether it is libavcodec or ffprobe that is > broken. You seem uninterested in this question, which makes me not very > interested in spending time on this. I need to fight about every single character of submitted code, and you are trying to justify your commit that clearly breaks behavior instead of either reverting or offering a solution. Instead I need to go through stupid discussions with you. I don't understand that behavior. For most others it would be totally clear that such commit would need to be reverted until a new solution is found. I have reported the issue nicely and well explained. But you start to find some justifications instead of suggesting any solution. I don't like that. I wish I wouldn't have been required to write that much text, and you would have just responded something like, OK, I'll see how I can resolve the regression that my commit has caused. That would be a normal reaction IMO. Regards, softworkz
diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c index fbe4a5e413..dbaa9f78a2 100644 --- a/libavcodec/avcodec.c +++ b/libavcodec/avcodec.c @@ -158,17 +158,15 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code codec = avctx->codec; codec2 = ffcodec(codec); - if ((avctx->codec_type == AVMEDIA_TYPE_UNKNOWN || avctx->codec_type == codec->type) && - avctx->codec_id == AV_CODEC_ID_NONE) { - avctx->codec_type = codec->type; - avctx->codec_id = codec->id; - } - if (avctx->codec_id != codec->id || (avctx->codec_type != codec->type && - avctx->codec_type != AVMEDIA_TYPE_ATTACHMENT)) { + if ((avctx->codec_type != AVMEDIA_TYPE_UNKNOWN && avctx->codec_type != codec->type) || + (avctx->codec_id != AV_CODEC_ID_NONE && avctx->codec_id != codec->id)) { av_log(avctx, AV_LOG_ERROR, "Codec type or id mismatches\n"); return AVERROR(EINVAL); } - avctx->codec = codec; + + avctx->codec_type = codec->type; + avctx->codec_id = codec->id; + avctx->codec = codec; if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE) return AVERROR(EINVAL);