[vlc-devel] [PATCH] splitter: use a different thread for each display
Alexandre Janniaux
ajanni at videolabs.io
Wed Dec 11 14:05:54 CET 2019
Hi,
On Wed, Dec 11, 2019 at 01:12:46PM +0200, Rémi Denis-Courmont wrote:
> If you want thread barriers, use thread barriers, not semaphore. That's super inefficient, ironically for a performance-focused patch.
Does VLC threads have thread barriers ? I looked for them but
didn't find them. The splitter is platform-independant, so if
I have to implement them for every platforms, or reimplement
them with other primitives, I don't think it's a reasonable
change to do this for this patchset. Would you prefer a mutex
and condvar implementation? The main benefit of semaphore was
clarity here.
The patch is not a performance-focused patch. It does not try
to improve the performance but solves an concurrent issue
(one thread for multiple outputs) with a parallelization
solution (one thread per output). The fact that it might lead
to different performance metrics is just a side effect.
> It's also highly questionable idea anyway. This looks like a bug in the GL display, as this can't work anyway with high frame rate.
I don't think that's a bug. VSYNC and sleeping can be
mandatory on some (if not most) platforms. Either the OpenGL
and other alike backends should run in its own thread or this
patch is needed. Both solution are fine to me but I see no
issues with parallelizing the splitter as the cost of prepare
is likely better parallelized anyway for CPU renderers. With
engines like OpenGL, it can also prevent weird issues with
global state and with engines like Vulkan you'll get better
performances.
Also, I don't think the bug with higher framerate comes from
OpenGL, it seems really weird to try drawing faster than the
refresh rate anyway and dropping the frames which don't have
the time slot to draw seems to be the correct solution anyway.
> Plus making multiple threads contend for internal resources of the display backend is likely to make performance worse rather than better.
IMHO, that's a different issue. Currently the pictures are
copied so it changes nothing to contention. CPU to GPU upload
can suffer a bit from this but nothing more than the current
GPU to CPU to GPU conversions that I'll fix after this patch.
There are other issues with this design, but it's out of the
scope of simple maintenance. In particular, displays in the
splitter should probably have their own vout_thread, and the
handling of the splitter should probably not be a corner case
in the vout code. But this is more than a day work, not a bug
fix.
Regards,
--
Alexandre Janniaux
Videolabs
> Le 11 décembre 2019 12:32:10 GMT+02:00, Alexandre Janniaux <ajanni at videolabs.io> a écrit :
> >Display modules are expected to present the image to the user from the
> >display callback. Some display can achieve it asynchronously but others
> >having VSYNC mechanisms like OpenGL are forcing the display to sleep
> >until the next VSYNC. In a general way, similar display with a limited
> >number of render buffer will wait until a new buffer gets released and
> >available for processing.
> >
> >While it is correct for one display, the behaviour will make splitter
> >managed display miss the screen frame deadline and generate stuttering
> >with more than one output. It stems from display operations being
> >executed sequentially for each output.
> >
> >Instead, parallelize and synchronize each display's prepare+display so
> >that display taking VSYNC into account don't accumulate sleeping time.
> >---
> > modules/video_output/splitter.c | 132 +++++++++++++++++++++++++++++---
> > 1 file changed, 122 insertions(+), 10 deletions(-)
> >
> >diff --git a/modules/video_output/splitter.c
> >b/modules/video_output/splitter.c
> >index 8d6532ceb4..195bf2c50c 100644
> >--- a/modules/video_output/splitter.c
> >+++ b/modules/video_output/splitter.c
> >@@ -34,23 +34,77 @@
> > #include <vlc_codec.h>
> > #include <vlc_vout_display.h>
> > #include <vlc_video_splitter.h>
> >+#include <vlc_threads.h>
> >
> > struct vlc_vidsplit_part {
> > vout_window_t *window;
> > vout_display_t *display;
> >+ picture_t *picture;
> >+ vlc_tick_t date;
> > vlc_sem_t lock;
> > unsigned width;
> > unsigned height;
> > };
> >
> >+struct vlc_vidsplit_thread
> >+{
> >+ vout_display_t *vd;
> >+ struct vlc_vidsplit_part *part;
> >+ vlc_thread_t thread;
> >+};
> >+
> > struct vout_display_sys_t {
> > video_splitter_t splitter;
> > vlc_mutex_t lock;
> >
> > picture_t **pictures;
> > struct vlc_vidsplit_part *parts;
> >+ struct vlc_vidsplit_thread *threads;
> >+
> >+ vlc_sem_t prepare_wait;
> >+ vlc_sem_t prepare_done;
> >+ vlc_sem_t display_wait;
> >+ vlc_sem_t display_done;
> > };
> >
> >+static void* vlc_vidsplit_ThreadDisplay(void *data)
> >+{
> >+ struct vlc_vidsplit_thread *vd_thread = data;
> >+ vout_display_t *vd = vd_thread->vd;
> >+ vout_display_sys_t *sys = vd->sys;
> >+ struct vlc_vidsplit_part *part = vd_thread->part;
> >+
> >+ for (;;)
> >+ {
> >+ vlc_sem_wait(&sys->prepare_wait);
> >+
> >+ /* We might need to stop the prepare just after the barrier
> >+ * and we were not supposed to take the semaphore token. */
> >+ if (part->display == NULL)
> >+ {
> >+ /* If we don't have a display, we can early exist. */
> >+ vlc_sem_post(&sys->prepare_done);
> >+ continue;
> >+ }
> >+
> >+ part->picture = vout_display_Prepare(part->display,
> >+ part->picture, NULL,
> >+ part->date);
> >+
> >+ /* notify readiness */
> >+ vlc_sem_post(&sys->prepare_done);
> >+
> >+ vlc_sem_wait(&sys->display_wait);
> >+ if (part->picture)
> >+ vout_display_Display(part->display, part->picture);
> >+
> >+ /* notify that image has been displayed */
> >+ vlc_sem_post(&sys->display_done);
> >+ }
> >+
> >+ return NULL;
> >+}
> >+
> > static void vlc_vidsplit_Prepare(vout_display_t *vd, picture_t *pic,
> > subpicture_t *subpic, vlc_tick_t date)
> > {
> >@@ -69,26 +123,61 @@ static void vlc_vidsplit_Prepare(vout_display_t
> >*vd, picture_t *pic,
> > }
> > vlc_mutex_unlock(&sys->lock);
> >
> >+ /* After here, part are locked until all vout display has finished
> >+ * displaying the picture. See vlc_vidsplit_Display. */
> >+ for (int i = 0; i < sys->splitter.i_output; i++)
> >+ vlc_sem_wait(&sys->parts[i].lock);
> >+
> >+ /* Now that all display are waiting, prepare their state and
> >remove unused
> >+ * pictures from killed display. */
> > for (int i = 0; i < sys->splitter.i_output; i++) {
> > struct vlc_vidsplit_part *part = &sys->parts[i];
> >-
> >- vlc_sem_wait(&part->lock);
> >- sys->pictures[i] = vout_display_Prepare(part->display,
> >- sys->pictures[i],
> >NULL, date);
> >+ part->picture = sys->pictures[i];
> >+ part->date = date;
> >+
> >+ if (part->display == NULL)
> >+ {
> >+ /* The display died, cleanup. */
> >+ part->picture = NULL;
> >+ if (sys->pictures[i] != NULL)
> >+ {
> >+ picture_Release(sys->pictures[i]);
> >+ sys->pictures[i] = NULL;
> >+ }
> >+ }
> > }
> >+
> >+ /* Start preparing each vout display. */
> >+ for (int i = 0; i < sys->splitter.i_output; ++i)
> >+ if (sys->parts[i].display != NULL)
> >+ vlc_sem_post(&sys->prepare_wait);
> >+
> >+ /* Wait for each vout display to have finished. */
> >+ for (int i = 0; i < sys->splitter.i_output; ++i)
> >+ if (sys->parts[i].display != NULL)
> >+ vlc_sem_wait(&sys->prepare_done);
> >+
> >+ /* The time to prepare all vout display is the preparation time
> >for the
> >+ * slower of all display, which matches the core requirement. */
> > }
> >
> >static void vlc_vidsplit_Display(vout_display_t *vd, picture_t
> >*picture)
> > {
> > vout_display_sys_t *sys = vd->sys;
> >
> >- for (int i = 0; i < sys->splitter.i_output; i++) {
> >- struct vlc_vidsplit_part *part = &sys->parts[i];
> >+ /* Request each output to display the picture. */
> >+ for (int i = 0; i < sys->splitter.i_output; i++)
> >+ if (sys->parts[i].display != NULL)
> >+ vlc_sem_post(&sys->display_wait);
> >
> >- if (sys->pictures[i] != NULL)
> >- vout_display_Display(part->display, sys->pictures[i]);
> >- vlc_sem_post(&part->lock);
> >- }
> >+ /* Wait until every output has displayed a picture. */
> >+ for (int i = 0; i < sys->splitter.i_output; i++)
> >+ if (sys->parts[i].display != NULL)
> >+ vlc_sem_wait(&sys->display_done);
> >+
> >+ /* Release parts lock, we can't read sys->parts[i] after that. */
> >+ for (int i = 0; i < sys->splitter.i_output; i++)
> >+ vlc_sem_post(&sys->parts[i].lock);
> >
> > (void) picture;
> > }
> >@@ -115,6 +204,7 @@ static void vlc_vidsplit_Close(vout_display_t *vd)
> >
> > for (int i = 0; i < n; i++) {
> > struct vlc_vidsplit_part *part = &sys->parts[i];
> >+ struct vlc_vidsplit_thread *thread = &sys->threads[i];
> > vout_display_t *display;
> >
> > vlc_sem_wait(&part->lock);
> >@@ -122,6 +212,9 @@ static void vlc_vidsplit_Close(vout_display_t *vd)
> > part->display = NULL;
> > vlc_sem_post(&part->lock);
> >
> >+ vlc_cancel(thread->thread);
> >+ vlc_join(thread->thread, NULL);
> >+
> > if (display != NULL)
> > vout_display_Delete(display);
> >
> >@@ -259,12 +352,19 @@ static int vlc_vidsplit_Open(vout_display_t *vd,
> > * sizeof (*sys->pictures));
> > sys->parts = vlc_obj_malloc(obj,
> > splitter->i_output * sizeof (*sys->parts));
> >+ sys->threads = vlc_obj_malloc(obj,
> >+ splitter->i_output * sizeof
> >(*sys->threads));
> > if (unlikely(sys->pictures == NULL || sys->parts == NULL)) {
> > splitter->i_output = 0;
> > vlc_vidsplit_Close(vd);
> > return VLC_ENOMEM;
> > }
> >
> >+ vlc_sem_init(&sys->prepare_wait, 0);
> >+ vlc_sem_init(&sys->prepare_done, 0);
> >+ vlc_sem_init(&sys->display_wait, 0);
> >+ vlc_sem_init(&sys->display_done, 0);
> >+
> > for (int i = 0; i < splitter->i_output; i++) {
> > const video_splitter_output_t *output = &splitter->p_output[i];
> > vout_display_cfg_t vdcfg = {
> >@@ -305,6 +405,18 @@ static int vlc_vidsplit_Open(vout_display_t *vd,
> > part->display = display;
> > vout_display_SetSize(display, part->width, part->height);
> > vlc_sem_post(&part->lock);
> >+
> >+ sys->threads[i].vd = vd;
> >+ sys->threads[i].part = part;
> >+
> >+ int ret_clone = vlc_clone(&sys->threads[i].thread,
> >+ vlc_vidsplit_ThreadDisplay,
> >+ &sys->threads[i],
> >+ VLC_THREAD_PRIORITY_VIDEO);
> >+
> >+ if (ret_clone != VLC_SUCCESS) {
> >+ /* TODO: Abort and cleanup */
> >+ }
> > }
> >
> > vd->prepare = vlc_vidsplit_Prepare;
> >--
> >2.24.1
> >
> >_______________________________________________
> >vlc-devel mailing list
> >To unsubscribe or modify your subscription options:
> >https://mailman.videolan.org/listinfo/vlc-devel
>
> --
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list