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