[vlc-devel] [PATCH] splitter: use a different thread for each display
Alexandre Janniaux
ajanni at videolabs.io
Wed Dec 11 14:33:41 CET 2019
Hi,
On Wed, Dec 11, 2019 at 03:11:27PM +0200, Rémi Denis-Courmont wrote:
> The splitter is not a corner case any longer, I already fixed that for the most part. What's left is necessary for CLI syntax backward compatibility.
It's far better than before, but it's still a corner case.
We're checking whether `video-splitter` has a value and
disable the window creation if so, and the splitter output
is synchronizing the rendering after the vout_thread whereas
the vout_thread is actually the synchronization primitive in
the video pipeline (the one using the clock).
> And it can't work with its own vout thread, otherwise filters and SPU would run twice in the pipeline, which is not the intended behaviour.
>
Why wouldn't it be? Having a splitter module with different
filter on each output doesn't seem like a wrong usecase at
all to me.
However, like I mentioned, this is badly define as is and
would need work before even discussing what is correct and
what is wrong.
This is out of the scope of the review too.
Regards,
--
Alexandre Janniaux
Videolabs
> Le 11 décembre 2019 15:05:54 GMT+02:00, Alexandre Janniaux <ajanni at videolabs.io> a écrit :
> >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
> >_______________________________________________
> >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