[FFmpeg-devel] [PATCH] avcodec/libx264: remove FF_CODEC_CAP_INIT_THREADSAFE flag

[Marton Balint]

On Sat, 20 Oct 2018, Vittorio Giovara wrote:

> On Sat, Oct 20, 2018 at 12:30 PM Marton Balint <cus at passwd.hu> wrote:
>
>>
>>
>> On Sat, 20 Oct 2018, Hendrik Leppkes wrote:
>>
>> > On Sat, Oct 20, 2018 at 12:35 PM Marton Balint <cus at passwd.hu> wrote:
>> >>
>> >> Libx264 uses strtok which is not thread safe. Strtok is used in
>> >> x264_param_default_preset in param_apply_tune in x264/common/base.c.
>> >> Therefore the flag must be removed.
>> >>
>> >> Fixes ticket #7446.
>> >>
>> >> Signed-off-by: Marton Balint <cus at passwd.hu>
>> >> ---
>> >>  libavcodec/libx264.c | 6 ++----
>> >>  1 file changed, 2 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
>> >> index 54e6703d73..d6367bf557 100644
>> >> --- a/libavcodec/libx264.c
>> >> +++ b/libavcodec/libx264.c
>> >> @@ -1063,8 +1063,7 @@ AVCodec ff_libx264_encoder = {
>> >>      .priv_class       = &x264_class,
>> >>      .defaults         = x264_defaults,
>> >>      .init_static_data = X264_init_static,
>> >> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
>> >> -                        FF_CODEC_CAP_INIT_CLEANUP,
>> >> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
>> >>      .wrapper_name     = "libx264",
>> >>  };
>> >>  #endif
>> >> @@ -1115,8 +1114,7 @@ AVCodec ff_libx262_encoder = {
>> >>      .priv_class       = &X262_class,
>> >>      .defaults         = x264_defaults,
>> >>      .pix_fmts         = pix_fmts_8bit,
>> >> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
>> >> -                        FF_CODEC_CAP_INIT_CLEANUP,
>> >> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
>> >>      .wrapper_name     = "libx264",
>> >>  };
>> >>  #endif
>> >> --
>> >> 2.16.4
>> >>
>> >
>> > Was this ever reported to x264?
>>
>> Not by me.
>>
>> > Theoretically some other library or another part of a calling
>> > application could still use strtok and break it.
>>
>> I agree, this should be fixed in x264 as well, but until then it
>> seems harmless to remove the flag.
>>
>
> this could potentially break a couple of use cases where it "happens to
> work" (especially those that rely on the aforementioned abaa1226)
> rather that dropping the flag entirely, why not fixing x264 first, bump its
> version, and apply the flag conditionally on that?

Fine by me, if someone could pick this up on the x264 side that would be 
great.

Thanks,
Marton