lavc/hevcdec: unbreak WPP/progress2 code

The "progress2" API in pthread_slice.c currently associates a progress value with a thread rather than a job, relying on the broken assumption that a job's thread number is equal to its job number modulo thread count. This removes this API entirely, and changes hevcdec to use a ThreadProgress-based implementation that associates a mutex/cond/progress value with every job. Fixes races and deadlocks in hevdec with slice threading, e.g. some of those mentioned in #11221.

Anton Khirnov committed 2 years ago

Commit 79c47df1 parent c50f79a5 files · +46 −139Browse files

Unified · Split

5 files

libavcodec/hevc/hevcdec.c +43 −14
libavcodec/hevc/hevcdec.h +3 −0
libavcodec/pthread_slice.c +0 −102
libavcodec/thread.h +0 −4
libavcodec/utils.c +0 −19

libavcodec/hevc/hevcdec.c+43−14

@@ -54,6 +54,7 @@ · 53 unmodified lines
54	54	#include "progressframe.h"
55	55	#include "refstruct.h"
56	56	#include "thread.h"
	57	+#include "threadprogress.h"
57	58
58	59	static const uint8_t hevc_pel_weight[65] = { [2] = 0, [4] = 1, [6] = 2, [8] = 3, [12] = 4, [16] = 5, [24] = 6, [32] = 7, [48] = 8, [64] = 9 };
59	60
@@ -2751,6 +2752,8 @@ static int hls_decode_entry_wpp(AVCodecContext avctx, void hevc_lclist, · 2691 unmodified lines
2751	2752	const uint8_t *data = s->data + s->sh.offset[ctb_row];
2752	2753	const size_t data_size = s->sh.size[ctb_row];
2753	2754
	2755	+ int progress = 0;
	2756	+
2754	2757	int ret;
2755	2758
2756	2759	if (ctb_row)
@@ -2762,13 +2765,15 @@ static int hls_decode_entry_wpp(AVCodecContext avctx, void hevc_lclist, · 5 unmodified lines
2762	2765
2763	2766	hls_decode_neighbour(lc, l, pps, sps, x_ctb, y_ctb, ctb_addr_ts);
2764	2767
2765		− ff_thread_await_progress2(s->avctx, ctb_row, thread, SHIFT_CTB_WPP);
	2768	+ if (ctb_row)
	2769	+ ff_thread_progress_await(&s->wpp_progress[ctb_row - 1],
	2770	+ progress + SHIFT_CTB_WPP + 1);
2766	2771
2767	2772	/* atomic_load's prototype requires a pointer to non-const atomic variable
2768	2773	* (due to implementations via mutexes, where reads involve writes).
2769	2774	* Of course, casting const away here is nevertheless safe. */
2770	2775	if (atomic_load((atomic_int*)&s->wpp_err)) {
2771		− ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP);
	2776	+ ff_thread_progress_report(&s->wpp_progress[ctb_row], INT_MAX);
2772	2777	return 0;
2773	2778	}
2774	2779
@@ -2792,19 +2797,19 @@ static int hls_decode_entry_wpp(AVCodecContext avctx, void hevc_lclist, · 17 unmodified lines
2792	2797	ctb_addr_ts++;
2793	2798
2794	2799	ff_hevc_save_states(lc, pps, ctb_addr_ts);
2795		− ff_thread_report_progress2(s->avctx, ctb_row, thread, 1);
	2800	+ ff_thread_progress_report(&s->wpp_progress[ctb_row], ++progress);
2796	2801	ff_hevc_hls_filters(lc, l, pps, x_ctb, y_ctb, ctb_size);
2797	2802
2798	2803	if (!more_data && (x_ctb+ctb_size) < sps->width && ctb_row != s->sh.num_entry_point_offsets) {
2799	2804	/* Casting const away here is safe, because it is an atomic operation. */
2800	2805	atomic_store((atomic_int*)&s->wpp_err, 1);
2801		− ff_thread_report_progress2(s->avctx, ctb_row ,thread, SHIFT_CTB_WPP);
	2806	+ ff_thread_progress_report(&s->wpp_progress[ctb_row], INT_MAX);
2802	2807	return 0;
2803	2808	}
2804	2809
2805	2810	if ((x_ctb+ctb_size) >= sps->width && (y_ctb+ctb_size) >= sps->height ) {
2806	2811	ff_hevc_hls_filter(lc, l, pps, x_ctb, y_ctb, ctb_size);
2807		− ff_thread_report_progress2(s->avctx, ctb_row , thread, SHIFT_CTB_WPP);
	2812	+ ff_thread_progress_report(&s->wpp_progress[ctb_row], INT_MAX);
2808	2813	return ctb_addr_ts;
2809	2814	}
2810	2815	ctb_addr_rs = pps->ctb_addr_ts_to_rs[ctb_addr_ts];
@@ -2814,17 +2819,43 @@ static int hls_decode_entry_wpp(AVCodecContext avctx, void hevc_lclist, · 3 unmodified lines
2814	2819	break;
2815	2820	}
2816	2821	}
2817		− ff_thread_report_progress2(s->avctx, ctb_row ,thread, SHIFT_CTB_WPP);
	2822	+ ff_thread_progress_report(&s->wpp_progress[ctb_row], INT_MAX);
2818	2823
2819	2824	return 0;
2820	2825	error:
2821	2826	l->tab_slice_address[ctb_addr_rs] = -1;
2822	2827	/* Casting const away here is safe, because it is an atomic operation. */
2823	2828	atomic_store((atomic_int*)&s->wpp_err, 1);
2824		− ff_thread_report_progress2(s->avctx, ctb_row ,thread, SHIFT_CTB_WPP);
	2829	+ ff_thread_progress_report(&s->wpp_progress[ctb_row], INT_MAX);
2825	2830	return ret;
2826	2831	}
2827	2832
	2833	+static int wpp_progress_init(HEVCContext *s, unsigned count)
	2834	+{
	2835	+ if (s->nb_wpp_progress < count) {
	2836	+ void *tmp = av_realloc_array(s->wpp_progress, count,
	2837	+ sizeof(*s->wpp_progress));
	2838	+ if (!tmp)
	2839	+ return AVERROR(ENOMEM);
	2840	+
	2841	+ s->wpp_progress = tmp;
	2842	+ memset(s->wpp_progress + s->nb_wpp_progress, 0,
	2843	+ (count - s->nb_wpp_progress) * sizeof(*s->wpp_progress));
	2844	+
	2845	+ for (int i = s->nb_wpp_progress; i < count; i++) {
	2846	+ int ret = ff_thread_progress_init(&s->wpp_progress[i], 1);
	2847	+ if (ret < 0)
	2848	+ return ret;
	2849	+ s->nb_wpp_progress = i + 1;
	2850	+ }
	2851	+ }
	2852	+
	2853	+ for (int i = 0; i < count; i++)
	2854	+ ff_thread_progress_reset(&s->wpp_progress[i]);
	2855	+
	2856	+ return 0;
	2857	+}
	2858	+
