| Message ID | 20230315144525.4140-1-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/libx265: flag as experimental | 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 |
On 15/03/2023 15:45, Anton Khirnov wrote: > This encoder leaks and overreads, as can be seen e.g. by running an > encode under valgrind with default encoder parameters. This was known > upstream since at least 2019 (e.g. bitbucket issue #482) but never fixed > until now. > > Since upstream does not seem to practice basic code hygiene, make sure > people do not use this encoder without knowing what they are getting > into. > --- > libavcodec/libx265.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c > index 420d0953af..d4511251a5 100644 > --- a/libavcodec/libx265.c > +++ b/libavcodec/libx265.c > @@ -895,7 +895,8 @@ FFCodec ff_libx265_encoder = { > .p.id = AV_CODEC_ID_HEVC, > .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY | > AV_CODEC_CAP_OTHER_THREADS | > - AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE, > + AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE | > + AV_CODEC_CAP_EXPERIMENTAL, > .p.priv_class = &class, > .p.wrapper_name = "libx265", > .init = libx265_encode_init, My understanding of this flag for encoders is that it indicates some possible experimental/non-standard bitstream features contained in output data, not that the implementation itself is considered sub-par. Regards, Tobias
Quoting James Almer (2023-03-15 15:46:41) > On 3/15/2023 11:45 AM, Anton Khirnov wrote: > > This encoder leaks and overreads, as can be seen e.g. by running an > > encode under valgrind with default encoder parameters. This was known > > upstream since at least 2019 (e.g. bitbucket issue #482) but never fixed > > until now. > > > > Since upstream does not seem to practice basic code hygiene, make sure > > people do not use this encoder without knowing what they are getting > > into. > > This is a really bad idea. It will break several scripts and command > lines that were working perfectly fine and giving the desired results. I thought we prioritize security and correctness over keeping commandlines working at all cost. Besides, this might motivate someone into actually fixing x265. We have a long-standing reputation for being an endless source of security issues. I think we should take stands like this more often if we want to get rid of it.
Quoting Tobias Rapp (2023-03-15 16:00:04) > On 15/03/2023 15:45, Anton Khirnov wrote: > > > This encoder leaks and overreads, as can be seen e.g. by running an > > encode under valgrind with default encoder parameters. This was known > > upstream since at least 2019 (e.g. bitbucket issue #482) but never fixed > > until now. > > > > Since upstream does not seem to practice basic code hygiene, make sure > > people do not use this encoder without knowing what they are getting > > into. > > --- > > libavcodec/libx265.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c > > index 420d0953af..d4511251a5 100644 > > --- a/libavcodec/libx265.c > > +++ b/libavcodec/libx265.c > > @@ -895,7 +895,8 @@ FFCodec ff_libx265_encoder = { > > .p.id = AV_CODEC_ID_HEVC, > > .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY | > > AV_CODEC_CAP_OTHER_THREADS | > > - AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE, > > + AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE | > > + AV_CODEC_CAP_EXPERIMENTAL, > > .p.priv_class = &class, > > .p.wrapper_name = "libx265", > > .init = libx265_encode_init, > > My understanding of this flag for encoders is that it indicates some > possible experimental/non-standard bitstream features contained in > output data, not that the implementation itself is considered sub-par. Our internal aac encoder used to be marked as experimental. vorbis and opus still are. So it's clearly about the implementation quality rather than standard compliance.
Tobias Rapp: > On 15/03/2023 15:45, Anton Khirnov wrote: > >> This encoder leaks and overreads, as can be seen e.g. by running an >> encode under valgrind with default encoder parameters. This was known >> upstream since at least 2019 (e.g. bitbucket issue #482) but never fixed >> until now. >> >> Since upstream does not seem to practice basic code hygiene, make sure >> people do not use this encoder without knowing what they are getting >> into. >> --- >> libavcodec/libx265.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c >> index 420d0953af..d4511251a5 100644 >> --- a/libavcodec/libx265.c >> +++ b/libavcodec/libx265.c >> @@ -895,7 +895,8 @@ FFCodec ff_libx265_encoder = { >> .p.id = AV_CODEC_ID_HEVC, >> .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY | >> AV_CODEC_CAP_OTHER_THREADS | >> - AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE, >> + AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE | >> + AV_CODEC_CAP_EXPERIMENTAL, >> .p.priv_class = &class, >> .p.wrapper_name = "libx265", >> .init = libx265_encode_init, > > My understanding of this flag for encoders is that it indicates some > possible experimental/non-standard bitstream features contained in > output data, not that the implementation itself is considered sub-par. > You are confusing the AV_CODEC_CAP_EXPERIMENTAL with FF_COMPLIANCE_EXPERIMENTAL. - Andreas
diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c index 420d0953af..d4511251a5 100644 --- a/libavcodec/libx265.c +++ b/libavcodec/libx265.c @@ -895,7 +895,8 @@ FFCodec ff_libx265_encoder = { .p.id = AV_CODEC_ID_HEVC, .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY | AV_CODEC_CAP_OTHER_THREADS | - AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE, + AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE | + AV_CODEC_CAP_EXPERIMENTAL, .p.priv_class = &class, .p.wrapper_name = "libx265", .init = libx265_encode_init,