From 385febe27632075160c903a22502fcb78b160ae4 Mon Sep 17 00:00:00 2001 From: wm4 Date: Mon, 6 Jul 2015 21:55:37 +0200 Subject: sub: protect ASS_Renderer state Each subtitle track gets its own decoder instance (sd_ass). But they use a shared ASS_Renderer. This is done mainly because of fontconfig. Initializing fontconfig is very slow when using it with memory fonts, so there's a practical need to cache this memory font state, which is done by not creating separate ASS_Renderers. This is very dirty and very evil, but we probably can't get rid of it any time soon. The shared ASS_Renderer was not properly synchronized. While the program logic guarantees that only one sd_ass instance is visible at a time, there are other interactions that require synchronization. In particular, I suspect concurrent execution of mp_ass_configure_fonts() and sd_ass.get_bitmaps cause issues in a newer libass development branch. So here's a shitty hack that hopefully fixes things, hopefully only until libass becomes less dependent on fontconfig. --- player/core.h | 1 + player/main.c | 3 +++ player/sub.c | 5 ++++- sub/dec_sub.c | 4 +++- sub/dec_sub.h | 4 +++- sub/sd.h | 1 + sub/sd_ass.c | 10 +++++++++- 7 files changed, 24 insertions(+), 4 deletions(-) diff --git a/player/core.h b/player/core.h index 907c997b96..7b3e47d8d2 100644 --- a/player/core.h +++ b/player/core.h @@ -317,6 +317,7 @@ typedef struct MPContext { * loaded across ordered chapters, instead of reloading and rescanning * them on each transition. (Both of these objects contain this state.) */ + pthread_mutex_t ass_lock; struct ass_renderer *ass_renderer; struct ass_library *ass_library; struct mp_log *ass_log; diff --git a/player/main.c b/player/main.c index 06b1867ebe..b5d019db81 100644 --- a/player/main.c +++ b/player/main.c @@ -222,6 +222,7 @@ void mp_destroy(struct MPContext *mpctx) pthread_detach(pthread_self()); mp_msg_uninit(mpctx->global); + pthread_mutex_destroy(&mpctx->ass_lock); talloc_free(mpctx); } @@ -329,6 +330,8 @@ struct MPContext *mp_create(void) .playback_abort = mp_cancel_new(mpctx), }; + pthread_mutex_init(&mpctx->ass_lock, NULL); + mpctx->global = talloc_zero(mpctx, struct mpv_global); // Nothing must call mp_msg*() and related before this diff --git a/player/sub.c b/player/sub.c index 89da9f719c..548e7b22d9 100644 --- a/player/sub.c +++ b/player/sub.c @@ -306,12 +306,15 @@ static void reinit_subdec(struct MPContext *mpctx, struct track *track, sub_set_video_res(dec_sub, w, h); sub_set_video_fps(dec_sub, fps); - sub_set_ass_renderer(dec_sub, mpctx->ass_library, mpctx->ass_renderer); + sub_set_ass_renderer(dec_sub, mpctx->ass_library, mpctx->ass_renderer, + &mpctx->ass_lock); sub_init_from_sh(dec_sub, track->stream); if (mpctx->ass_renderer) { + pthread_mutex_lock(&mpctx->ass_lock); mp_ass_configure_fonts(mpctx->ass_renderer, opts->sub_text_style, mpctx->global, mpctx->ass_log); + pthread_mutex_unlock(&mpctx->ass_lock); } // Don't do this if the file has video/audio streams. Don't do it even diff --git a/sub/dec_sub.c b/sub/dec_sub.c index f03526ca8b..c16f7b5887 100644 --- a/sub/dec_sub.c +++ b/sub/dec_sub.c @@ -156,11 +156,13 @@ void sub_set_extradata(struct dec_sub *sub, void *data, int data_len) } void sub_set_ass_renderer(struct dec_sub *sub, struct ass_library *ass_library, - struct ass_renderer *ass_renderer) + struct ass_renderer *ass_renderer, + pthread_mutex_t *ass_lock) { pthread_mutex_lock(&sub->lock); sub->init_sd.ass_library = ass_library; sub->init_sd.ass_renderer = ass_renderer; + sub->init_sd.ass_lock = ass_lock; pthread_mutex_unlock(&sub->lock); } diff --git a/sub/dec_sub.h b/sub/dec_sub.h index 495ac46402..ddec46e243 100644 --- a/sub/dec_sub.h +++ b/sub/dec_sub.h @@ -3,6 +3,7 @@ #include #include +#include #include "osd.h" @@ -31,7 +32,8 @@ void sub_set_video_res(struct dec_sub *sub, int w, int h); void sub_set_video_fps(struct dec_sub *sub, double fps); void sub_set_extradata(struct dec_sub *sub, void *data, int data_len); void sub_set_ass_renderer(struct dec_sub *sub, struct ass_library *ass_library, - struct ass_renderer *ass_renderer); + struct ass_renderer *ass_renderer, + pthread_mutex_t *ass_lock); void sub_init_from_sh(struct dec_sub *sub, struct sh_stream *sh); bool sub_is_initialized(struct dec_sub *sub); diff --git a/sub/sd.h b/sub/sd.h index 7b2399056a..a77028a43c 100644 --- a/sub/sd.h +++ b/sub/sd.h @@ -28,6 +28,7 @@ struct sd { // Shared renderer for ASS - done to avoid reloading embedded fonts. struct ass_library *ass_library; struct ass_renderer *ass_renderer; + pthread_mutex_t *ass_lock; // If false, try to remove multiple subtitles. // (Only for decoders which have accept_packets_in_advance set.) diff --git a/sub/sd_ass.c b/sub/sd_ass.c index 6f3e24a6f9..f8b1180560 100644 --- a/sub/sd_ass.c +++ b/sub/sd_ass.c @@ -59,7 +59,7 @@ static bool supports_format(const char *format) static int init(struct sd *sd) { struct MPOpts *opts = sd->opts; - if (!sd->ass_library || !sd->ass_renderer || !sd->codec) + if (!sd->ass_library || !sd->ass_renderer || !sd->ass_lock || !sd->codec) return -1; struct sd_ass_priv *ctx = talloc_zero(NULL, struct sd_ass_priv); @@ -67,6 +67,8 @@ static int init(struct sd *sd) ctx->is_converted = sd->converted_from != NULL; + pthread_mutex_lock(sd->ass_lock); + ctx->ass_track = ass_new_track(sd->ass_library); if (!ctx->is_converted) ctx->ass_track->track_type = TRACK_TYPE_ASS; @@ -78,6 +80,8 @@ static int init(struct sd *sd) mp_ass_add_default_styles(ctx->ass_track, opts); + pthread_mutex_unlock(sd->ass_lock); + return 0; } @@ -197,6 +201,8 @@ static void get_bitmaps(struct sd *sd, struct mp_osd_res dim, double pts, if (pts == MP_NOPTS_VALUE || !sd->ass_renderer) return; + pthread_mutex_lock(sd->ass_lock); + ASS_Renderer *renderer = sd->ass_renderer; double scale = dim.display_par; if (!ctx->is_converted && (!opts->ass_style_override || @@ -224,6 +230,8 @@ static void get_bitmaps(struct sd *sd, struct mp_osd_res dim, double pts, if (!ctx->is_converted) mangle_colors(sd, res); + + pthread_mutex_unlock(sd->ass_lock); } struct buf { -- cgit v1.2.3