2828	2859	static int hls_slice_data_wpp(HEVCContext s, const H2645NAL nal)
2829	2860	{
2830	2861	const HEVCPPS *const pps = s->pps;
@@ -2909,7 +2940,7 @@ static int hls_slice_data_wpp(HEVCContext s, const H2645NAL nal) · 78 unmodified lines
2909	2940	}
2910	2941
2911	2942	atomic_store(&s->wpp_err, 0);
2912		− res = ff_slice_thread_allocz_entries(s->avctx, s->sh.num_entry_point_offsets + 1);
	2943	+ res = wpp_progress_init(s, s->sh.num_entry_point_offsets + 1);
2913	2944	if (res < 0)
2914	2945	return res;
2915	2946
@@ -3826,6 +3857,10 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx) · 910 unmodified lines
3826	3857
3827	3858	ff_hevc_ps_uninit(&s->ps);
3828	3859
	3860	+ for (int i = 0; i < s->nb_wpp_progress; i++)
	3861	+ ff_thread_progress_destroy(&s->wpp_progress[i]);
	3862	+ av_freep(&s->wpp_progress);
	3863	+
3829	3864	av_freep(&s->sh.entry_point_offset);
3830	3865	av_freep(&s->sh.offset);
3831	3866	av_freep(&s->sh.size);
@@ -3981,12 +4016,6 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) · 149 unmodified lines
3981	4016	HEVCContext *s = avctx->priv_data;
3982	4017	int ret;
3983	4018
3984		− if (avctx->active_thread_type & FF_THREAD_SLICE) {
3985		− ret = ff_slice_thread_init_progress(avctx);
3986		− if (ret < 0)
3987		− return ret;
3988		− }
3989		−
3990	4019	ret = hevc_init_context(avctx);
3991	4020	if (ret < 0)
3992	4021	return ret;

