Message ID | GV1P250MB073766C1F26E45A3838BC14D8FC72@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] avcodec/libx265: Don't copy A53 closed captions by default | 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 |
tis 2024-06-11 klockan 09:42 +0200 skrev Andreas Rheinhardt: > The SEI handling of libx265 is buggy and can easily lead > to memory corruption: It reuses certain buffers, but when > reusing them it presumes that it is enough for these buffers > to exist and does not check whether they are actually large > enough to hold what is intended to be stored in them.* > > Our users are exposed to this because forwarding A53 CC data > is enabled by default. Change this to make it disabled > by default. > > "Fixes" tickets #9666, #10411, #11052 and (presumably) #10906. Shouldn't users use non-buggy versions of libx26? I've had people ask about CC, and I'm sure many users would be annoyed at them suddenly breaking. I suggest complaining loudly at compile time and/or when loading libx265 instead /Tomas
Tomas Härdin: > tis 2024-06-11 klockan 09:42 +0200 skrev Andreas Rheinhardt: >> The SEI handling of libx265 is buggy and can easily lead >> to memory corruption: It reuses certain buffers, but when >> reusing them it presumes that it is enough for these buffers >> to exist and does not check whether they are actually large >> enough to hold what is intended to be stored in them.* >> >> Our users are exposed to this because forwarding A53 CC data >> is enabled by default. Change this to make it disabled >> by default. >> >> "Fixes" tickets #9666, #10411, #11052 and (presumably) #10906. > > Shouldn't users use non-buggy versions of libx26? I've had people ask > about CC, and I'm sure many users would be annoyed at them suddenly > breaking. I suggest complaining loudly at compile time and/or when > loading libx265 instead Non-buggy versions of libx265? People use what they have because it exists. - Andreas
tis 2024-06-11 klockan 10:05 +0200 skrev Andreas Rheinhardt: > Tomas Härdin: > > tis 2024-06-11 klockan 09:42 +0200 skrev Andreas Rheinhardt: > > > The SEI handling of libx265 is buggy and can easily lead > > > to memory corruption: It reuses certain buffers, but when > > > reusing them it presumes that it is enough for these buffers > > > to exist and does not check whether they are actually large > > > enough to hold what is intended to be stored in them.* > > > > > > Our users are exposed to this because forwarding A53 CC data > > > is enabled by default. Change this to make it disabled > > > by default. > > > > > > "Fixes" tickets #9666, #10411, #11052 and (presumably) #10906. > > > > Shouldn't users use non-buggy versions of libx26? I've had people > > ask > > about CC, and I'm sure many users would be annoyed at them suddenly > > breaking. I suggest complaining loudly at compile time and/or when > > loading libx265 instead > > Non-buggy versions of libx265? People use what they have because it > exists. What I'm getting at is that this is libx265's responsibility, and the responsibility of packagers not to ship broken versions of it. Does all A53 CCs break with the present libx265 bug or only some? /Tomas
Tomas Härdin: > tis 2024-06-11 klockan 10:05 +0200 skrev Andreas Rheinhardt: >> Tomas Härdin: >>> tis 2024-06-11 klockan 09:42 +0200 skrev Andreas Rheinhardt: >>>> The SEI handling of libx265 is buggy and can easily lead >>>> to memory corruption: It reuses certain buffers, but when >>>> reusing them it presumes that it is enough for these buffers >>>> to exist and does not check whether they are actually large >>>> enough to hold what is intended to be stored in them.* >>>> >>>> Our users are exposed to this because forwarding A53 CC data >>>> is enabled by default. Change this to make it disabled >>>> by default. >>>> >>>> "Fixes" tickets #9666, #10411, #11052 and (presumably) #10906. >>> >>> Shouldn't users use non-buggy versions of libx26? I've had people >>> ask >>> about CC, and I'm sure many users would be annoyed at them suddenly >>> breaking. I suggest complaining loudly at compile time and/or when >>> loading libx265 instead >> >> Non-buggy versions of libx265? People use what they have because it >> exists. > > What I'm getting at is that this is libx265's responsibility, and the > responsibility of packagers not to ship broken versions of it. Does all > A53 CCs break with the present libx265 bug or only some? > 1. There is no version of libx265 with this bug fixed (the bug itself is here: https://bitbucket.org/multicoreware/x265_git/src/8787e87020d77416f0ff0b7f3c97ac8b90332c31/source/encoder/encoder.cpp#lines-1086:1117) 2. The issue can happen when a later frame uses larger SEIs or more SEIs than an earlier frame. Because x265 thinks that every buffer that exists can handle an arbitrary amount of data. - Andreas
tis 2024-06-11 klockan 10:16 +0200 skrev Andreas Rheinhardt: > Tomas Härdin: > > tis 2024-06-11 klockan 10:05 +0200 skrev Andreas Rheinhardt: > > > Tomas Härdin: > > > > tis 2024-06-11 klockan 09:42 +0200 skrev Andreas Rheinhardt: > > > > > The SEI handling of libx265 is buggy and can easily lead > > > > > to memory corruption: It reuses certain buffers, but when > > > > > reusing them it presumes that it is enough for these buffers > > > > > to exist and does not check whether they are actually large > > > > > enough to hold what is intended to be stored in them.* > > > > > > > > > > Our users are exposed to this because forwarding A53 CC data > > > > > is enabled by default. Change this to make it disabled > > > > > by default. > > > > > > > > > > "Fixes" tickets #9666, #10411, #11052 and (presumably) > > > > > #10906. > > > > > > > > Shouldn't users use non-buggy versions of libx26? I've had > > > > people > > > > ask > > > > about CC, and I'm sure many users would be annoyed at them > > > > suddenly > > > > breaking. I suggest complaining loudly at compile time and/or > > > > when > > > > loading libx265 instead > > > > > > Non-buggy versions of libx265? People use what they have because > > > it > > > exists. > > > > What I'm getting at is that this is libx265's responsibility, and > > the > > responsibility of packagers not to ship broken versions of it. Does > > all > > A53 CCs break with the present libx265 bug or only some? > > > > 1. There is no version of libx265 with this bug fixed (the bug itself > is > here: > https://bitbucket.org/multicoreware/x265_git/src/8787e87020d77416f0ff0b7f3c97ac8b90332c31/source/encoder/encoder.cpp#lines-1086:1117 > ) Then I expect libx265 to fix it posthaste and push a new release, and for Debian etc to discourage installing versions prior to that as appropriate We can change the default of course (yolo!), but expect non-zero numbers of angry users whose workflows suddenly break /Tomas
Andreas Rheinhardt: > The SEI handling of libx265 is buggy and can easily lead > to memory corruption: It reuses certain buffers, but when > reusing them it presumes that it is enough for these buffers > to exist and does not check whether they are actually large > enough to hold what is intended to be stored in them.* > > Our users are exposed to this because forwarding A53 CC data > is enabled by default. Change this to make it disabled > by default. > > "Fixes" tickets #9666, #10411, #11052 and (presumably) #10906. > > *: See https://trac.ffmpeg.org/ticket/9666#comment:1 > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/libx265.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c > index ac1dbc4f97..0dc7ab6eeb 100644 > --- a/libavcodec/libx265.c > +++ b/libavcodec/libx265.c > @@ -948,7 +948,7 @@ static const AVOption options[] = { > { "tune", "set the x265 tune parameter", OFFSET(tune), AV_OPT_TYPE_STRING, { 0 }, 0, 0, VE }, > { "profile", "set the x265 profile", OFFSET(profile), AV_OPT_TYPE_STRING, { 0 }, 0, 0, VE }, > { "udu_sei", "Use user data unregistered SEI if available", OFFSET(udu_sei), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, > - { "a53cc", "Use A53 Closed Captions (if available)", OFFSET(a53_cc), AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, VE }, > + { "a53cc", "Use A53 Closed Captions (if available)", OFFSET(a53_cc), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, > { "x265-params", "set the x265 configuration using a :-separated list of key=value parameters", OFFSET(x265_opts), AV_OPT_TYPE_DICT, { 0 }, 0, 0, VE }, > #if X265_BUILD >= 167 > { "dolbyvision", "Enable Dolby Vision RPU coding", OFFSET(dovi.enable), AV_OPT_TYPE_BOOL, {.i64 = FF_DOVI_AUTOMATIC }, -1, 1, VE, .unit = "dovi" }, Will apply this patch tomorrow unless there are objections. - Andreas
diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c index ac1dbc4f97..0dc7ab6eeb 100644 --- a/libavcodec/libx265.c +++ b/libavcodec/libx265.c @@ -948,7 +948,7 @@ static const AVOption options[] = { { "tune", "set the x265 tune parameter", OFFSET(tune), AV_OPT_TYPE_STRING, { 0 }, 0, 0, VE }, { "profile", "set the x265 profile", OFFSET(profile), AV_OPT_TYPE_STRING, { 0 }, 0, 0, VE }, { "udu_sei", "Use user data unregistered SEI if available", OFFSET(udu_sei), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, - { "a53cc", "Use A53 Closed Captions (if available)", OFFSET(a53_cc), AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, VE }, + { "a53cc", "Use A53 Closed Captions (if available)", OFFSET(a53_cc), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, { "x265-params", "set the x265 configuration using a :-separated list of key=value parameters", OFFSET(x265_opts), AV_OPT_TYPE_DICT, { 0 }, 0, 0, VE }, #if X265_BUILD >= 167 { "dolbyvision", "Enable Dolby Vision RPU coding", OFFSET(dovi.enable), AV_OPT_TYPE_BOOL, {.i64 = FF_DOVI_AUTOMATIC }, -1, 1, VE, .unit = "dovi" },
The SEI handling of libx265 is buggy and can easily lead to memory corruption: It reuses certain buffers, but when reusing them it presumes that it is enough for these buffers to exist and does not check whether they are actually large enough to hold what is intended to be stored in them.* Our users are exposed to this because forwarding A53 CC data is enabled by default. Change this to make it disabled by default. "Fixes" tickets #9666, #10411, #11052 and (presumably) #10906. *: See https://trac.ffmpeg.org/ticket/9666#comment:1 Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/libx265.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)