[Bug 740803] New: waylandsink: Double-check the frame callback before dropping frames

["GStreamer" (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803
GStreamer | gst-plugins-bad | git

Summary: waylandsink: Double-check the frame callback before
dropping frames
Classification: Platform
Product: GStreamer
Version: git
OS/Version: Linux
Status: UNCONFIRMED
Severity: enhancement
Priority: Normal
Component: gst-plugins-bad
AssignedTo: gstreamer-***@lists.freedesktop.org
ReportedBy: ***@igel.co.jp
QAContact: gstreamer-***@lists.freedesktop.org
GNOME version: ---


Created an attachment (id=291625)
View: https://bugzilla.gnome.org/attachment.cgi?id=291625
Review: https://bugzilla.gnome.org/review?bug=740803&attachment=291625

waylandsink: Double-check the frame callback before dropping frames

I wrote the comment that is the same as the attached patch against waylandsink.

The wl_display events dispatching runs asynchronously in a seperate
thread from the streaming thread. This means that processing a
frame callback event could be slightly late, causing unnecessary
frame drops when the video framerate is the almost same as a display
vsync frequency.

This change alleviates this problem by dispatching any pending events
with wl_display_roundtrip() in the streaming thread before
double-checking whether the frame should actually be dropped.

["GStreamer" (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803
GStreamer | gst-plugins-bad | git

Kazunori Kobayashi <kkobayas> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@igel.co.jp,
| |***@igel.co.jp

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

--- Comment #1 from Damian Hobson-Garcia <***@igel.co.jp> ---
Any comments on this bug and patch? Does this sound reasonable from a wayland
API point of view?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

Daniel Stone <***@fooishbar.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@fooishbar.org

--- Comment #2 from Daniel Stone <***@fooishbar.org> ---
(In reply to Damian Hobson-Garcia from comment #1)

Post by "GStreamer" (GNOME Bugzilla)
Any comments on this bug and patch? Does this sound reasonable from a
wayland API point of view?

You really need to use wl_display_roundtrip_queue, passing sink->display->queue
as the event queue to dispatch on.

Short of that, I don't really know whether blocking in the streaming thread is
sensible, so would have to defer to other GStreamer people.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

Nicolas Dufresne (stormer) <***@collabora.co.uk> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@collabora.
| |co.uk

--- Comment #3 from Nicolas Dufresne (stormer) <***@collabora.co.uk> ---
(In reply to Daniel Stone from comment #2)

Post by "GStreamer" (GNOME Bugzilla)
Short of that, I don't really know whether blocking in the streaming thread
is sensible, so would have to defer to other GStreamer people.

Blocking in the streaming thread can be done, as long as the blocking operation
can be cancelled at anytime (see unlock() virtual / FLUSH_START event). At the
moment, using roundtrip would require creating a specific queue for this
operation.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #291625|none |needs-work
status| |

--- Comment #4 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Review of attachment 291625:
--> (https://bugzilla.gnome.org/review?bug=740803&attachment=291625)

::: ext/wayland/gstwaylandsink.c
@@ +604,3 @@
+ redraw_flag = g_atomic_int_get (&sink->redraw_pending);
+ while (redraw_flag == TRUE && retry > 0) {
+ wl_display_roundtrip (sink->display->display);

wl_display_roundtrip_queue()

@@ +605,3 @@
+ while (redraw_flag == TRUE && retry > 0) {
+ wl_display_roundtrip (sink->display->display);
+ redraw_flag = g_atomic_int_get (&sink->redraw_pending);

This is no longer an atomic op, it's protected by the render_lock now. That
basically means, you'll get a deadlock. You need to drop the render lock before
calling wl_display_roundtrip_queue().

@@ +606,3 @@
+ wl_display_roundtrip (sink->display->display);
+ redraw_flag = g_atomic_int_get (&sink->redraw_pending);
+ retry--;

If retry is 1, why do you keep this code ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

Praveen R Jadhav <***@samsung.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@samsung.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

Post by "GStreamer" (GNOME Bugzilla)
This is no longer an atomic op, it's protected by the render_lock now. That
basically means, you'll get a deadlock. You need to drop the render lock
before calling wl_display_roundtrip_queue().

It is better to check using g_cond_wait_until()/g_cond_signal() APIs and
completely avoid wl_display_roundtrip_queue() IMO. wl_display_roundtrip() is a
blocking call which is not returned until frame_redraw_cb() and
buffer_release() callbacks are not completed. Whereas g_cond may be signalled
as soon as frame_redraw_cb() is received.
Conditional timeout of one frame duration (fps_d/fps_n) is reasonable which can
be finalized after testing further.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

--- Comment #6 from Praveen R Jadhav <***@samsung.com> ---
Created attachment 353731
--> https://bugzilla.gnome.org/attachment.cgi?id=353731&action=edit
waylandsink: Conditional wait for FrameRedrawCallback before dropping the frame

While rendering, if frame_redraw_pending() is not called from wayland server
yet, then render()/expose() APIs should wait for atleast 1 frame duration
before dropping the frame completely.
This conditional wait operation will handle race conditions when render
requests are made from multiple paths.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

Hyunjun Ko <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@igalia.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

--- Comment #7 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Review of attachment 353731:
--> (https://bugzilla.gnome.org/review?bug=740803&attachment=353731)

Why do need to wait for the duration of a frame, additionally to what basesink
already wait for ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

--- Comment #8 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Something I really don't like with this patch, is that it makes the code much
more complicated, specially that you would need to hook this up to unlock().
Its also counter productive, as this should be solved the right way with the
presentation timestamp patch-set.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

George Kiagiadakis <***@tolabaki.gr> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@tolabaki.gr

--- Comment #9 from George Kiagiadakis <***@tolabaki.gr> ---
This patch will create issues in the streaming thread when the wayland server
stops rendering the video window (because it may not be visible or maybe
because the lock screen is on). It is perfectly valid for the server to do this
and may take a long time (in case of the lock screen for example) for the frame
callback to be called. In the meantime, the streaming thread *should* continue
working at the same rate and drop those frames that are not going to be
displayed anyway. If you start waiting an additional frame duration for every
frame sync, then GstBaseSink will find that every one in two frames is
delivered too late and will start firing warnings and QoS events.

(In reply to Praveen R Jadhav from comment #6)

Post by "GStreamer" (GNOME Bugzilla)
This conditional wait operation will handle race conditions when render
requests are made from multiple paths.

I'm not sure how you mean that multiple requests can be made from multiple
paths. If you mean expose() and show_frame() being called at the same time, I
think the solution is to just remove the implementation of expose() completely.
I implemented it a couple of years ago just to follow the pattern for all video
sinks, but I haven't found any practical use for it in wayland.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

--- Comment #10 from Praveen R Jadhav <***@samsung.com> ---
(In reply to George Kiagiadakis from comment #9)

Post by "GStreamer" (GNOME Bugzilla)
This patch will create issues in the streaming thread when the wayland
server stops rendering the video window (because it may not be visible or
maybe because the lock screen is on). It is perfectly valid for the server
to do this and may take a long time (in case of the lock screen for example)
for the frame callback to be called. In the meantime, the streaming thread
*should* continue working at the same rate and drop those frames that are
not going to be displayed anyway. If you start waiting an additional frame
duration for every frame sync, then GstBaseSink will find that every one in
two frames is delivered too late and will start firing warnings and QoS
events.

I too accept that 1 frame duration is a bit longer and is a worst case
scenario. I have tested various scenarios with this patch, but din't find any
case where frames are dropped because of timeout.(Regarding wayland server
delay during screen lock, profiles I work on don't render video in this
scenario). About the streaming scenario, if the wayland server delays
intentionally, then QoS events will be triggered for sure. Reducing wait time
to a lesser duration will minimize render thread faltering.

Post by "GStreamer" (GNOME Bugzilla)
(In reply to Praveen R Jadhav from comment #6)

Post by "GStreamer" (GNOME Bugzilla)
This conditional wait operation will handle race conditions when render
requests are made from multiple paths.

I'm not sure how you mean that multiple requests can be made from multiple
paths. If you mean expose() and show_frame() being called at the same time,
I think the solution is to just remove the implementation of expose()
completely. I implemented it a couple of years ago just to follow the
pattern for all video sinks, but I haven't found any practical use for it in
wayland.

While rendering 2 videos(local and remote) during video call, If user swaps
video screens, then set_render_rect() and expose() are called to update new
display regions. In this scenario, we can clearly observe background if
expose() request to render last frame is dropped. This patch ensures last frame
is updated forcefully and flicker issues are not observed.


@ Nicolas Dufresne: As mentioned above, 1 frame duration is the worst case
scenario. I tested using wl_display_roundtrip_queue()/wl_display_roundtrip().
They are blocking calls and render thread has to wait even though
frame_redraw_callback() is already returned from server. This delays even
further. Using g_cond is one approach reduces overall delay because of blocking
call in render thread. Using presentation time will not help for expose()
cases.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |NEEDINFO

--- Comment #11 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
I'm sorry, but I don't think we will merge this patch. We'll need a better
solution. Ideally if you could provide a small application to reproduce the
issue, that would be ideal. I think your description indicate that this is
relatively easy to simulate.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=740803

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEEDINFO |RESOLVED
CC| |***@coaxion.net
Resolution|--- |INCOMPLETE

--- Comment #12 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Closing this bug report as no further information has been provided. Please
feel free to reopen this bug report if you can provide the information that was
asked for in a previous comment.
Thanks!