libavcodec/hevc/hevcdec.h+3−0

@@ -540,6 +540,9 @@ typedef struct HEVCContext { · 539 unmodified lines
540	540	/** The target for the common_cabac_state of the local contexts. */
541	541	HEVCCABACState cabac;
542	542
	543	+ struct ThreadProgress *wpp_progress;
	544	+ unsigned nb_wpp_progress;
	545	+
543	546	atomic_int wpp_err;
544	547
545	548	const uint8_t *data;

libavcodec/pthread_slice.c+0−102

@@ -41,11 +41,6 @@ typedef int (action_func)(AVCodecContext c, void arg); · 40 unmodified lines
41	41	typedef int (action_func2)(AVCodecContext c, void arg, int jobnr, int threadnr);
42	42	typedef int (main_func)(AVCodecContext *c);
43	43
44		−typedef struct Progress {
45		− pthread_cond_t cond;
46		− pthread_mutex_t mutex;
47		−} Progress;
48		−
49	44	typedef struct SliceThreadContext {
50	45	AVSliceThread *thread;
51	46	action_func *func;
@@ -54,11 +49,6 @@ typedef struct SliceThreadContext { · 2 unmodified lines
54	49	void *args;
55	50	int *rets;
56	51	int job_size;
57		−
58		− int *entries;
59		− int entries_count;
60		− int thread_count;
61		− Progress *progress;
62	52	} SliceThreadContext;
63	53
64	54	static void main_function(void *priv) {
@@ -82,18 +72,9 @@ static void worker_func(void *priv, int jobnr, int threadnr, int nb_jobs, int nb · 17 unmodified lines
82	72	void ff_slice_thread_free(AVCodecContext *avctx)
83	73	{
84	74	SliceThreadContext *c = avctx->internal->thread_ctx;
85		− int i;
86	75
87	76	avpriv_slicethread_free(&c->thread);
88	77
89		− for (i = 0; i < c->thread_count; i++) {
90		− Progress *const progress = &c->progress[i];
91		− pthread_mutex_destroy(&progress->mutex);
92		− pthread_cond_destroy(&progress->cond);
93		− }
94		−
95		− av_freep(&c->entries);
96		− av_freep(&c->progress);
97	78	av_freep(&avctx->internal->thread_ctx);
98	79	}
99	80
@@ -175,86 +156,3 @@ int ff_slice_thread_init(AVCodecContext *avctx) · 75 unmodified lines
175	156	avctx->execute2 = thread_execute2;
176	157	return 0;
177	158	}
178		−
179		−int av_cold ff_slice_thread_init_progress(AVCodecContext *avctx)
180		−{
181		− SliceThreadContext *const p = avctx->internal->thread_ctx;
182		− int err, i = 0, thread_count = avctx->thread_count;
183		−
184		− p->progress = av_calloc(thread_count, sizeof(*p->progress));
185		− if (!p->progress) {
186		− err = AVERROR(ENOMEM);
187		− goto fail;
188		− }
189		−
190		− for (; i < thread_count; i++) {
191		− Progress *const progress = &p->progress[i];
192		− err = pthread_mutex_init(&progress->mutex, NULL);
193		− if (err) {
194		− err = AVERROR(err);
195		− goto fail;
196		− }
197		− err = pthread_cond_init (&progress->cond, NULL);
198		− if (err) {
199		− err = AVERROR(err);
200		− pthread_mutex_destroy(&progress->mutex);
201		− goto fail;
202		− }
203		− }
204		− err = 0;
205		−fail:
206		− p->thread_count = i;
207		− return err;
208		−}
209		−
210		−void ff_thread_report_progress2(AVCodecContext *avctx, int field, int thread, int n)
211		−{
212		− SliceThreadContext *p = avctx->internal->thread_ctx;
213		− Progress *const progress = &p->progress[thread];
214		− int *entries = p->entries;
215		−
216		− pthread_mutex_lock(&progress->mutex);
217		− entries[field] +=n;
218		− pthread_cond_signal(&progress->cond);
219		− pthread_mutex_unlock(&progress->mutex);
220		−}
221		−
222		−void ff_thread_await_progress2(AVCodecContext *avctx, int field, int thread, int shift)
223		−{
224		− SliceThreadContext *p = avctx->internal->thread_ctx;
225		− Progress *progress;
226		− int *entries = p->entries;
227		−
228		− if (!entries \|\| !field) return;
229		−
230		− thread = thread ? thread - 1 : p->thread_count - 1;
231		− progress = &p->progress[thread];
232		−
233		− pthread_mutex_lock(&progress->mutex);
234		− while ((entries[field - 1] - entries[field]) < shift){
235		− pthread_cond_wait(&progress->cond, &progress->mutex);
236		− }
237		− pthread_mutex_unlock(&progress->mutex);
238		−}
239		−
240		−int ff_slice_thread_allocz_entries(AVCodecContext *avctx, int count)
241		−{
242		− if (avctx->active_thread_type & FF_THREAD_SLICE) {
243		− SliceThreadContext *p = avctx->internal->thread_ctx;
244		−
245		− if (p->entries_count == count) {
246		− memset(p->entries, 0, p->entries_count * sizeof(*p->entries));
247		− return 0;
248		− }
249		− av_freep(&p->entries);
250		−
251		− p->entries = av_calloc(count, sizeof(*p->entries));
252		− if (!p->entries) {
253		− p->entries_count = 0;
254		− return AVERROR(ENOMEM);
255		− }
256		− p->entries_count = count;
257		− }
258		−
259		− return 0;
260		−}

libavcodec/thread.h+0−4

@@ -56,10 +56,6 @@ int ff_thread_get_buffer(AVCodecContext avctx, AVFrame f, int flags); · 55 unmodified lines
56	56	int ff_slice_thread_execute_with_mainfunc(AVCodecContext *avctx,
57	57	int (action_func2)(AVCodecContext c, void *arg, int jobnr, int threadnr),
58	58	int (main_func)(AVCodecContext c), void arg, int ret, int job_count);
59		−int ff_slice_thread_allocz_entries(AVCodecContext *avctx, int count);
60		−int ff_slice_thread_init_progress(AVCodecContext *avctx);
61		−void ff_thread_report_progress2(AVCodecContext *avctx, int field, int thread, int n);
62		−void ff_thread_await_progress2(AVCodecContext *avctx, int field, int thread, int shift);
63	59
64	60	enum ThreadingStatus {
65	61	FF_THREAD_IS_COPY,

libavcodec/utils.c+0−19

@@ -913,25 +913,6 @@ int ff_thread_can_start_frame(AVCodecContext *avctx) · 912 unmodified lines
913	913	{
914	914	return 1;
915	915	}
916		−
917		−int ff_slice_thread_init_progress(AVCodecContext *avctx)
918		−{
919		− return 0;
920		−}
921		−
922		−int ff_slice_thread_allocz_entries(AVCodecContext *avctx, int count)
923		−{
924		− return 0;
925		−}
926		−
927		−void ff_thread_await_progress2(AVCodecContext *avctx, int field, int thread, int shift)
928		−{
929		−}
930		−
931		−void ff_thread_report_progress2(AVCodecContext *avctx, int field, int thread, int n)
932		−{
933		−}
934		−
935	916	#endif
936	917
937	918	const uint8_t avpriv_find_start_code(const uint8_t restrict p,

lavc/hevcdec: unbreak WPP/progress2 code

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code