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

[Hendrik Leppkes]

On Sat, Oct 20, 2018 at 4:50 PM Vittorio Giovara
<vittorio.giovara at gmail.com> 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?

What exactly would this break? avcodec should just do the right thing.
It would just be slower.
The flag can be re-introduced if a fixed version is released.

- Hendrik